bug#11467: Parfait problems with GNU coreutils

[Jim Meyering]

Jim Meyering wrote:

> Rich Burridge wrote:
> ...
>> I've attached a patch that we are applying to the code to fix these problems.
>>
>> Here's the evaluation of why the changes have been made:
>>
>> There are three different types of Parfait errors here:
>>
>> 1/ In fmt.c, in the get_paragraph() routine, Parfait thinks that there
>>    is a potential problem of a negative index into the 'word' array.
>>    But that situation is impossible. Earlier in the routine, get_line()
>>    is called, and this has to have incremented word_limit. The solution
>>    was to add a Parfait style comment before the offending line of code
>>    to shut parfait up.
>>
>> 2/ In sort.c, there is an occurrence in the main() routine where 's' was
>>    being dereferenced, but it could have been NULL. A check was added
>>    to only do the dereferencing if that wasn't the case. It was noticed
>>    that there was already similar code in the same routine, so this seems
>>    a reasonable solution.
>>
>> 3/ In stty.c, there were two occurrences where bitsp was being dereferenced
>>    that could have been a NULL pointer. In each case, a check was added
>>    to only do the dereferencing if that wasn't the case. It was noticed
>>    that there was already similar code in the sane_mode() routine, so this
>>    seems a reasonable solution.
>
> Thanks again.
> I've just confirmed that your proposed stty.c change
> is not required, since bitsp cannot be NULL when it is
> dereferenced.
>
> Are the following proposed changes enough to placate parfait?
> I prefer to use assert, because that tends to work also for
> static analysis tools like clang and coverity.
>
> Subject: [PATCH] maint: add assertions to placate static analysis tools
>
> A static analysis tool (http://labs.oracle.com/projects/parfait/)
> produced some false positive diagnostics.  Add assertions to help
> it understand that the code is correct.
> * src/stty.c: Include <assert.h>.
> (display_changed): Add an assertion to placate parfait.
> (display_all): Likewise.
> * src/sort.c: Include <assert.h>.
> (main): Add an assertion to placate parfait.

Hi Rich,

If you can confirm that the following incremental patch
is enough to inform parfait that there's no real problem
in this part of fmt.c, I'll push the full patch below.

diff --git a/src/fmt.c b/src/fmt.c
index 308b645..e0c04cb 100644
--- a/src/fmt.c
+++ b/src/fmt.c
@@ -20,6 +20,7 @@
 #include <stdio.h>
 #include <sys/types.h>
 #include <getopt.h>
+#include <assert.h>

 /* Redefine.  Otherwise, systems (Unicos for one) with headers that define
    it to be a type get syntax errors for the variable declaration below.  */
@@ -610,6 +611,11 @@ get_paragraph (FILE *f)
       while (same_para (c) && in_column == other_indent)
         c = get_line (f, c);
     }
+
+  /* Tell static analysis tools that word_limit[-1] is ok.
+     word_limit is guaranteed to have been incremented by get_line.  */
+  assert (word < word_limit);
+
   (word_limit - 1)->period = (word_limit - 1)->final = true;
   next_char = c;
   return true;


>From 5917d6365e03eee7c28c4add3acf79e9f473d703 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Mon, 14 May 2012 15:44:41 +0200
Subject: [PATCH] maint: add assertions to placate static analysis tools

A static analysis tool (http://labs.oracle.com/projects/parfait/)
produced some false positive diagnostics.  Add assertions to help
it understand that the code is correct.
* src/stty.c: Include <assert.h>.
(display_changed): Add an assertion to placate parfait.
(display_all): Likewise.
* src/sort.c: Include <assert.h>.
(main): Add an assertion to placate parfait.
* src/fmt.c: Include <assert.h>.
(get_paragraph): Add an assertion to placate parfait.
---
 src/fmt.c  | 6 ++++++
 src/sort.c | 5 +++++
 src/stty.c | 8 ++++++++
 3 files changed, 19 insertions(+)

diff --git a/src/fmt.c b/src/fmt.c
index 308b645..3da198e 100644
--- a/src/fmt.c
+++ b/src/fmt.c
@@ -20,6 +20,7 @@
 #include <stdio.h>
 #include <sys/types.h>
 #include <getopt.h>
+#include <assert.h>

 /* Redefine.  Otherwise, systems (Unicos for one) with headers that define
    it to be a type get syntax errors for the variable declaration below.  */
@@ -610,6 +611,11 @@ get_paragraph (FILE *f)
       while (same_para (c) && in_column == other_indent)
         c = get_line (f, c);
     }
+
+  /* Tell static analysis tools that using word_limit[-1] is ok.
+     word_limit is guaranteed to have been incremented by get_line.  */
+  assert (word < word_limit);
+
   (word_limit - 1)->period = (word_limit - 1)->final = true;
   next_char = c;
   return true;
diff --git a/src/sort.c b/src/sort.c
index 493e7f1..2593a2a 100644
--- a/src/sort.c
+++ b/src/sort.c
@@ -28,6 +28,7 @@
 #include <sys/types.h>
 #include <sys/wait.h>
 #include <signal.h>
+#include <assert.h>
 #include "system.h"
 #include "argmatch.h"
 #include "error.h"
@@ -4243,6 +4244,10 @@ main (int argc, char **argv)
                           char const *optarg1 = argv[optind++];
                           s = parse_field_count (optarg1 + 1, &key->eword,
                                              N_("invalid number after '-'"));
+                          /* When called with a non-NULL message ID,
+                             parse_field_count cannot return NULL.  Tell static
+                             analysis tools that dereferencing S is safe.  */
+                          assert (s);
                           if (*s == '.')
                             s = parse_field_count (s + 1, &key->echar,
                                                N_("invalid number after '.'"));
diff --git a/src/stty.c b/src/stty.c
index eb07f85..a3fc3dd 100644
--- a/src/stty.c
+++ b/src/stty.c
@@ -52,6 +52,7 @@
 #endif
 #include <getopt.h>
 #include <stdarg.h>
+#include <assert.h>

 #include "system.h"
 #include "error.h"
@@ -1538,6 +1539,12 @@ display_changed (struct termios *mode)

       bitsp = mode_type_flag (mode_info[i].type, mode);
       mask = mode_info[i].mask ? mode_info[i].mask : mode_info[i].bits;
+
+      /* bitsp would be NULL only for "combination" modes, yet those
+         are filtered out above via the OMIT flag.  Tell static analysis
+         tools that it's ok to dereference bitsp here.  */
+      assert (bitsp);
+
       if ((*bitsp & mask) == mode_info[i].bits)
         {
           if (mode_info[i].flags & SANE_UNSET)
@@ -1615,6 +1622,7 @@ display_all (struct termios *mode, char const 
*device_name)

       bitsp = mode_type_flag (mode_info[i].type, mode);
       mask = mode_info[i].mask ? mode_info[i].mask : mode_info[i].bits;
+      assert (bitsp); /* See the identical assertion and comment above.  */
       if ((*bitsp & mask) == mode_info[i].bits)
         wrapf ("%s", mode_info[i].name);
       else if (mode_info[i].flags & REV)
--
1.7.10.2.484.gcd07cc5