Re: mmc-utils patch to fix strncpy mem overlap

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Memonry Technology]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux