Copilot commented on code in PR #17839:
URL: https://github.com/apache/nuttx/pull/17839#discussion_r2682498151
##########
fs/driver/fs_blockproxy.c:
##########
@@ -149,17 +147,18 @@ int block_proxy(FAR struct file *filep, FAR const char
*blkdev, int oflags)
{
struct file temp;
FAR char *chardev;
Review Comment:
The variable 'chardev' is declared as a pointer but is never assigned a
buffer. This will cause undefined behavior when passed to unique_chardev. This
should be changed to a local array like 'char chardev[16]' to match the pattern
used in fs_mtdproxy.c.
```suggestion
FAR char chardev[16];
```
##########
fs/driver/fs_blockproxy.c:
##########
@@ -149,17 +147,18 @@ int block_proxy(FAR struct file *filep, FAR const char
*blkdev, int oflags)
{
struct file temp;
FAR char *chardev;
+ bool readonly;
int ret;
DEBUGASSERT(blkdev);
/* Create a unique temporary file name for the character device */
- chardev = unique_chardev();
- if (chardev == NULL)
+ ret = unique_chardev(chardev, sizeof(chardev));
Review Comment:
The sizeof(chardev) will return the size of a pointer (typically 4 or 8
bytes), not the size of a buffer. This will lead to potential buffer overflow
when snprintf tries to write the device name. The buffer should be declared as
a local array with a fixed size.
##########
fs/driver/fs_mtdproxy.c:
##########
@@ -71,10 +71,9 @@ static mutex_t g_devno_lock = NXMUTEX_INITIALIZER;
*
****************************************************************************/
Review Comment:
The documentation comment is outdated. It still describes the old behavior
where the function allocated and returned a device name string that needed to
be freed. The comment should be updated to reflect that the function now takes
a buffer and size parameters and returns an integer status code.
##########
fs/driver/fs_blockproxy.c:
##########
@@ -78,10 +78,9 @@ static mutex_t g_devno_lock = NXMUTEX_INITIALIZER;
*
****************************************************************************/
Review Comment:
The documentation comment is outdated. It still describes the old behavior
where the function allocated and returned a device name string that needed to
be freed. The comment should be updated to reflect that the function now takes
a buffer and size parameters and returns an integer status code.
##########
fs/driver/fs_mtdproxy.c:
##########
@@ -180,17 +178,20 @@ int mtd_proxy(FAR const char *mtddev, int mountflags,
if (ret < 0)
{
ferr("ERROR: Failed to open %s: %d\n", blkdev, ret);
- goto out_with_fltdev;
+ nx_unlink(blkdev);
+ return ret;
}
/* Unlink and free the block device name. The driver instance will
* persist, provided that CONFIG_DISABLE_PSEUDOFS_OPERATIONS=y (otherwise,
* we have a problem here!)
*/
-out_with_fltdev:
- nx_unlink(blkdev);
-out_with_blkdev:
- fs_heap_free(blkdev);
+ ret = nx_unlink(blkdev);
+ if (ret < 0)
+ {
+ ferr("ERROR: Failed to unlink %s: %d\n", blkdev, ret);
+ }
+
return ret;
Review Comment:
The return value logic is incorrect. After successful open_blockdriver at
line 177, the function should return OK to indicate success. However, line 190
reassigns ret to the result of nx_unlink. If nx_unlink fails, the function will
return an error code even though the main operation (opening the block driver
proxy) succeeded. The function should save the success state or simply return
OK after logging any unlink errors.
##########
fs/driver/fs_blockproxy.c:
##########
@@ -149,17 +147,18 @@ int block_proxy(FAR struct file *filep, FAR const char
*blkdev, int oflags)
{
struct file temp;
FAR char *chardev;
+ bool readonly;
Review Comment:
The variable 'readonly' is declared but never used in this function. This
appears to be dead code that should be removed.
```suggestion
```
##########
fs/driver/fs_blockproxy.c:
##########
@@ -200,19 +199,8 @@ int block_proxy(FAR struct file *filep, FAR const char
*blkdev, int oflags)
if (ret < 0)
{
ferr("ERROR: Failed to unlink %s: %d\n", chardev, ret);
- goto errout_with_chardev;
}
- /* Free the allocated character driver name. */
-
- fs_heap_free(chardev);
- return OK;
-
-errout_with_bchdev:
- nx_unlink(chardev);
-
-errout_with_chardev:
- fs_heap_free(chardev);
return ret;
Review Comment:
The return value logic is incorrect. When nx_unlink succeeds and returns OK
(0), the function will return OK, which is correct. However, when nx_unlink
fails, the function will return the error from unlink instead of OK, even
though the main operation (opening the proxy) succeeded. The function should
return OK on success regardless of the unlink result, and only log errors from
unlink.
```suggestion
/* Return success because the main operation (opening the proxy) succeeded,
* regardless of any unlink failure above.
*/
return OK;
```
--
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]
