Re: [PATCH nft include v2 7/7] scanner: remove parser_state->indesc_idx

[Laurent Fasnacht <fasnacht@protonmail.ch>]

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Monday, February 10, 2020 11:31 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:

> On Mon, Feb 10, 2020 at 10:17:28AM +0000, Laurent Fasnacht wrote:
> > static void scanner_pop_indesc(struct parser_state *state)
> > {
> >
> > -   state->indesc_idx--;
> >     if (!list_empty(&state->indesc_list)) {
> >     state->indesc = list_entry(state->indesc->list.prev, struct input_descriptor, list);
> >     } else {
> >     @@ -968,10 +966,6 @@ void scanner_destroy(struct nft_ctx *nft)
> >     {
> >     struct parser_state *state = yyget_extra(nft->scanner);
> >
> > -   do {
> >
> > -       yypop_buffer_state(nft->scanner);
> >
> >
>
> nft_pop_buffer_state calls free().
>
> Are you sure this can be removed without incurring in memleaks?

I'm pretty sure it can, since scanner_destroy is only called from outside of the scanner itself (and I'm expecting calling that function during the parsing might break a lot things ;-).

In my understanding, any file that is being parsed should reach the end of file at some point, and therefore have scanner_pop_buffer called. This is true also in case of parsing errors.

I've tested that understanding by counting the number of yypush and yypop calls in various cases, and by adding an assert in scanner_destroy (which consist in asserting that the "active" part of the stack is empty):

assert(state->indesc->list.next == state->indesc_list.next);

This has succeeded both for the test suite and for the very complex rule set I'm working on currently.

Just as a comment about the stack: in implementation with patch 6/7 applied, it consists in two parts, the active part and the "dangling tail".  The start of the stack is in state->indesc_list, and the active element (in state->indesc) is the end of the active part. All the elements in the active part of the stack have buffer pushed. When parsing a file ends, the buffer is popped and state->indesc is moved (but the input descriptor itself is not removed from the list, so it stays in the tail part until scanner_destroy is called).