[DTrace-devel] [PATCH v2 03/23] lexer: work around bug in flex <= 2.6.0

[Kris Van Hees]

On Mon, Dec 11, 2023 at 08:34:08PM +0000, Nick Alcock wrote:
> On 6 Dec 2023, Kris Van Hees said:
> > On Mon, Nov 27, 2023 at 04:47:09PM +0000, Nick Alcock wrote:
> >> Flex commit f863c949, released in 2.6.1, is a one-line fix to the
> >> generated flex skeleton that fixes an old bug that breaks yywrap();
> >> if it returns nonzero, input() is meant to return 0 to its caller
> >> on EOF. This bug makes it return EOF instead, which no lexer expects
> >> (and certainly the DTrace lexer does not).
> >> 
> >> Alas we cannot get around this bug just by defining %option noyywrap,
> >> since that still emits the same buggy piece of the skeleton but just
> >> introduces its own yywrap() which returns 1. And there's no way to
> >> fix the skeleton from flex itself, and we can't require flex >= 2.6.1
> >> since OL7 has 2.5.37.
> >> 
> >> So, for now, resort to a one-line patch to the generated lexer, silently
> >> applied and skipped if it doesn't apply.  (Any generated lexer it doesn't
> >> apply on will have this bug fixed.)
> >
> > Repeating my review comments from the previous posting of this patch:
> >
> > # I think it might be better to perhaps do this using a sed command since there
> > # is only a single 'return EOF' present in the generated dt_lex.c anyway.  So a
> > # simple s/return EOF;/return 0;/ would suffice.  And it is a no-op if no
> > # return EOF is found.
> 
> That seems terribly fragile to me -- what if a later version of flex
> introduces another 'return EOF;'? What if we use that extremely
> inoffensive and commonplace line ourselves?
> 
> The reason I used patch was that patch has patch context, which makes
> this much less likely to misapply. Patching source with sed has been
> considered obsolete for decades for a reason.

But isn't the way you do the patching going to silently ignore patch failures?
So if the patching fails for any other reason than the flex version being new
enough to not need the patch, we won't know.  That is not good either.

Perhaps we should consider including pre-generated flex output that is used
when flex is too old?  Other projects have done that and it avoids fragile
patching one way or another.

> > Doing so makes this patch smaller and avoids extra build dependencies.
> 
> You'll pardon me for not worrying about requiring *patch*. It's tiny and
> ubiquitously available. I worry about as much as I would about requiring
> cat.
> 
> > Alternatively, you could require flex >= 2.6.1 *except* on OL7.  Then the
> > patch becomes a simple change to dtrace.spec, right?  That would be even more
> > desirable.  I can live with the odd (buggy) error reporting behaviour this
> > causes on older flex versions if wew ccan reasonably assume that most people
> > will have a recent enough flex anyway.
> 
> That seems reasonable enough to me though, and much less ugly -- except
> that this bug causes buffer overruns (it's the fundamental reason for
> the err.pspec-default.d failure). This *is* a program running as root...
> but if we don't care, I'm happy to just require a recent flex!