On Sun, Jun 28, 2020 at 3:17 PM H. Nikolaus Schaller <hns@xxxxxxxxxxxxx> wrote: > > Hi, > > > Am 28.06.2020 um 07:51 schrieb Masahiro Yamada <masahiroy@xxxxxxxxxx>: > > > > On Thu, Jun 25, 2020 at 5:47 PM H. Nikolaus Schaller <hns@xxxxxxxxxxxxx> wrote: > >> > >> strsep() is neither standard C nor POSIX and used outside > >> the kernel code here. Using it here requires that the > >> build host supports it out of the box which is e.g. > >> not true for a Darwin build host and using a cross-compiler. > >> This leads to: > >> > >> scripts/mod/modpost.c:145:2: warning: implicit declaration of function 'strsep' [-Wimplicit-function-declaration] > >> return strsep(stringp, "\n"); > >> ^ > >> > >> and a segfault when running MODPOST. > >> > >> See also: https://stackoverflow.com/a/7219504 > >> > >> So let's add some lines of code separating the string at the > >> next newline character instead of using strsep(). It does not > >> hurt kernel size or speed since this code is run on the build host. > >> > >> Fixes: ac5100f5432967 ("modpost: add read_text_file() and get_line() helpers") > >> Signed-off-by: H. Nikolaus Schaller <hns@xxxxxxxxxxxxx> > >> --- > >> scripts/mod/modpost.c | 7 ++++++- > >> 1 file changed, 6 insertions(+), 1 deletion(-) > >> > >> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c > >> index 6aea65c65745..8fe63989c6e1 100644 > >> --- a/scripts/mod/modpost.c > >> +++ b/scripts/mod/modpost.c > >> @@ -138,11 +138,16 @@ char *read_text_file(const char *filename) > >> > >> char *get_line(char **stringp) > >> { > >> + char *p; > >> /* do not return the unwanted extra line at EOF */ > >> if (*stringp && **stringp == '\0') > > > > This check does not make sense anymore. > > > > Previously, get_line(NULL) returns NULL. > > > > With your patch, get_line(NULL) crashes > > due to NULL-pointer dereference. > > Well, that is original code. Sorry for confusion. I meant this: char *s = NULL; get_line(&s); In the current code, get_line(&s) returns NULL. As 'man strsep' says this: "If *stringp is NULL, the strsep() function returns NULL and does nothing else." With your patch, **stringp will cause NULL-pointer dereference. > > I have only replaced the strsep() function. > But yes, it looks to be better in addition to > my patch. > > > > > > > > >> return NULL; > >> > >> - return strsep(stringp, "\n"); > >> + p = *stringp; > >> + while (**stringp != '\n') > >> + (*stringp)++; > > > > > > Is this a safe conversion? > > > > If the input file does not contain '\n' at all, > > this while-loop continues running, > > and results in the segmentation fault > > due to buffer over-run. > > Ah, yes, you are right. > > We should use > > + while (**stringp && **stringp != '\n') > > > > > > > > >> + *(*stringp)++ = '\0'; > >> + return p; > >> } > > > > > > > > How about this? > > > > char *get_line(char **stringp) > > { > > char *orig = *stringp; > > ^^^ this still segfaults with get_line(NULL) This is OK. get_line(NULL) should crash because we never expect the case ' stringp == NULL'. We need to care about the case ' *stringp == NULL'. In this case, get_line() should return NULL. > > char *next; > > > > /* do not return the unwanted extra line at EOF */ > > if (!orig || *orig == '\0') > > return NULL; > > > > next = strchr(orig, '\n'); > > if (next) > > *next++ = '\0'; > > > > *stringp = next; > > Yes, this code is easier to understand than my while loop. > And strchr() is POSIX. > > So should I submit an updated patch or do you want to submit > it (with a suggested-by: H. Nikolaus Schaller <hns@xxxxxxxxxxxxx>) Please send a patch. (Co-developed-by if you want to give some credit to me) > BR and thanks, > Nikolaus Schaller > > -- Best Regards Masahiro Yamada
