chSemSignal assertion Topic is solved

[craig.sacco]

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

[Giovanni]

Hi,

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

Giovanni

[craig.sacco]

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

[Tabulous]

mmmmm Ive also seen this.

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

Code: Select all

void chSemSignal(Semaphore *sp) {

  chDbgCheck(sp != NULL, "chSemSignal");
  chSysLock();
  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();
}

[Giovanni]

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

[Giovanni]

Hi,

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

Giovanni