On Thu, Aug 16, 2018 at 08:07:48PM -0400, Rob Mosher wrote:
> Any word on getting this fixed in the repo?
It's not in my domain to do this. I added Chris and Ulf explicitly to
To:, I think Chris must be convinced this is a good idea.
Best regards
Uwe
>
> --
> 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
> > >
> >
>
>
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
