ChibiOS Free Embedded RTOS

Hi,

I've been attempting to track down the cause of the following assertion in chSemSignal() in version 2.6.0 of ChibiOS:

void chSemSignal(Semaphore *sp) {

  chDbgCheck(sp != NULL, "chSemSignal");
  chDbgAssert(((sp->s_cnt >= 0) && isempty(&sp->s_queue)) ||
              ((sp->s_cnt < 0) && notempty(&sp->s_queue)),
              "chSemSignal(), #1",
              "inconsistent semaphore");

  chSysLock();
  if (++sp->s_cnt <= 0)
    chSchWakeupS(fifo_remove(&sp->s_queue), RDY_OK);
  chSysUnlock();
}

Is there a reason why this check is not performed under a chSysLock() context to prevent modifications to the semaphore while performing this check, and would it be safe to simply move the chSysLock() before this assertion check. I'm basing this on the fact that almost every other other OS function (except S-class or I-class APIs) will perform a system lock before checking for assertions, and that it would be safe for me to make this modification.

Note that this potentially adverse behaviour is present in the HEAD of the ChibiOS repository in GitHub (https://github.com/ChibiOS/ChibiOS/blob ... rc/chsem.c)

Regards,
Craig

Hi,

Good point, it is meant to be done inside the critical zone, moving this in "bug reports".

Giovanni

Hi Giovanni,

It may be worthwhile performing an audit through the kernel code to look for other issues like this - a similar issue exists in the chSemSignalWait() function.

Regards,
Craig

mmmmm Ive also seen this.

i take it the fixed would be to move the lock to include teh assert ? i.e

Correct, assertions on critical kernel data should be protected by a critical zone, moving the chSysLock() is the correct action.

It is not a critical problem but still a problem.

Giovanni

Hi,

Fixed as bug #865. There were no more instances of the problem in RT code.

Giovanni