Re: [PATCH] modpost: remove use of non-standard strsep() in HOSTCC code

[H. Nikolaus Schaller]

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