Re: Static code-checking observations

[Gavin Smith]

On 25 January 2017 at 00:22, Hans-Bernhard Bröker <address@hidden> wrote:

> I had a go with clang's analyzer (on Cygwin) on current SVN source (r7649).
> It found about a dozen possible uninitalized reads, use-after-frees and null
> pointer dereferences, primarily in 'info':
>
> (Reported "dead stores" snipped here)
> Logic error
> Branch condition evaluates to a garbage value           2
> Called function pointer is null (null dereference)      1
> Dereference of null pointer                             6
> Result of operation is garbage or undefined             1
> Unix API (<-- NULL passed to string functions)          2
>
> Memory Error
> Memory leak                                             1
> Use-after-free                                          2
>
> (Reports list attached separately because lines are too long).
>
> The full report is a bit big for sending via the list (1 MB zipped), so I've
> attached a preliminary set of patches for many of the above instead.

I've worked through the set of patches you sent and tried to
understand what they were meant to fix. This is what I have left:

Index: infodoc.c
===================================================================
--- infodoc.c   (Revision 7649)
+++ infodoc.c   (Arbeitskopie)
@@ -372,7 +372,7 @@
   /* If there is more than one window on the screen, check if the user typed
      "H" for help message before typing "h" for tutorial.  If so, close help
      message so the tutorial will not be in a small window. */
-  if (windows->next)
+  if (windows && windows->next)
     {
       WINDOW *help_window = get_internal_info_window (info_help_nodename);
       if (help_window && help_window == active_window)

I think this flagged up as a problem because of this loop earlier:

for (win = windows; win; win = win->next)

which would have checked that 'windows' was not null. However, the
check in the loop ("win") is needed for subsequent entries in the
linked list, but not for the first. The loop could be rewritten to put
the condition at the end so it isn't checked for the first entry in
the linked list, but I doubt that this would help the clarity of the
code.

Index: tilde.c
===================================================================
--- tilde.c     (Revision 7649)
+++ tilde.c     (Arbeitskopie)
@@ -114,16 +114,13 @@
 char *
 tilde_expand (char *string)
 {
-  char *result;
-  int result_size, result_index;
+  char *result = NULL;
+  unsigned int result_size = 0, result_index = 0;

-  result_size = result_index = 0;
-  result = NULL;
-
   /* Scan through STRING expanding tildes as we come to them. */
   while (1)
     {
-      register int start, end;
+      unsigned int start, end;
       char *tilde_word, *expansion;
       int len;

@@ -132,7 +129,10 @@

       /* Copy the skipped text into the result. */
       if ((result_index + start + 1) > result_size)
-        result = xrealloc (result, 1 + (result_size += (start + 20)));
+        {
+          result_size += start + 20;
+          result = xrealloc (result, 1 + result_size);
+        }

       strncpy (result + result_index, string, start);
       result_index += start;

I don't know what this is supposed to fix. The line in the log

Logic error     Unix API
 info/tilde.c                            tilde_expand
  137     3

refers to the line with "strncpy" on it.