[LEDE-DEV] Bug when processing long lines

[Jakub Horák]

Hello,

On 01/11/2018 08:26 PM, Alexandru Ardelean wrote:
>> I don't post patches to mailing lists very often, so I apologize if I'm
>> sending this in a wrong format or in a too broken english.
> 
> Hey,
> 
> Patches usually are sent with git send-mail.
> So  "git send-mail
> 0001-procd-Fix-behavior-when-parsing-long-lines.patch
> --to=openwrt-devel at lists.openwrt.org"
> [ FWIW: LEDE has merged back to OpenWrt :) ]

So... should I resend it there?

> Now about the patch.
> 
> -       str = ustream_get_read_buf(s, NULL);
> +       str = ustream_get_read_buf(s, &buflen);
>         if (!str)
>             break;
> 
> -       newline = strchr(str, '\n');
> -       if (!newline)
> -           break;
> -
> -       *newline = 0;
> +       /* search for '\n', take into account NUL bytes */
> +       newline = memchr(str, '\n', buflen);
> +       if (!newline) {
> +           /* is there a room in buffer? */
> +           if (buflen < s->r.buffer_len) {
> +               /* yes - bailout, newline may still come */
> +               break;
> +           } else {
> +               /* no - force newline */
> +               len = buflen;
> 
> It's weird that this would happen here, since there should be a
> ustream_reserve() call that would guarantee that there is sufficient
> buffer size.
> I could be wrong, or it could be a bug somewhere; who knows ?

The buffer might be full at this point and that's OK - we are checking
just after we read data into it. However if the buffer is full and it
doesn't contain a newline - then it will never contain a new-line -
because its full.
> 
> In any case, if this is a correct approach, I think you should also
> add  *(str + len) = 0  ; to make sure the string is null-terminated.

The string is guaranteed to be null terminated, see the comment six
lines bellow:

> +           }
> +       } else {
> +           *newline = 0;
> +           len = newline + 1 - str;
> +       }
> +       /* "str" is NUL terminated by ustream_get_read_buf */
>         ulog(prio, "%s\n", str);
> -
> -       len = newline + 1 - str;
>         ustream_consume(s, len);
> 
> 
> Alex

Best regards,
Jakub


> 
>>
>> Best regards,
>> Jakub Horak
>>
>> _______________________________________________
>> Lede-dev mailing list
>> Lede-dev at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/lede-dev
>>