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

[Simon Glass]

Hi,

On Tue, 21 Nov 2023 at 21:16, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> 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.

OK I have taken a harder look at this now.

Wouldn't that indicate that the string was exactly the maximum length?

Also, what happens to the chars after that on the same line?

What is actually going wrong that is leading to the need for this
patch? How can it be correct to parse just some of the line and leave
the rest for later?

I think this needs a harder look, but I don't have enough info to help here.

>
> 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
> >>
>

Regards,
Simon