Re: [PATCH] dfa: don't overrun a malloc'd buffer for certain regexps

[Stefano Lattarini]

Hi Jim.  Sorry to jump in just to be nitpicky, but ...

On Friday 17 June 2011, Jim Meyering wrote:
> If you give your favorite grep program the wrong ERE,
> it may clobber the heap:
> 
>     $ grep -E '(^| )*(a|b)*(c|d)*( |$)' < /dev/null
>     [Exit 139 (SEGV)]
> 
> This bug affects gawk, too, since they use the same dfa.c:
> 
>     $gawk '{ /(^| )*(a|b)*(c|d)*( |$)/ }' < /dev/null
>     [Exit 139 (SEGV)]
> 
> 
> 
> From 0b91d6928e9d098d3746ce9f4bb4160a2e685f5c Mon Sep 17 00:00:00 2001
> From: Jim Meyering <address@hidden>
> Date: Fri, 17 Jun 2011 10:27:06 +0200
> Subject: [PATCH] dfa: don't overrun a malloc'd buffer for certain regexps
> 
> * src/dfa.c (dfaanalyze): Allocate space for twice as many
> positions as there are leaves.  Before this change, for some
> regular expressions, DFA analysis would have inserted far more
> "positions" than dfa->nleaves (up to double).
> Reported by Raymond Russell in http://savannah.gnu.org/bugs/?33547
> * tests/dfa-heap-overrun: Trigger the overrun.
> * tests/Makefile.am (TESTS): Add it.
> * NEWS (Bug fixes): Mention it.
> ---
>  NEWS                   |    3 +++
>  src/dfa.c              |    2 +-
>  tests/Makefile.am      |    4 ++--
>  tests/dfa-heap-overrun |   26 ++++++++++++++++++++++++++
>  4 files changed, 32 insertions(+), 3 deletions(-)
>  create mode 100755 tests/dfa-heap-overrun
> 
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index 8d51727..1f0d2cf 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -46,6 +46,7 @@ TESTS =                                             \
>    case-fold-char-range                               \
>    case-fold-char-type                                \
>    char-class-multibyte                               \
> +  dfa-heap-overrun                           \
>    dfaexec-multibyte                          \
>    empty                                              \
>    equiv-classes                                 \
> @@ -103,7 +104,6 @@ MALLOC_PERTURB_ = 1
>  TESTS_ENVIRONMENT =                          \
>    tmp__=$$TMPDIR; test -d "$$tmp__" || tmp__=.;      \
>    TMPDIR=$$tmp__; export TMPDIR;             \
> -  exec 9>&2;                                 \
>    shell_or_perl_() {                         \
>      if grep '^\#!/usr/bin/perl' "$$1" > /dev/null; then                      
> \
>        if $(PERL) -e 'use warnings' > /dev/null 2>&1; then            \
> @@ -141,6 +141,6 @@ TESTS_ENVIRONMENT =                               \
>    PERL='$(PERL)'                             \
>    SHELL='$(SHELL)'                           \
>    PATH='$(abs_top_builddir)/src$(PATH_SEPARATOR)'"$$PATH" \
> -  ; shell_or_perl_
> +  ; shell_or_perl_ 9>&2
>
... wouldn't it be better to do this fix to TESTS_ENVIRONMENT in
a follow-up patch?  It's not even mentioned in the ChangeLog.

Regards,
  Stefano