Messages in this thread | | | Subject | Re: [PATCH] modpost: remove use of non-standard strsep() in HOSTCC code | From | "H. Nikolaus Schaller" <> | Date | Sun, 28 Jun 2020 10:20:04 +0200 |
| |
> Am 28.06.2020 um 09:52 schrieb Masahiro Yamada <masahiroy@kernel.org>: > > On Sun, Jun 28, 2020 at 3:17 PM H. Nikolaus Schaller <hns@goldelico.com> wrote: >> >> Hi, >> >>> Am 28.06.2020 um 07:51 schrieb Masahiro Yamada <masahiroy@kernel.org>: >>> >>> On Thu, Jun 25, 2020 at 5:47 PM H. Nikolaus Schaller <hns@goldelico.com> 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@goldelico.com> >>>> --- >>>> 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.
Ah, now I see. strsep() has a special case that is not covered by my patch.
On the other hand, get_line() is only called as get_line(&pos) and pos = buf can not be NULL because that is checked before in read_dump(). This is why I did not observe a segfault.
But it is wise to make get_line() it more robust than needed. We do never know who will copy this code fragment... And I am tempted to handle the get_line(NULL) case as well.
>> 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@goldelico.com>) > > Please send a patch. > (Co-developed-by if you want to give some credit to me)
Yes, I will do in the next days.
BR and thanks, Nikolaus Schaller
| |