GNU bug report logs - #15307
minor fix to dfa.c

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 15307 in the body.
You can then email your comments to 15307 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Aharon Robbins <arnold <at> skeeve.com>
To: bug-grep <at> gnu.org
Subject: minor fix to dfa.c
Date: Sun, 08 Sep 2013 11:53:07 +0200

The following fix to dfa.c was suggested by a static checking tool.
I'm applying it in the gawk code base.

Basically, it's theoretically possible for len to have run off the end
of the `str' array.

Thanks,

Arnold

diff --git a/dfa.c b/dfa.c
index 8b79eb7..490a075 100644
--- a/dfa.c
+++ b/dfa.c
@@ -1038,7 +1038,8 @@ parse_bracket_exp (void)
                     /* This is in any case an invalid class name.  */
                     str[0] = '\0';
                 }
-              str[len] = '\0';
+              if (len < BRACKET_BUFFER_SIZE)
+                 str[len] = '\0';
 
               /* Fetch bracket.  */
               FETCH_WC (c, wc, _("unbalanced ["));

Message #8 received at 15307 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Aharon Robbins <arnold <at> skeeve.com>
Cc: 15307 <at> debbugs.gnu.org
Subject: Re: bug#15307: minor fix to dfa.c
Date: Sun, 8 Sep 2013 11:01:08 -0700

On Sun, Sep 8, 2013 at 2:53 AM, Aharon Robbins <arnold <at> skeeve.com> wrote:
> The following fix to dfa.c was suggested by a static checking tool.
> I'm applying it in the gawk code base.
>
> Basically, it's theoretically possible for len to have run off the end
> of the `str' array.
...
> diff --git a/dfa.c b/dfa.c
> index 8b79eb7..490a075 100644
> --- a/dfa.c
> +++ b/dfa.c
> @@ -1038,7 +1038,8 @@ parse_bracket_exp (void)
>                      /* This is in any case an invalid class name.  */
>                      str[0] = '\0';
>                  }
> -              str[len] = '\0';
> +              if (len < BRACKET_BUFFER_SIZE)
> +                 str[len] = '\0';
>
>                /* Fetch bracket.  */
>                FETCH_WC (c, wc, _("unbalanced ["));

Hi Arnold,

Thanks, but that makes it look like "str" will instead fail to be
NUL-terminated,
in which case the following strcmp (aka STREQ) would overrun the buffer.
Yes, this is all theoretical, but still...

I see that the current limit is 31:

  $ for i in 30 31 32 33; do printf "$i "; src/grep -E '[[:'$(perl -e
'print "a"x'$i)':]]'; done
  30 src/grep: Invalid character class name
  31 src/grep: Invalid character class name
  32 src/grep: Unmatched [ or [^
  33 src/grep: Unmatched [ or [^

So I propose this patch instead:

From f1e1fb2c5c1538c313f8488ef687b9a96684f54e Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering <at> fb.com>
Date: Sun, 8 Sep 2013 10:49:52 -0700
Subject: [PATCH] dfa: appease a static analyzer, and save 95 stack bytes

* src/dfa.c (MAX_BRACKET_STRING_LEN): Rename from BRACKET_BUFFER_SIZE
and decrease from 128 to 32.
(parse_bracket_exp): Add one byte more than MAX_BRACKET_STRING_LEN
to the length of "str" buffer, to avoid appearance that we may store
the trailing NUL beyond the end of buffer.  A string of length 32
or greater is rejected by earlier processing, so would never reach
this code.  Addresses http://bugs.gnu.org/15307
---
 src/dfa.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/dfa.c b/src/dfa.c
index ad38d3b..b447a8a 100644
--- a/src/dfa.c
+++ b/src/dfa.c
@@ -975,8 +975,8 @@ parse_bracket_exp (void)
          dfa is ever called.  */
       if (c == '[' && (syntax_bits & RE_CHAR_CLASSES))
         {
-#define BRACKET_BUFFER_SIZE 128
-          char str[BRACKET_BUFFER_SIZE];
+#define MAX_BRACKET_STRING_LEN 32
+          char str[MAX_BRACKET_STRING_LEN + 1];
           FETCH_WC (c1, wc1, _("unbalanced ["));

           /* If pattern contains '[[:', '[[.', or '[[='.  */
@@ -990,7 +990,7 @@ parse_bracket_exp (void)
                   FETCH_WC (c, wc, _("unbalanced ["));
                   if ((c == c1 && *lexptr == ']') || lexleft == 0)
                     break;
-                  if (len < BRACKET_BUFFER_SIZE)
+                  if (len < MAX_BRACKET_STRING_LEN)
                     str[len++] = c;
                   else
                     /* This is in any case an invalid class name.  */
-- 
1.8.4.99.gd2dbd39

Message #11 received at 15307 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Aharon Robbins <arnold <at> skeeve.com>
Cc: 15307 <at> debbugs.gnu.org
Subject: Re: bug#15307: minor fix to dfa.c
Date: Wed, 11 Sep 2013 08:36:52 -0700

On Sun, Sep 8, 2013 at 11:01 AM, Jim Meyering <jim <at> meyering.net> wrote:
> On Sun, Sep 8, 2013 at 2:53 AM, Aharon Robbins <arnold <at> skeeve.com> wrote:
>> The following fix to dfa.c was suggested by a static checking tool.
>> I'm applying it in the gawk code base.
>>
>> Basically, it's theoretically possible for len to have run off the end
>> of the `str' array.
>>...
>
> Hi Arnold,
>
> Thanks, but that makes it look like "str" will instead fail to be
> NUL-terminated,
> in which case the following strcmp (aka STREQ) would overrun the buffer.
> Yes, this is all theoretical, but still...
>
> I see that the current limit is 31:
>
>   $ for i in 30 31 32 33; do printf "$i "; src/grep -E '[[:'$(perl -e
> 'print "a"x'$i)':]]'; done
>   30 src/grep: Invalid character class name
>   31 src/grep: Invalid character class name
>   32 src/grep: Unmatched [ or [^
>   33 src/grep: Unmatched [ or [^
>
> So I propose this patch instead:

Hi Arnold,

I was going to push that change, but then realized I didn't know
which static analysis tool you were referring to.  Which was it?

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.