Re: Static code-checking observations

[Hans-Bernhard Bröker]

Am 04.02.2017 um 02:02 schrieb Gavin Smith:

> 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)

I don't think so.  I see it as being flagged simply because the checker 
didn't see anything to guarantee that 'windows' itself isn't NULL.  Now 
that might be ensured by code elsewhere (e.g. this function might never 
even be called if windows were 0), but the checker saw no such guarantee.

> 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.

Exactly, and the trouble is that the checker sees a possiblity that 
strncpy's argument might be NULL.

The trouble here is that `result' is NULL until the branch doing that 
xrealloc has taken place.  The changes to unsigned are (failed) attempts 
to allow the checker to see through  those calculations more easily and 
realize that the condition will always be true on the first pass, and 
therefore the xrealloc would always make result non-NULL.

The ultimate obstacle may be xrealloc() here, though.  Its purpose is to 
never return unless it managed to make its output non-NULL.  But it's 
hard for the checker to figure that out cross-module.