Copilot commented on code in PR #3317:
URL: https://github.com/apache/nuttx-apps/pull/3317#discussion_r2686878110
##########
system/ymodem/rb_main.c:
##########
@@ -192,35 +194,51 @@ static int handler(FAR struct ymodem_ctx_s *ctx)
close(priv->fd);
}
- filename = ctx->file_name;
- if (priv->skip_perfix != NULL)
+ temp = ctx->file_name;
+
+ if (priv->skip_prefix != NULL)
{
- size_t len = strlen(priv->skip_perfix);
+ size_t len = strlen(priv->skip_prefix);
- if (strncmp(ctx->file_name, priv->skip_perfix,
+ if (strncmp(ctx->file_name, priv->skip_prefix,
len) == 0)
{
- filename += len;
+ temp += len;
}
}
if (priv->skip_suffix != NULL)
{
- size_t len = strlen(filename);
+ size_t len = strlen(temp);
size_t len2 = strlen(priv->skip_suffix);
- if (len > len2 && strcmp(filename + len - len2,
+ if (len > len2 && strcmp(temp + len - len2,
priv->skip_suffix) == 0)
{
- filename[len - len2] = '\0';
+ temp[len - len2] = '\0';
Review Comment:
Modifying the original filename through the temp pointer is incorrect. The
temp pointer points to ctx->file_name (or an offset within it), which is owned
by the caller. Writing '\0' to this location modifies the original buffer and
could cause side effects. Instead, the suffix should be handled during the
final filename construction in snprintf by calculating the correct length to
copy.
##########
system/ymodem/rb_main.c:
##########
@@ -192,35 +194,51 @@ static int handler(FAR struct ymodem_ctx_s *ctx)
close(priv->fd);
}
- filename = ctx->file_name;
- if (priv->skip_perfix != NULL)
+ temp = ctx->file_name;
+
+ if (priv->skip_prefix != NULL)
{
- size_t len = strlen(priv->skip_perfix);
+ size_t len = strlen(priv->skip_prefix);
- if (strncmp(ctx->file_name, priv->skip_perfix,
+ if (strncmp(ctx->file_name, priv->skip_prefix,
len) == 0)
{
- filename += len;
+ temp += len;
}
}
if (priv->skip_suffix != NULL)
{
- size_t len = strlen(filename);
+ size_t len = strlen(temp);
size_t len2 = strlen(priv->skip_suffix);
- if (len > len2 && strcmp(filename + len - len2,
+ if (len > len2 && strcmp(temp + len - len2,
priv->skip_suffix) == 0)
{
- filename[len - len2] = '\0';
+ temp[len - len2] = '\0';
}
}
if (priv->foldname != NULL)
{
- snprintf(temp, sizeof(temp), "%s/%s", priv->foldname,
- filename);
- filename = temp;
+ ret = snprintf(filename, sizeof(filename), "%s/%s%s%s",
+ priv->foldname, priv->prepend_prefix, temp,
+ priv->append_suffix);
+ }
+ else
+ {
+ ret = snprintf(filename, sizeof(filename), "%s%s%s",
+ priv->prepend_prefix, temp, priv->append_suffix);
+ }
+
+ if (ret < 0)
+ {
+ return ret;
+ }
+
+ else if (ret >= sizeof(filename))
Review Comment:
The 'else if' on line 239 should not have an 'else' since the previous 'if'
block on line 234 already returns. This creates unnecessary nesting and reduces
code clarity. The condition should be a simple 'if' statement.
```suggestion
if (ret >= sizeof(filename))
```
##########
system/ymodem/rb_main.c:
##########
@@ -382,9 +409,10 @@ int main(int argc, FAR char *argv[])
break;
case 'f':
priv.foldname = optarg;
- if (priv.foldname[strlen(priv.foldname)] == '/')
+ len = strlen(priv.foldname);
+ if (len > 0 && priv.foldname[len - 1] == '/')
{
- priv.foldname[strlen(priv.foldname)] = '\0';
+ priv.foldname[len - 1] = '\0';
Review Comment:
Modifying optarg by writing '\0' to it is incorrect. The optarg pointer is
managed by getopt_long and points to the argument string. Modifying it directly
could cause side effects or memory issues since it may point to argv[] which
shouldn't be modified. Instead, consider creating a copy of foldname if you
need to remove the trailing slash, or handle the slash during path construction.
##########
system/ymodem/rb_main.c:
##########
@@ -326,6 +344,10 @@ static void show_usage(FAR const char *progname)
"\t-p|--skip_prefix <prefix>: Remove file name prefix\n");
fprintf(stderr,
"\t-s|--skip_suffix <suffix>: Remove file name suffix\n");
+ fprintf(stderr,
+ "\t-e|--prepend_prefix <prefix>: prepend file name a prefix\n");
+ fprintf(stderr,
+ "\t-a|--append_suffix <suffix>: append file name a suffix\n");
Review Comment:
The help text has a grammatical error. It should read "prepend a prefix to
file name" rather than "prepend file name a prefix".
```suggestion
"\t-e|--prepend_prefix <prefix>: prepend a prefix to file name\n");
fprintf(stderr,
"\t-a|--append_suffix <suffix>: append a suffix to file name\n");
```
##########
system/ymodem/rb_main.c:
##########
@@ -326,6 +344,10 @@ static void show_usage(FAR const char *progname)
"\t-p|--skip_prefix <prefix>: Remove file name prefix\n");
fprintf(stderr,
"\t-s|--skip_suffix <suffix>: Remove file name suffix\n");
+ fprintf(stderr,
+ "\t-e|--prepend_prefix <prefix>: prepend file name a prefix\n");
+ fprintf(stderr,
+ "\t-a|--append_suffix <suffix>: append file name a suffix\n");
Review Comment:
The help text has a grammatical error. It should read "append a suffix to
file name" rather than "append file name a suffix".
```suggestion
"\t-e|--prepend_prefix <prefix>: prepend a prefix to file name\n");
fprintf(stderr,
"\t-a|--append_suffix <suffix>: append a suffix to file name\n");
```
--
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]
