On Wed, Oct 14, 2009 at 3:40 PM, Cristian Ionescu-Idbohrn
<[email protected]> wrote:
>> archival/bz/blocksort.c:260: warning: conversion to 'uint32_t' from
>> 'int32_t' may change the sign of the result
>>
>> What to do? fmap[k] = (uint32_t)i; ?
>
> Something similar is done a few lines down:
>
> 263: nBhtab = 2 + ((uint32_t)nblock / 32); /* bbox: unsigned div is easier
> */
_This_ produces smaller code, unlike "fmap[k] = (uint32_t)i"
which produces only warm and fuzzy feeling "the warning is gone"
and adds to code obfuscation.
> Does 'i' _have to_ be int32_t? Looks like it's because nblock is
> int32_t. Theoretically, nblock could be negative and there's nothing
> in the function that validates that.
nblock can't be negative. I looked into it. But the sheer amount
of such tweaks in bz2 code in order to just reduce, not eliminate,
warnings there, is big enough to make me worry about
introducing a subtle bug.
> Anyway, 'i' iterates from 0 to something '< nblock', and it can't ever
> get negative in that loop. After validation (nblock >= 0), I would do
> it like this:
>
> uint32_t i;
>
>> Or this:
>>
>> k = fmap[i] - H;
>> if (k < 0)
>> k += nblock;
>> eclass[k] = j;
>>
>> fmap[i] and H are uint32_t, k is int32_t, and it CAN be negative.
>
> Right. And, theoretically, k may be < 0 even after:
>
> if (k < 0)
> k += nblock;
>
> and:
>
> eclass[k] = j;
>
> will misbehave. More code is needed to get it right.
When bz2 compressor will start failing after this on some obscure file,
I'll come to you ok?
One bz2 bug was along these lines: "If you compress 20 Gigabytes of
bytes with value 351, bzip2 segfaults". Do you really want to debug
*that sort* of bugs just to make warnings go away?
>> If you can deal with some if them by changing signedness of the
>> variables, that is ok.
>
> That is obvious.
>
>> Adding casts, and widening variables is less desirable.
>
> Yes. But how do you do it. Just casting is not smart enough.
Casting is actively bad. In "a = b" gcc can warn you about many
problems. In "a = (long)b" gcc will not warn even if b
is a char pointer!
> Is it that smart and tempting to often push your luck just a little
> bit more in order to shrink the code size with a few more bytes (yes,
> I know, they add up; but still...)?
And what about additions? For example, du sums file lengths.
If a and b are off_t's, a + b can overflow off_t!
Do you want to fix that too? How?
Oh no, look, BUG!
int allocated_out = 0;
...
while ((c = getc(istream)) != EOF) {
if (offset_out + 1 >= allocated_out) {
allocated_out += 1024;
line_out = xrealloc(line_out, allocated_out);
}
if (c == '\n') {...}
if (column > width) {...}
line_out[offset_out++] = c;
What if allocated_out == INT_MAX? "offset_out + 1" is < 0,
and "line_out[offset_out++] = c" stores c at line_out[INT_MAX],
next one would store a byte at line_out[-INT_MAX-1]...
I can find a lot of "theoretical" bugs like this.
I can even construct a file which will trigger it.
Your expectations of software reliability are at odds
with reality. Programs are not THAT reliable.
Push just about anything far enough into a corner case,
and it will break.
--
vda
_______________________________________________
busybox mailing list
[email protected]
http://lists.busybox.net/mailman/listinfo/busybox
