[Mesa-dev] [PATCH] glcpp: Don't use alternation in the lookahead for empty pragmas.

[Kenneth Graunke]

On Monday, August 18, 2014 11:49:13 AM Carl Worth wrote:
> We've found that there's a buffer overrun bug in flex that's triggered by
> using alternation in a lookahead pattern.
> 
> Fortunately, we don't need to match the exact {NEWLINE} expression to detect
> an empty pragma. It suffices to verify that there are no non-space characters
> before any newline character. So we can use a simple [\r\n] to get the desired
> behavior while avoiding the flex bug.
> 
> Fixes Piglit's 16385-consecutive-chars and
> 17000-consecutive-chars-identifier tests.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=82472
> Signed-off-by: Carl Worth <cworth at cworth.org>
> Approach-suggested-by: Kenneth Graunke <kenneth at whitecape.org>

You probably don't need this tag :)

> CC: Kenneth Graunke <kenneth at whitecape.org>
> ---
> 
>  Thanks for chasing down the fix for this regression of mine, Ken.
> 
>  I am embarrassed that I clearly didn't run piglit enough while testing my
>  original branch.
> 
>  With your fix above, there is some state that's not updated as it should
>  be when returning a NEWLINE token, (such as incrementing yylineno, etc.).
>  I tried to improve things to update all that state, but it proved
>  problematic, (putting the state updates in a common function doesn't
>  work because only the outer lexing function has access to local variables
>  like yylineno).

Thanks for catching this.

> 
>  The alternate approach here was your recommendation, of course.
> 
>  src/glsl/glcpp/glcpp-lex.l | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/src/glsl/glcpp/glcpp-lex.l b/src/glsl/glcpp/glcpp-lex.l
> index 98d500e..aaef7b8 100644
> --- a/src/glsl/glcpp/glcpp-lex.l
> +++ b/src/glsl/glcpp/glcpp-lex.l
> @@ -289,8 +289,14 @@ HEXADECIMAL_INTEGER	0[xX][0-9a-fA-F]+[uU]?
>  }
>  
>  	/* Swallow empty #pragma directives, (to avoid confusing the
> -	 * downstream compiler). */
> -<HASH>pragma{HSPACE}*/{NEWLINE} {
> +	 * downstream compiler).
> +	 *
> +	 * Note: We use a simple regular expression for the lookahead
> +	 * here. Specifically, we cannot use the complete {NEWLINE} expression
> +	 * since it uses alternation and we've found that there's a flex bug
> +	 * where using alternation in the lookahead portion of a pattern
> +	 * triggers a buffer overrun.  /

Erm.  Shouldn't there be a star here? ^^^

I can't imagine this compiles...or worse, it does, and continues the comment to the */ in the rule below...

> +<HASH>pragma{HSPACE}*/[\r\n] {
>  	BEGIN INITIAL;
>  }

With that fixed, and piglit tested, this would get my Reviewed-by.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140818/3b996e07/attachment.sig>