On Tuesday, February 11, 2020 12:32 AM, Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> 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?
