| Message ID | 20200210101709.9182-5-fasnacht@protonmail.ch |
|---|---|
| State | Accepted |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | Improve include behaviour | expand |
On Mon, Feb 10, 2020 at 10:17:24AM +0000, Laurent Fasnacht wrote: > This static array is redundant wth the indesc_list structure, but > is less flexible. Skipping this patch and taking 5/7, I get a crash. Sorry, I'm trying to understand if there is an easy fix for this, without simplifying things. I mean, first fix things, then move on and improve everything. The removal of the array stack looks like mandatory dependency to fix this? Please, help me understand. Thanks.
On Tuesday, February 11, 2020 12:32 AM, Pablo Neira Ayuso <pablo@netfilter.org> wrote: > On Mon, Feb 10, 2020 at 10:17:24AM +0000, Laurent Fasnacht wrote: > > > This static array is redundant wth the indesc_list structure, but > > is less flexible. > > Skipping this patch and taking 5/7, I get a crash. Probably was out of bounds in state->indescs[MAX_INCLUDE_DEPTH]. > Sorry, I'm trying to understand if there is an easy fix for this, > without simplifying things. I mean, first fix things, then move on and > improve everything. In my opinion, there isn't, unfortunately. That's why it took me quite some time to fix. Let me explain: The data structure the original code is trying to achieve is a stack of input descriptors, where one entry in that stack is one level of include. This stack is implemented using the indesc_idx variable and the indescs and fd array. Unfortunately, there is an issue with that design for glob includes: glob includes add all the discovered files to that stack at once (in reverse alphabetic order so the parsing happens in alphabetic order). The bound check for the static array incorrectly leads to the error message that the maximum inclusion depth is reached. However, removing that check alone will result in out of bound access of both fd and indescs arrays) So basically, in order to apply patch 5/7, you need 2/7 and 4/7 (and 3/7, which is minor refactoring) to get rid the static arrays. As an example, the test supplied in patch 1/7 only has one level of inclusion, but adds all the files to the stack, so the static arrays will overflow. As a side note, the stack implementation is incorrect, both in the original code and after patch 5/7 is applied (since it's a minimal patch, as you previously ask). The stack implementation is fixed in patch 6/7. That specific patch is however unrelated to the include depth problem. I believe however that it's the correct fix of bug #1383, and should be applied in order to have a sane data structure. Does that help?
On Mon, Feb 10, 2020 at 10:17:24AM +0000, Laurent Fasnacht wrote: > This static array is redundant wth the indesc_list structure, but > is less flexible. Patches applied, from 3 to 7. Thanks.
diff --git a/include/parser.h b/include/parser.h index 949284d9..66db92d8 100644 --- a/include/parser.h +++ b/include/parser.h @@ -15,7 +15,6 @@ struct parser_state { struct input_descriptor *indesc; - struct input_descriptor *indescs[MAX_INCLUDE_DEPTH]; unsigned int indesc_idx; struct list_head indesc_list; diff --git a/src/scanner.l b/src/scanner.l index 37b01639..8397846b 100644 --- a/src/scanner.l +++ b/src/scanner.l @@ -668,19 +668,19 @@ addrstring ({macaddr}|{ip4addr}|{ip6addr}) static void scanner_push_indesc(struct parser_state *state, struct input_descriptor *indesc) { - state->indescs[state->indesc_idx] = indesc; - state->indesc = state->indescs[state->indesc_idx++]; list_add_tail(&indesc->list, &state->indesc_list); + state->indesc = indesc; + state->indesc_idx++; } static void scanner_pop_indesc(struct parser_state *state) { state->indesc_idx--; - - if (state->indesc_idx > 0) - state->indesc = state->indescs[state->indesc_idx - 1]; - else + if (!list_empty(&state->indesc_list)) { + state->indesc = list_entry(state->indesc->list.prev, struct input_descriptor, list); + } else { state->indesc = NULL; + } } static void scanner_pop_buffer(yyscan_t scanner)
This static array is redundant wth the indesc_list structure, but is less flexible. Signed-off-by: Laurent Fasnacht <fasnacht@protonmail.ch> --- include/parser.h | 1 - src/scanner.l | 12 ++++++------ 2 files changed, 6 insertions(+), 7 deletions(-)