chSemSignal assertion Topic is solved

Report here problems in any of ChibiOS components. This forum is NOT for support.
User avatar
craig.sacco
Posts: 5
Joined: Mon Jul 17, 2017 2:22 am
Location: Melbourne, Australia
Has thanked: 2 times
Been thanked: 2 times
Contact:

chSemSignal assertion  Topic is solved

Postby craig.sacco » Mon Jul 17, 2017 2:35 am

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

User avatar
Giovanni
Site Admin
Posts: 10240
Joined: Wed May 27, 2009 8:48 am
Location: Salerno, Italy
Has thanked: 212 times
Been thanked: 198 times
Contact:

Re: chSemSignal assertion

Postby Giovanni » Mon Jul 17, 2017 7:39 am

Hi,

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

Giovanni

User avatar
craig.sacco
Posts: 5
Joined: Mon Jul 17, 2017 2:22 am
Location: Melbourne, Australia
Has thanked: 2 times
Been thanked: 2 times
Contact:

Re: chSemSignal assertion

Postby craig.sacco » Mon Jul 17, 2017 10:21 pm

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
Posts: 421
Joined: Fri May 03, 2013 12:02 pm
Has thanked: 1 time
Been thanked: 10 times

Re: chSemSignal assertion

Postby Tabulous » Tue Jul 18, 2017 9:17 am

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();
}

User avatar
Giovanni
Site Admin
Posts: 10240
Joined: Wed May 27, 2009 8:48 am
Location: Salerno, Italy
Has thanked: 212 times
Been thanked: 198 times
Contact:

Re: chSemSignal assertion

Postby Giovanni » Tue Jul 18, 2017 9:38 am

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

User avatar
Giovanni
Site Admin
Posts: 10240
Joined: Wed May 27, 2009 8:48 am
Location: Salerno, Italy
Has thanked: 212 times
Been thanked: 198 times
Contact:

Re: chSemSignal assertion

Postby Giovanni » Sat Jul 22, 2017 10:55 am

Hi,

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

Giovanni


Return to “Bug Reports”

Who is online

Users browsing this forum: No registered users and 1 guest