Any word on getting this fixed in the repo?
--
Rob Mosher
Senior Network and Software Engineer
Hurricane Electric / AS6939
On 8/8/2018 5:43 PM, Rob Mosher wrote:
Sorry for not looking into it deeper, it was just a quick patch to fix
the specific strncpy error. I didn't even pay attention to the fact
it was returning strdup(line). Was just trying to get this to build
with gcc8 real quick. There's a full diff with some additional
changes and info below.
Setting a pointer to the end of the leading spaces then truncating the
trailing spaces with \0 as you've done is a better approach.
Also I wonder what happens if line only contains spaces. Then
while (isspace(line[strlen(line) - 1]))
line[strlen(line) - 1] = '\0';
accesses line[-1] in the end.
strlen will always be at least 1. fgets() includes any carriage
returns and returns NULL on EOF, so the string will not be empty. If
the file is empty, preparsed will be set to NULL, and the function
returns.
preparsed = fgets(line, sizeof(line), f);
if (!preparsed) {
...
return NULL;
This line is also not necessary, as fgets adds a terminating \0:
line[sizeof(line) - 1] = '\0';
/* find the first non-space char */
while (*linestart && isspace(*linestart))
++linestart;
The above code will eventually hit the terminating \0, so we don't
need to use strnlen for the next loop, as it's guaranteed terminated
by fgets. You can just do:
len = strlen(linestart);
while (len && isspace(linestart[len - 1]))
--len;
linestart[len] = '\0';
I tested this with an empty file, file with just spaces, missing
carriage return, etc. Seems to work as intended.
$ tail -n2 test.c
printf("%s:%s:\n", argv[1], read_file(argv[1]));
}
$ >omg
$ ./test omg
Could not read data from MMC/SD file 'omg'.
omg:(null):
$ echo -n hi > omg
$ ./test omg
omg:hi:
$ echo ' hi ' > omg
$ ./test omg
omg:hi:
$ echo ' ' > omg
$ ./test omg
omg::
$ git diff
diff --git a/lsmmc.c b/lsmmc.c
index c4faa00..a355f89 100644
--- a/lsmmc.c
+++ b/lsmmc.c
@@ -317,7 +317,8 @@ int parse_ids(struct config *config)
char *read_file(char *name)
{
char *preparsed;
- char line[4096];
+ char line[4096], *linestart = line;
+ size_t len;
FILE *f;
f = fopen(name, "r");
@@ -347,15 +348,18 @@ char *read_file(char *name)
return NULL;
}
- line[sizeof(line) - 1] = '\0';
+ /* find the first non-space char */
+ while (*linestart && isspace(*linestart))
+ ++linestart;
- while (isspace(line[strlen(line) - 1]))
- line[strlen(line) - 1] = '\0';
+ /* right-strip the remaining string */
+ len = strlen(linestart);
+ while (len && isspace(linestart[len - 1]))
+ --len;
- while (isspace(line[0]))
- strncpy(&line[0], &line[1], sizeof(line));
+ linestart[len] = '\0';
- return strdup(line);
+ return strdup(linestart);
}
/* Hexadecimal string parsing functions */
--
Rob Mosher
Senior Network and Software Engineer
Hurricane Electric / AS6939
On 8/8/2018 5:04 PM, Uwe Kleine-König wrote:
[adding linux-mmc ML and kernel@xxxxxxxxxxxxxx to Cc:]
Hello Rob,
On Wed, Aug 08, 2018 at 04:34:49PM -0400, Rob Mosher wrote:
I see you guys committed recently to mmc-utils, so sending this over.
Cheers
lsmmc.c:356:3: error: 'strncpy' accessing 4096 bytes at offsets 0 and 1
overlaps 4095 bytes at offset 1 [-Werror=restrict]
strncpy(&line[0], &line[1], sizeof(line));
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Right, strncpy(3) specifies that the strings must not overlap.
$ git show
commit 24a86c433e7e9cfd37f3d4688871036b5afea08b
Author: Rob Mosher <nyt-kernel@xxxxxxxxxxxxxxxxxxx>
Date: Wed Aug 8 15:55:40 2018 -0400
Fix strncpy overlap
This fixes an overlap condition when strncpy is called
Signed-off-by: Rob Mosher <nyt-kernel@xxxxxxxxxxxxxxxxxxx>
diff --git a/lsmmc.c b/lsmmc.c
index c4faa00..893f8e1 100644
--- a/lsmmc.c
+++ b/lsmmc.c
@@ -318,6 +318,7 @@ char *read_file(char *name)
{
char *preparsed;
char line[4096];
+ int i, space_count = 0;
FILE *f;
f = fopen(name, "r");
@@ -352,8 +353,13 @@ char *read_file(char *name)
while (isspace(line[strlen(line) - 1]))
line[strlen(line) - 1] = '\0';
- while (isspace(line[0]))
- strncpy(&line[0], &line[1], sizeof(line));
+ while (isspace(line[space_count]))
+ space_count++;
+
+ if (space_count > 0) {
+ for(i=0;line[i] != '\0';i++)
+ line[i] = line[i+space_count];
+ }
Your patch does two things at once:
- Instead of moving the string by 1 for each space, count the spaces
and move in one go
- open code strncpy in a variant that works.
Both should be noted in the commit log.
Also I wonder what happens if line only contains spaces. Then
while (isspace(line[strlen(line) - 1]))
line[strlen(line) - 1] = '\0';
accesses line[-1] in the end.
Note however that moving the whitespace stripped string in the line
buffer is not needed at all given that the function in question just
returns
strdup(line)
.
So it should be good enough to do something like:
char *linestart = line;
size_t len;
/* find the first non-space char */
while (*linestart && isspace(*linestart))
++linestart;
/* right-strip the remaining string */
len = strnlen(linestart, sizeof(line) - (linestart - line) - 1);
while (len && isspace(linestart[len - 1]))
--len;
linestart[len] = '\0';
return strdup(linestart);
This is untested however and I didn't verify if the 2nd parameter to
strnlen is right, maybe the -1 can be dropped?
Best regards
Uwe
