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. 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) > 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>) BR and thanks, Nikolaus Schaller
