<<< Date Index >>>     <<< Thread Index >>>

Re: [PATCH 7 of 9] Fix comparison signedness warning



On Fri, Aug 06, 2010 at 11:49:24PM +0200, Vincent Lefevre wrote:
On 2010-08-06 23:33:29 +0200, Matthias Andree wrote:
-    if (chunk >= sizeof (buf))
+    if ((size_t)chunk >= sizeof (buf))

I would say that a signedness warning would be here a compiler bug
since with a simple variable range analysis, one can deduce that
chunk is non-negative. With such a cast, you may hide a real bug
if the context changes, e.g. because a better compiler would no
longer be able to emit a warning.

FYI, GCC 4.4.5 doesn't have this problem.

xvii:~/software/mutt/mutt> make pop_lib.o
gcc -DPKGDATADIR=\"/home/vinc17/share/mutt\" -DSYSCONFDIR=\"/home/vinc17/etc\" -DDOTLOCK_PATH=\"/usr/bin/mutt_dotlock\" -DMUTTLOCALEDIR=\"/home/vinc17/share/locale\" -DHAVE_CONFIG_H=1 -I. -I. -I. -I./imap -Iintl -I./intl -Wall -pedantic -Wno-long-long -g -O2 -MT pop_lib.o -MD -MP -MF .deps/pop_lib.Tpo -c -o pop_lib.o pop_lib.c
mv -f .deps/pop_lib.Tpo .deps/pop_lib.Po

Michael Elkins wrote:

gcc 4.4.3 w/ -Wextra does emit this error. I think that is the difference you
are seeing.

Vincent, Michael,

same here, "gcc (SUSE Linux) 4.5.0 20100604 [gcc-4_5-branch revision 160292]" needs -Wextra for this warning, and cannot - unlike a human - deduce that 'chunk' is non-negative. Haven't tried upcoming GCC 4.6 developer versions though, neither clang 2.7.

Because of the deduction, I cast without adding a check, but you have a valid point. Note there is a "if (chunk < 0)" check above.

This is the whole function, comments below; indentation clobbered by unexpand and nl:

   454  /*
   455   * This function calls  funct(*line, *data)  for each received line,
456 * funct(NULL, *data) if rewind(*data) needs, exits when fail or done.
   457   * Returned codes:
   458   *  0 - successful,
   459   * -1 - conection lost,
   460   * -2 - invalid command or execution error,
   461   * -3 - error in funct(*line, *data)
   462   */
463 int pop_fetch_data (POP_DATA *pop_data, char *query, progress_t *progressbar,
   464                      int (*funct) (char *, void *), void *data)
   465  {
   466    char buf[LONG_STRING];
   467    char *inbuf;
   468    char *p;
   469    int ret, chunk = 0;
   470    long pos = 0;
   471    size_t lenbuf = 0;
   472  
   473    strfcpy (buf, query, sizeof (buf));
   474    ret = pop_query (pop_data, buf, sizeof (buf));
   475    if (ret < 0)
   476      return ret;
   477  
   478    inbuf = safe_malloc (sizeof (buf));
   479  
   480    FOREVER
   481    {
482 chunk = mutt_socket_readln_d (buf, sizeof (buf), pop_data->conn, M_SOCK_LOG_HDR);
   483      if (chunk < 0)
   484      {
   485        pop_data->status = POP_DISCONNECTED;
   486        ret = -1;
   487        break;
   488      }
   489  
   490      p = buf;
   491      if (!lenbuf && buf[0] == '.')
   492      {
   493        if (buf[1] != '.')
   494          break;
   495        p++;
   496      }
   497  
   498      strfcpy (inbuf + lenbuf, p, sizeof (buf));
   499      pos += chunk;
   500  
   501      if ((size_t)chunk >= sizeof (buf))
   502      {
   503        lenbuf += strlen (p);
   504      }
   505      else
   506      {
   507        if (progressbar)
   508          mutt_progress_update (progressbar, pos, -1);
   509        if (ret == 0 && funct (inbuf, data) < 0)
   510          ret = -3;
   511        lenbuf = 0;
   512      }
   513  
   514      safe_realloc (&inbuf, lenbuf + sizeof (buf));
   515    }
   516  
   517    FREE (&inbuf);
   518    return ret;
   519  }

So we see the relvant check (chunk < 0 => break out of the loop) is at ll. 483..488.

The issue is with the ill-defined read() function signatures and return value semantics: the overloading of error and read length in the same ssize_t-typed return value is awkward. That will only ever use EOF == -1 from the bottom half of its return value domain, so you always have ssize_t (or worse, int in this case) returned from read-like functions where it would be useful if we had something like:

int newread(void *INPUT_OBJECT, char *buffer, size_t bufmax, size_t *actually_read);

where INPUT_OBJECT might be fd, FILE * whatever, buffer/bufmax as usual, and actually_read obtains the read size. ret can be 0 for success, or EOF (-1) for error. That resolves the in-band signalling that needs to overload the size. Yes, it enlarges the signature and hurts big time on x86, but I don't care. Any reasonable 16- or 32-bit microcontroller of the past two or three decades can handle that with little penalty. In doubt, drop bufmax and pass it in through actually_read.

I think it's too late now, for POSIX as well as for mutt (because we'd rewrite more or less the whole code).

Of course Vincent can validly argue that the compiler is too dumb to figure that the code isn't reached for chunk < 0, but it's a context-sensitive check - so it's expensive because it needs to track the whole decision tree. clang in analyzer mode might figure that out, but it's sloooooow that way.

So what do we do? Drop this patch? Keep it? Add a comment before line 501? Add an assertion before line 501?

--
Matthias Andree