[PATCH v3 4/5] lib: membuff: fix readline not returning line in case of overflow

[Heinrich Schuchardt]

On 11/21/23 19:40, Simon Glass wrote:
> Hi Svyatoslav,
>
> On Tue, 21 Nov 2023 at 11:35, Svyatoslav Ryhel <clamor95 at gmail.com> wrote:
>>
>> From: Ion Agorria <ion at agorria.com>
>>
>> If line overflows readline it will not be returned, fix this behavior,
>> make it optional and documented properly.
>>
>> Signed-off-by: Ion Agorria <ion at agorria.com>
>> Signed-off-by: Svyatoslav Ryhel <clamor95 at gmail.com>
>> Reviewed-by: Mattijs Korpershoek <mkorpershoek at baylibre.com>
>> ---
>>   boot/bootmeth_extlinux.c | 2 +-
>>   common/console.c         | 2 +-
>>   include/membuff.h        | 5 +++--
>>   lib/membuff.c            | 4 ++--
>>   4 files changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/boot/bootmeth_extlinux.c b/boot/bootmeth_extlinux.c
>> index aa2a4591eb..ae0ad1d53e 100644
>> --- a/boot/bootmeth_extlinux.c
>> +++ b/boot/bootmeth_extlinux.c
>> @@ -82,7 +82,7 @@ static int extlinux_fill_info(struct bootflow *bflow)
>>          log_debug("parsing bflow file size %x\n", bflow->size);
>>          membuff_init(&mb, bflow->buf, bflow->size);
>>          membuff_putraw(&mb, bflow->size, true, &data);
>> -       while (len = membuff_readline(&mb, line, sizeof(line) - 1, ' '), len) {
>> +       while (len = membuff_readline(&mb, line, sizeof(line) - 1, ' ', true), len) {
>>                  char *tok, *p = line;
>>
>>                  tok = strsep(&p, " ");
>> diff --git a/common/console.c b/common/console.c
>> index 8a869b137e..cd56035171 100644
>> --- a/common/console.c
>> +++ b/common/console.c
>> @@ -846,7 +846,7 @@ bool console_record_overflow(void)
>>   int console_record_readline(char *str, int maxlen)
>>   {
>>          return membuff_readline((struct membuff *)&gd->console_out, str,
>> -                               maxlen, '\0');
>> +                               maxlen, '\0', false);
>>   }
>>
>>   int console_record_avail(void)
>> diff --git a/include/membuff.h b/include/membuff.h
>> index 21051b0c54..4eba626ce1 100644
>> --- a/include/membuff.h
>> +++ b/include/membuff.h
>> @@ -192,10 +192,11 @@ int membuff_free(struct membuff *mb);
>>    * @mb: membuff to adjust
>>    * @str: Place to put the line
>>    * @maxlen: Maximum line length (excluding terminator)
>> + * @must_fit: If true then str is empty if line doesn't fit
>>    * Return: number of bytes read (including terminator) if a line has been
>> - *        read, 0 if nothing was there
>> + *        read, 0 if nothing was there or line didn't fit when must_fit is set
>>    */
>> -int membuff_readline(struct membuff *mb, char *str, int maxlen, int minch);
>> +int membuff_readline(struct membuff *mb, char *str, int maxlen, int minch, bool must_fit);
>>
>>   /**
>>    * membuff_extend_by() - expand a membuff
>> diff --git a/lib/membuff.c b/lib/membuff.c
>> index 36dc43a523..964e504d5b 100644
>> --- a/lib/membuff.c
>> +++ b/lib/membuff.c
>> @@ -288,7 +288,7 @@ int membuff_free(struct membuff *mb)
>>                          (mb->end - mb->start) - 1 - membuff_avail(mb);
>>   }
>>
>> -int membuff_readline(struct membuff *mb, char *str, int maxlen, int minch)
>> +int membuff_readline(struct membuff *mb, char *str, int maxlen, int minch, bool must_fit)
>
> Shouldn't that be the normal case? I might have this wrong, but this
> looks like a bug fix to me.

For the extlinux.conf case assuming that the file has ended if a line is
truncated seems to be the wrong thing to do.

Currently we return a value <= maxlen - 1 if no truncation has occurred
and 0 if a truncation occurred.

I would prefer if the function would return maxlen in the case of
truncation and the truncated string. This will allow us to detect
truncation without mistaking it for the end of the buffer.

Best regards

Heinrich

>>   {
>>          int len;  /* number of bytes read (!= string length) */
>>          char *s, *end;
>> @@ -310,7 +310,7 @@ int membuff_readline(struct membuff *mb, char *str, int maxlen, int minch)
>>          }
>>
>>          /* couldn't get the whole string */
>> -       if (!ok) {
>> +       if (!ok && must_fit) {
>>                  if (maxlen)
>>                          *orig = '\0';
>>                  return 0;
>> --
>> 2.40.1
>>