Re: [LEDE-DEV] Bug when processing long lines
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
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
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, &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@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
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, &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