pkarashchenko commented on a change in pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#discussion_r783045941
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -954,11 +867,8 @@ void nxsem_add_holder_tcb(FAR struct tcb_s *htcb, FAR
sem_t *sem)
pholder = nxsem_findorallocateholder(sem, htcb);
if (pholder != NULL)
{
- /* Then set the holder and increment the number of counts held by
- * this holder
- */
+ /* Increment the number of counts held by this holder */
- pholder->htcb = htcb;
pholder->counts++;
Review comment:
I still believe that this will bring bugs in case of 1 producer and 2
consumers signaling model. For example there are 3 tasks. Task 1 does
`sem_post` and Task 2 and 3 are calling `sem_wait`. For example Task 1 is the
highest priority and does 2 `sem_post` in a row. The Task 2 consumes 1 count
and does some work and Task 3 consumes 1 count and does some work. Then it is
repeated cyclically. In this case Task 2 and 3 will be sem holders always
(because we do not decrement `pholders->counts` in `nxsem_release_holders`) and
`pholders->counts` will overflow at some point and `nxsem_freecount0holder` or
`nxsem_restoreholderprio` will finally execute `nxsem_freeholder(sem,
pholder);` so all the system will be in the undefined behavior.
I really think that we need to limit `pholder->counts` with `SEM_VALUE_MAX`
and have here something like
```
if (pholder->counts < SEM_VALUE_MAX)
{
pholder->counts++;
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
