On Thu, Jan 11, 2018 at 6:28 PM, Jakub Horák <jakub.ho...@braiins.cz> 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, &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 ?
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
