Re: some callers of symbol_list_n_get might get NULL and crash?

[Akim Demaille]

Hi Mike,

> Le 30 déc. 2013 à 22:01, Mike Sullivan <address@hidden> a écrit :
> 
> I'm looking at updating the bison in Solaris and one of the requirements
> is to run an internal tool (parfait) on it to check for problems. It seems
> to have found that while symbol_list_n_get() can return NULL, there are
> a few callers that just dereference the return value and don't check for that.
> 
> Here's the output from a run on 2.7.1, but it looks like 3.0.2 hasn't changed
> in that respect:
> 
> Error: Null pointer dereference (CWE 476)
>   Write to null pointer symbol_list_n_get(effective_rule, n)
>        at line 798 of 
> components/bison/build/amd64/src/../../../src/scan-code.l in function 
> 'handle_action_dollar'.
>          Function symbol_list_n_get may return constant 'NULL' at line 193, 
> called at line 798.
>          Null pointer introduced at line 193 of 
> components/bison/build/amd64/src/symlist.c in function 'symbol_list_n_get'.
> Error: Null pointer dereference (CWE 476)
>   Read from null pointer symbol_list_n_get(...)
>        at line 268 of components/bison/build/amd64/src/reader.c in function 
> 'symbol_should_be_used'.
>          Function symbol_list_n_get may return constant 'NULL' at line 193, 
> called at line 268.
>          Null pointer introduced at line 193 of 
> components/bison/build/amd64/src/symlist.c in function 'symbol_list_n_get'.
> Error: Null pointer dereference (CWE 476)
>   Read from null pointer symbol_list_n_get(...)
>        at line 536 of components/bison/build/amd64/src/reader.c in function 
> 'packgram'.
>          Function symbol_list_n_get may return constant 'NULL' at line 193, 
> called at line 536.
>          Null pointer introduced at line 193 of 
> components/bison/build/amd64/src/symlist.c in function 'symbol_list_n_get'.
> 
> from a quick look it does appear to be correct, but I don't know if at the
> times those calls occur that a NULL would ever really be returned. So they
> could be false positives, but it seems worth reporting just in case :)

Thanks for the report.  Well, it's good if that's the only
things the tool sees, since I guess it means it also validated
our grammar parser which is written in Bison, so in a way, it
also validated (some of) our generated parsers.

I'm about to install the following patch to address this issue.
Thanks a lot.

commit 401e8068e95da9c34deb92b22649cd00a7d507e8
Author: Akim Demaille <address@hidden>
Date:   Fri Jan 9 14:21:09 2015 +0100

    bison: use warnings from static code analysis
    
    A static analysis tool reports that some callers of symbol_list_n_get
    might get NULL and not handle it properly.  This is not the case, yet
    we can suppress this pattern.
    
    Reported by Mike Sullivan.
    <https://lists.gnu.org/archive/html/bug-bison/2013-12/msg00027.html>
    
    * src/symlist.c (symbol_list_n_get): Rename as...
    (symbol_list_n_get0): this.
    Adjust dependencies.
    (symbol_list_n_get): New, cannot return 0.
    * src/symlist.h: Adjust documentation.
    * src/scan-code.l: Style change.

diff --git a/THANKS b/THANKS
index d5feb16..4fc9984 100644
--- a/THANKS
+++ b/THANKS
@@ -91,6 +91,7 @@ Michel d'Hooge            address@hidden
 Michiel De Wilde          address@hidden
 Mickael Labau             address@hidden
 Mike Castle               address@hidden
+Mike Sullivan             address@hidden
 Neil Booth                address@hidden
 Nelson H. F. Beebe        address@hidden
 Nick Bowler               address@hidden
diff --git a/src/scan-code.l b/src/scan-code.l
index f90916c..308d1d0 100644
--- a/src/scan-code.l
+++ b/src/scan-code.l
@@ -711,7 +711,7 @@ handle_action_dollar (symbol_list *rule, char *text, 
location dollar_loc)
                       "]b4_rhs_value(%d, %d, ", effective_rule_length, n);
       obstack_quote (&obstack_for_string, type_name);
       obstack_sgrow (&obstack_for_string, ")[");
-      if (n > 0)
+      if (0 < n)
         symbol_list_n_get (effective_rule, n)->action_props.is_value_used =
           true;
       break;
diff --git a/src/symlist.c b/src/symlist.c
index b58cf5c..89ae29a 100644
--- a/src/symlist.c
+++ b/src/symlist.c
@@ -170,8 +170,10 @@ symbol_list_length (symbol_list const *l)
 | Get item N in symbol list L.  |
 `------------------------------*/
 
-symbol_list *
-symbol_list_n_get (symbol_list *l, int n)
+/* Same as symbol_list_n_get, but returns 0 on invalid calls. */
+
+static symbol_list *
+symbol_list_n_get0 (symbol_list *l, int n)
 {
   int i;
 
@@ -189,6 +191,14 @@ symbol_list_n_get (symbol_list *l, int n)
   return l;
 }
 
+symbol_list *
+symbol_list_n_get (symbol_list *l, int n)
+{
+  l = symbol_list_n_get0 (l, n);
+  aver (l);
+  return l;
+}
+
 
 /*--------------------------------------------------------------.
 | Get the data type (alternative in the union) of the value for |
diff --git a/src/symlist.h b/src/symlist.h
index 639b593..b63fc51 100644
--- a/src/symlist.h
+++ b/src/symlist.h
@@ -116,7 +116,8 @@ void symbol_list_free (symbol_list *list);
 /** Return the length of \c l. */
 int symbol_list_length (symbol_list const *l);
 
-/** Get item \c n in symbol list \c l.  */
+/** Get item \c n in symbol list \c l.
+ ** Never returns NULL, aborts if invalid call.  */
 symbol_list *symbol_list_n_get (symbol_list *l, int n);
 
 /* Get the data type (alternative in the union) of the value for