You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2023/01/19 15:21:36 UTC
[GitHub] [nuttx] acassis opened a new pull request, #8194: fs: Avoid accessing filep fields if it is NULL & add DEBUGASSERTs
acassis opened a new pull request, #8194:
URL: https://github.com/apache/nuttx/pull/8194
## Summary
Avoid accessing filep fields if it is NULL & add DEBUGASSERTs
## Impact
Avoid crashes
## Testing
esp32-devkit
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] acassis commented on a diff in pull request #8194: fs: Avoid accessing filep fields if it is NULL & add DEBUGASSERTs
Posted by GitBox <gi...@apache.org>.
acassis commented on code in PR #8194:
URL: https://github.com/apache/nuttx/pull/8194#discussion_r1082431352
##########
fs/inode/fs_files.c:
##########
@@ -298,7 +301,10 @@ int files_duplist(FAR struct filelist *plist, FAR struct filelist *clist)
#endif
filep = &plist->fl_files[i][j];
- if (filep->f_inode == NULL || (filep->f_oflags & O_CLOEXEC) != 0)
+ DEBUGASSERT(filep);
+
+ if (filep && (filep->f_inode == NULL ||
+ (filep->f_oflags & O_CLOEXEC) != 0))
Review Comment:
Actually the original fix as only that "if (filep && (...))", I added the DEBUGASSET later because it will avoid some silent issue that could happen in some cases.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] pkarashchenko commented on a diff in pull request #8194: fs: Avoid accessing filep fields if it is NULL & add DEBUGASSERTs
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #8194:
URL: https://github.com/apache/nuttx/pull/8194#discussion_r1082239504
##########
fs/inode/fs_files.c:
##########
@@ -298,7 +301,10 @@ int files_duplist(FAR struct filelist *plist, FAR struct filelist *clist)
#endif
filep = &plist->fl_files[i][j];
- if (filep->f_inode == NULL || (filep->f_oflags & O_CLOEXEC) != 0)
+ DEBUGASSERT(filep);
+
+ if (filep && (filep->f_inode == NULL ||
+ (filep->f_oflags & O_CLOEXEC) != 0))
Review Comment:
I do not understand this. Why do we need `DEBUGASSERT(filep);` and `if (filep`? I would expect code to be
```suggestion
DEBUGASSERT(filep);
if (filep->f_inode == NULL ||
(filep->f_oflags & O_CLOEXEC) != 0)
```
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] acassis commented on pull request #8194: fs: Avoid accessing filep fields if it is NULL & add DEBUGASSERTs
Posted by GitBox <gi...@apache.org>.
acassis commented on PR #8194:
URL: https://github.com/apache/nuttx/pull/8194#issuecomment-1397572188
@xiaoxiang781216 any idea why we still getting this error:
```
make[1]: *** [uClibc++.defs:26: uClibc++] Error 6
make[1]: Target 'context' not remade because of errors.
make: *** [tools/Unix.mk:432: libs/libxx/.context] Error 2
make: Target 'all' not remade because of errors.
/github/workspace/sources/nuttx/tools/testbuild.sh: line 338: /github/workspace/sources/nuttx/../nuttx/nuttx.manifest: No such file or directory
Normalize stm32f4discovery/cxxtest
```
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] pkarashchenko commented on a diff in pull request #8194: fs: Avoid accessing filep fields if it is NULL & add DEBUGASSERTs
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on code in PR #8194:
URL: https://github.com/apache/nuttx/pull/8194#discussion_r1082963845
##########
fs/inode/fs_files.c:
##########
@@ -298,7 +301,10 @@ int files_duplist(FAR struct filelist *plist, FAR struct filelist *clist)
#endif
filep = &plist->fl_files[i][j];
- if (filep->f_inode == NULL || (filep->f_oflags & O_CLOEXEC) != 0)
+ DEBUGASSERT(filep);
+
+ if (filep && (filep->f_inode == NULL ||
+ (filep->f_oflags & O_CLOEXEC) != 0))
Review Comment:
But in such case if `filep` is checked unconditionally shouldn't we move the assert before `continue`, so that will eliminate unnecessary duplicated NULL check?
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 merged pull request #8194: fs: Avoid accessing filep fields if it is NULL & add DEBUGASSERTs
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged PR #8194:
URL: https://github.com/apache/nuttx/pull/8194
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 commented on pull request #8194: fs: Avoid accessing filep fields if it is NULL & add DEBUGASSERTs
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #8194:
URL: https://github.com/apache/nuttx/pull/8194#issuecomment-1398020868
> @xiaoxiang781216 any idea why we still getting this error:
>
> ```
> make[1]: *** [uClibc++.defs:26: uClibc++] Error 6
> make[1]: Target 'context' not remade because of errors.
> make: *** [tools/Unix.mk:432: libs/libxx/.context] Error 2
> make: Target 'all' not remade because of errors.
> /github/workspace/sources/nuttx/tools/testbuild.sh: line 338: /github/workspace/sources/nuttx/../nuttx/nuttx.manifest: No such file or directory
> Normalize stm32f4discovery/cxxtest
> ```
Download fail?
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org