From: Laurent Fasnacht <fasnacht@protonmail.ch>
To: "netfilter-devel@vger.kernel.org" <netfilter-devel@vger.kernel.org>
Cc: "pablo@netfilter.org" <pablo@netfilter.org>
Subject: Re: [PATCH nft include v2 4/7] scanner: remove parser_state->indescs static array
Date: Tue, 11 Feb 2020 04:36:44 +0000 [thread overview]
Message-ID: <B0GhcdgsD8gKeSn6wk4mAmzGp6cAi-uxHSUVpr5yexgU0kmx4o3uoS_ci4H49xeBrf7F39IoG5OC-peD3kC6TlNtL7zpvbBzQZP7FYu5GRY=@protonmail.ch> (raw)
In-Reply-To: <20200210233256.bis2igtwje77vimm@salvia>
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?
next prev parent reply other threads:[~2020-02-11 4:36 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-02-10 10:17 [PATCH nft include v2 0/7] Improve include behaviour Laurent Fasnacht
2020-02-10 10:17 ` [PATCH nft include v2 1/7] tests: shell: add test for glob includes Laurent Fasnacht
2020-02-12 20:45 ` Pablo Neira Ayuso
2020-02-10 10:17 ` [PATCH nft include v2 2/7] scanner: move the file descriptor to be in the input_descriptor structure Laurent Fasnacht
2020-02-10 22:46 ` Pablo Neira Ayuso
2020-02-10 10:17 ` [PATCH nft include v2 3/7] scanner: move indesc list append in scanner_push_indesc Laurent Fasnacht
2020-02-10 22:46 ` Pablo Neira Ayuso
2020-02-10 10:17 ` [PATCH nft include v2 4/7] scanner: remove parser_state->indescs static array Laurent Fasnacht
2020-02-10 23:32 ` Pablo Neira Ayuso
2020-02-11 4:36 ` Laurent Fasnacht [this message]
2020-02-12 20:45 ` Pablo Neira Ayuso
2020-02-10 10:17 ` [PATCH nft include v2 5/7] scanner: correctly compute include depth Laurent Fasnacht
2020-02-10 10:17 ` [PATCH nft include v2 6/7] scanner: fix indesc_list stack to be in the correct order Laurent Fasnacht
2020-02-10 22:33 ` Pablo Neira Ayuso
2020-02-11 4:42 ` Laurent Fasnacht
2020-02-10 10:17 ` [PATCH nft include v2 7/7] scanner: remove parser_state->indesc_idx Laurent Fasnacht
2020-02-10 22:31 ` Pablo Neira Ayuso
2020-02-11 5:04 ` Laurent Fasnacht
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='B0GhcdgsD8gKeSn6wk4mAmzGp6cAi-uxHSUVpr5yexgU0kmx4o3uoS_ci4H49xeBrf7F39IoG5OC-peD3kC6TlNtL7zpvbBzQZP7FYu5GRY=@protonmail.ch' \
--to=fasnacht@protonmail.ch \
--cc=netfilter-devel@vger.kernel.org \
--cc=pablo@netfilter.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).