| Message ID | 20210617194326.2107129-1-cjense@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 5fe6b476c7298b525f5b30ef192dd78578ac6e40 |
| Headers | show |
| Series | [1/2] libtraceevent: Add eof checks. | expand |
On Thu, 17 Jun 2021 19:43:25 +0000 Claire Jensen <cjense@google.com> wrote: Hi Claire, Thanks for sending the patches, I'll try to get some time to look at them (note, that I have a lot of other duties that I need to finish before I can get to this). > Added checking for __read_char and peek_char to make sure value is not at end > of file. > > This issue was found while fuzz testing. One of the test cases created an infinite loop because __read_token had reached end of file. Checking was added to all cases where this may occur. You don't need to fix this now, but for future reference, we follow the Linux guidelines on submitting patches which includes having line breaks at 74 (although I use 76) bytes, for the change log. This makes the change logs easier to read. Thanks! -- Steve > > Signed-off-by: Claire Jensen <cjense@google.com> >
On Thu, 17 Jun 2021 15:58:28 -0400 Steven Rostedt <rostedt@goodmis.org> wrote: > On Thu, 17 Jun 2021 19:43:25 +0000 > Claire Jensen <cjense@google.com> wrote: > > Hi Claire, > > Thanks for sending the patches, I'll try to get some time to look at them > (note, that I have a lot of other duties that I need to finish before I can > get to this). > > > Added checking for __read_char and peek_char to make sure value is not at end > > of file. > > > > This issue was found while fuzz testing. One of the test cases created an infinite loop because __read_token had reached end of file. Checking was added to all cases where this may occur. > > You don't need to fix this now, but for future reference, we follow the > Linux guidelines on submitting patches which includes having line breaks at > 74 (although I use 76) bytes, for the change log. > > This makes the change logs easier to read. > I made the mistake of adding this patch and pushing it to a new release without running my test suite against it. It ended up breaking the parsing. When running with --debug -N, I get: [ftrace:branch] unexpected type 1 [sched:sched_switch] unknown op '' [irq:irq_handler_exit] unexpected type 1 [timer:timer_start] unknown op '' [kvm:vcpu_match_mmio] unexpected type 1 [kvm:kvm_wait_lapic_expire] unknown op '' [kvm:kvm_vcpu_wakeup] unexpected type 1 [kvm:kvm_userspace_exit] unknown op '' [kvm:kvm_pv_tlb_flush] unexpected type 1 [kvm:kvm_ple_window_update] unknown op '' [kvm:kvm_pio] unknown op '' [kvm:kvm_pic_set_irq] unknown op '' [kvm:kvm_nested_vmrun] unexpected type 1 [kvm:kvm_nested_vmexit_inject] unknown op '' Error: expected type 5 but read 0 [kvm:kvm_nested_vmexit] unknown op '' Error: expected type 5 but read 0 [kvm:kvm_nested_vmenter_failed] bad op token [kvm:kvm_msr] unexpected type 1 [kvm:kvm_msi_set_irq] unknown op '' unknown op '' [kvm:kvm_ioapic_set_irq] unknown op '' Error: expected type 5 but read 0 unknown op '' [kvm:kvm_ioapic_delayed_eoi_inj] unknown op '' Error: expected type 5 but read 0 [kvm:kvm_exit] unknown op '' Error: expected type 5 but read 0 [kvm:kvm_emulate_insn] unknown op '' Error: expected type 5 but read 0 [kvm:kvm_cpuid] unexpected type 1 unknown op '' [kvm:kvm_apic_ipi] unknown op '' Error: expected type 5 but read 0 unknown op '' [kvm:kvm_apic_accept_irq] unknown op '' Error: expected type 5 but read 0 [kvm:kvm_age_page] unexpected type 1 with the patch, and no errors without it. I have to revert this patch and push a new version out. -- Steve
On Wed, Jun 23, 2021 at 6:06 PM Steven Rostedt <rostedt@goodmis.org> wrote: > > On Thu, 17 Jun 2021 15:58:28 -0400 > Steven Rostedt <rostedt@goodmis.org> wrote: > > > On Thu, 17 Jun 2021 19:43:25 +0000 > > Claire Jensen <cjense@google.com> wrote: > > > > Hi Claire, > > > > Thanks for sending the patches, I'll try to get some time to look at them > > (note, that I have a lot of other duties that I need to finish before I can > > get to this). > > > > > Added checking for __read_char and peek_char to make sure value is not at end > > > of file. > > > > > > This issue was found while fuzz testing. One of the test cases created an infinite loop because __read_token had reached end of file. Checking was added to all cases where this may occur. > > > > You don't need to fix this now, but for future reference, we follow the > > Linux guidelines on submitting patches which includes having line breaks at > > 74 (although I use 76) bytes, for the change log. > > > > This makes the change logs easier to read. > > > > I made the mistake of adding this patch and pushing it to a new release > without running my test suite against it. It ended up breaking the parsing. > > When running with --debug -N, I get: Hi Steve, sorry for the breakage, could you give a full reproduction command? Thanks! Ian > [ftrace:branch] unexpected type 1 > [sched:sched_switch] unknown op '' > [irq:irq_handler_exit] unexpected type 1 > [timer:timer_start] unknown op '' > [kvm:vcpu_match_mmio] unexpected type 1 > [kvm:kvm_wait_lapic_expire] unknown op '' > [kvm:kvm_vcpu_wakeup] unexpected type 1 > [kvm:kvm_userspace_exit] unknown op '' > [kvm:kvm_pv_tlb_flush] unexpected type 1 > [kvm:kvm_ple_window_update] unknown op '' > [kvm:kvm_pio] unknown op '' > [kvm:kvm_pic_set_irq] unknown op '' > [kvm:kvm_nested_vmrun] unexpected type 1 > [kvm:kvm_nested_vmexit_inject] unknown op '' > Error: expected type 5 but read 0 > [kvm:kvm_nested_vmexit] unknown op '' > Error: expected type 5 but read 0 > [kvm:kvm_nested_vmenter_failed] bad op token > [kvm:kvm_msr] unexpected type 1 > [kvm:kvm_msi_set_irq] unknown op '' > unknown op '' > [kvm:kvm_ioapic_set_irq] unknown op '' > Error: expected type 5 but read 0 > unknown op '' > [kvm:kvm_ioapic_delayed_eoi_inj] unknown op '' > Error: expected type 5 but read 0 > [kvm:kvm_exit] unknown op '' > Error: expected type 5 but read 0 > [kvm:kvm_emulate_insn] unknown op '' > Error: expected type 5 but read 0 > [kvm:kvm_cpuid] unexpected type 1 > unknown op '' > [kvm:kvm_apic_ipi] unknown op '' > Error: expected type 5 but read 0 > unknown op '' > [kvm:kvm_apic_accept_irq] unknown op '' > Error: expected type 5 but read 0 > [kvm:kvm_age_page] unexpected type 1 > > with the patch, and no errors without it. > > I have to revert this patch and push a new version out. > > -- Steve
On Wed, 23 Jun 2021 22:50:58 -0700 Ian Rogers <irogers@google.com> wrote: > Hi Steve, > > sorry for the breakage, could you give a full reproduction command? You can download this: http://rostedt.org/private/trace-issue.dat And run trace-cmd report --debug on it, and you'll see that it fails to parse with the patch, but works fine without it. -- Steve
diff --git a/src/event-parse.c b/src/event-parse.c index 97c1a97..f454e23 100644 --- a/src/event-parse.c +++ b/src/event-parse.c @@ -1155,17 +1155,16 @@ static enum tep_event_type force_token(const char *str, char **tok); static enum tep_event_type __read_token(char **tok) { char buf[BUFSIZ]; - int ch, last_ch, quote_ch, next_ch; + int ch, last_ch, quote_ch, next_ch, read_ch, peek_ch; int i = 0; int tok_size = 0; enum tep_event_type type; *tok = NULL; - - ch = __read_char(); + ch = __read_char(); if (ch < 0) - return TEP_EVENT_NONE; + goto out_eof_error; type = get_type(ch); if (type == TEP_EVENT_NONE) @@ -1184,9 +1183,15 @@ static enum tep_event_type __read_token(char **tok) case TEP_EVENT_OP: switch (ch) { case '-': - next_ch = peek_char(); + peek_ch = peek_char(); + if (peek_ch < 0) + goto out_eof_error; + next_ch = peek_ch; if (next_ch == '>') { - buf[i++] = __read_char(); + read_ch = __read_char(); + if (read_ch < 0) + goto out_eof_error; + buf[i++] = read_ch; break; } /* fall through */ @@ -1197,9 +1202,14 @@ static enum tep_event_type __read_token(char **tok) case '<': last_ch = ch; ch = peek_char(); + if (ch < 0) + goto out_eof_error; if (ch != last_ch) goto test_equal; - buf[i++] = __read_char(); + read_ch = __read_char(); + if (read_ch < 0) + goto out_eof_error; + buf[i++] = read_ch; switch (last_ch) { case '>': case '<': @@ -1219,10 +1229,17 @@ static enum tep_event_type __read_token(char **tok) return type; test_equal: - ch = peek_char(); - if (ch == '=') - buf[i++] = __read_char(); - goto out; + peek_ch = peek_char(); + if (peek_ch < 0) + goto out_eof_error; + ch = peek_ch; + if (ch == '=') { + read_ch = __read_char(); + if (read_ch < 0) + goto out_eof_error; + buf[i++] = read_ch; + goto out; + } case TEP_EVENT_DQUOTE: case TEP_EVENT_SQUOTE: @@ -1242,6 +1259,8 @@ static enum tep_event_type __read_token(char **tok) } last_ch = ch; ch = __read_char(); + if(ch < 0) + goto out_eof_error; buf[i++] = ch; /* the '\' '\' will cancel itself */ if (ch == '\\' && last_ch == '\\') @@ -1259,6 +1278,8 @@ static enum tep_event_type __read_token(char **tok) do { ch = __read_char(); + if(ch < 0) + return TEP_EVENT_NONE; } while (isspace(ch)); if (ch == '"') goto concat; @@ -1273,7 +1294,13 @@ static enum tep_event_type __read_token(char **tok) break; } - while (get_type(peek_char()) == type) { + while (1) { + peek_ch = peek_char(); + if (peek_ch < 0) + goto out_eof_error; + if (get_type(peek_ch) != type) + break; + if (i == (BUFSIZ - 1)) { buf[i] = 0; tok_size += BUFSIZ; @@ -1282,8 +1309,10 @@ static enum tep_event_type __read_token(char **tok) return TEP_EVENT_NONE; i = 0; } - ch = __read_char(); - buf[i++] = ch; + read_ch = __read_char(); + if (read_ch < 0) + goto out_eof_error; + buf[i++] = read_ch; } out: @@ -1316,6 +1345,11 @@ static enum tep_event_type __read_token(char **tok) } return type; + +out_eof_error: + free(*tok); + *tok = NULL; + return TEP_EVENT_NONE; } static enum tep_event_type force_token(const char *str, char **tok)
Added checking for __read_char and peek_char to make sure value is not at end of file. This issue was found while fuzz testing. One of the test cases created an infinite loop because __read_token had reached end of file. Checking was added to all cases where this may occur. Signed-off-by: Claire Jensen <cjense@google.com> --- src/event-parse.c | 62 ++++++++++++++++++++++++++++++++++++----------- 1 file changed, 48 insertions(+), 14 deletions(-)