Re: [LEDE-DEV] Bug when processing long lines

[Jakub Horák]

On 02/13/2018 05:01 PM, John Crispin wrote:
>> I found a bug in procd that gets triggered when long lines are printed
>> by services whose stdout/stderr are being logged. The bug itself is
>> explained in the attached patch.
[SNIP]
> 
> Hi Jakub,
> 
> i've just posted and alternative solution. could you help test it please ?
> 
>     John

Hello John,
I currently do not have time to test it. But I've attached script to
trigger this bug in my previous e-mail, so I should be fairly easy to
run the test.

Best regards,
Jakub Horak

___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

Re: [LEDE-DEV] Bug when processing long lines

[John Crispin]

On 11/01/18 17:28, Jakub Horák wrote:

Hello LEDE developers,

I found a bug in procd that gets triggered when long lines are printed
by services whose stdout/stderr are being logged. The bug itself is
explained in the attached patch.

However, when I was testing the fix, I found out that the bug is present
in other places, mostly those that call "ustream_get_read_buf" function.
Some examples:

- ubox/log/logread.c:logread_fb_data_cb() - when buffer passed on the
descriptor is larger than 4096, reception stops
- netifd/main.c:netifd_process_log_read_cb - this is a place that seems
to have this bug fixed, but still has incorrect handling of NUL bytes
- libubox/examples/ustream-example.c:client_read_cb - this is probably
the place that originated this broken bit of code
- uhttpd/relay.c:relay_process_headers - another place that seems broken

I've attached an init script (that goes to /etc/init.d/flood) and three
Lua programs (flood[123].lua) that trigger this behavior:
- flood1.lua writes long message to stdout, that triggers this behavior
in procd
- flood2.lua writes message that gets correctly processed by procd, but
triggers the bug in logread
- flood3.lua writes message with embedded zeros

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.

Best regards,
Jakub Horak


Hi Jakub,

i've just posted and alternative solution. could you help test it please ?

    John





___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev



___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

Re: [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-de...@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, );
> 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@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/lede-dev
>>

___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

Re: [LEDE-DEV] Bug when processing long lines

[Alexandru Ardelean]

On Thu, Jan 11, 2018 at 6:28 PM, Jakub Horák  wrote:
> Hello LEDE developers,
>
> I found a bug in procd that gets triggered when long lines are printed
> by services whose stdout/stderr are being logged. The bug itself is
> explained in the attached patch.
>
> However, when I was testing the fix, I found out that the bug is present
> in other places, mostly those that call "ustream_get_read_buf" function.
> Some examples:
>
> - ubox/log/logread.c:logread_fb_data_cb() - when buffer passed on the
> descriptor is larger than 4096, reception stops
> - netifd/main.c:netifd_process_log_read_cb - this is a place that seems
> to have this bug fixed, but still has incorrect handling of NUL bytes
> - libubox/examples/ustream-example.c:client_read_cb - this is probably
> the place that originated this broken bit of code
> - uhttpd/relay.c:relay_process_headers - another place that seems broken
>
> I've attached an init script (that goes to /etc/init.d/flood) and three
> Lua programs (flood[123].lua) that trigger this behavior:
> - flood1.lua writes long message to stdout, that triggers this behavior
> in procd
> - flood2.lua writes message that gets correctly processed by procd, but
> triggers the bug in logread
> - flood3.lua writes message with embedded zeros
>
> 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-de...@lists.openwrt.org"
[ FWIW: LEDE has merged back to OpenWrt :) ]

Now about the patch.

-   str = ustream_get_read_buf(s, NULL);
+   str = ustream_get_read_buf(s, );
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 ?

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.

+   }
+   } 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 Horak
>
> ___
> Lede-dev mailing list
> Lede-dev@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/lede-dev
>

___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev