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