pkarashchenko commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r821660668
##########
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.
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.
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.
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.
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:
```
static ssize_t ajoy_read(FAR struct file *filep, FAR char *buffer,
size_t len)
{
FAR struct inode *inode;
FAR struct ajoy_open_s *opriv;
FAR struct ajoy_upperhalf_s *priv;
FAR const struct ajoy_lowerhalf_s *lower;
irqstate_t flags;
int ret;
DEBUGASSERT(filep && filep->f_inode);
inode = filep->f_inode;
DEBUGASSERT(inode->i_private);
priv = (FAR struct ajoy_upperhalf_s *)inode->i_private;
/* Make sure that the buffer is sufficiently large to hold at least one
* complete sample.
*
* REVISIT: Should also check buffer alignment.
*/
if (len < sizeof(struct ajoy_sample_s))
{
ierr("ERROR: buffer too small: %lu\n", (unsigned long)len);
return -EINVAL;
}
/* Read and return the current state of the joystick buttons */
lower = priv->au_lower;
DEBUGASSERT(lower && lower->al_sample);
ret = lower->al_sample(lower, (FAR struct ajoy_sample_s *)buffer);
if (ret >= 0)
{
/* Get exclusive access to the driver instance structure */
flags = enter_critical_section();
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;
}
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
