xiaoxiang781216 commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r821674599
##########
File path: drivers/input/ajoystick.c
##########
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char
*buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(&priv->au_exclsem);
- if (ret < 0)
- {
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
- }
+ flags = enter_critical_section();
Review comment:
> So what is not handled in this case?
>
> 1. Thread A just enter ajoy_close and switch to thread B. -- I assume that
this is done either before `ajoy_takesem(&priv->au_exclsem);` or inside the
`ajoy_takesem(&priv->au_exclsem);` call.
I mean before take au_exclsem.
> 2. Thread B just enter ajoy_read and switch to thread A again. -- This
will either switch before `ret = ajoy_takesem(&priv->au_exclsem);` or inside
the `ret = ajoy_takesem(&priv->au_exclsem);` call.
I mean before take au_exclsem too.
> 3. Thread A continue ajoy_close and release opriv -- Yes, so in this case
we need to access `filep->f_priv` (`opriv`) after `ret =
ajoy_takesem(&priv->au_exclsem);` is successful and `filep->f_priv` should be
set to `NULL` after memory is freed.
Yes.
> 4. Thread B resume and touch the freed memory -- this will not happen if
`ajoy_takesem(&priv->au_exclsem);` is successfully taken and `filep->f_priv` is
accessed after that.
>
Once ajoy_close return at the step 3, filep may be reused by VFS layer and
then f_priv may point to a different struct, which make the check of
filep->f_priv useless.
> My assumption is next: The `priv->au_exclsem` protects any modification of
`node->i_private` data and `filep->f_priv` data. If there is no modification,
then we do not need to take `priv->au_exclsem`. We can try to leverage critical
section approach. In other words the `ajoy_read` should look like:
>
> ```
> /* Get exclusive access to the driver structure */
>
> if (ret >= 0)
> {
> flags = enter_critical_section();
>
> DEBUGASSERT(filep && filep->f_inode);
> opriv = filep->f_priv;
>
> if (opriv != NULL)
> {
> opriv->ao_pollpending = false;
> ret = sizeof(struct ajoy_sample_s);
> }
> else
> {
> ret = -EPIPE;
> }
>
> leave_critical_section(flags);
> }
>
> return (ssize_t)ret;
> }
> ```
neither semaphore nor critical section can resolve the above sequence.
To handle the race condition between destruct and other paired functions,
the best approach is the reference count and delay release, other methods may
look like work, but not work really.
--
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
