Re: [Qemu-devel] [PATCH 1/5] Suppress some gcc warnings with -Wtype-limits

[andrzej zaborowski]

Hi,

On 4 September 2010 21:45, Blue Swirl <address@hidden> wrote:
> On Sat, Sep 4, 2010 at 5:57 PM, andrzej zaborowski <address@hidden> wrote:
>>>  -    if (event < 0 || event >= BLKDBG_EVENT_MAX) {
>>>  +    if ((int)event < 0 || event >= BLKDBG_EVENT_MAX) {
>> Is the behaviour incorrect for some values, or is it not correct C?
>> As far as I know it is correct in c99 because of type promotions
>> between enum, int and unsigned int.
>
> The problem is that the type of an enum may be signed or unsigned,
> which affects the comparison. For example, the following program
> produces different results when it's compiled with -DUNSIGNED and
> without:
> $ cat enum.c
> #include <stdio.h>
>
> int main(int argc, const char **argv)
> {
>    enum {
> #ifdef UNSIGNED
>        A = 0,
> #else
>        A = -1,
> #endif
>    } a;
>
>    a = atoi(argv[1]);
>    if (a < 0) {
>        puts("1: smaller");
>    } else {
>        puts("1: not smaller");
>    }
>
>    if ((int)a < 0) {
>        puts("2: smaller");
>    } else {
>        puts("2: not smaller");
>    }
>
>    return 0;
> }
> $ gcc -DUNSIGNED enum.c -o enum-unsigned
> $ gcc enum.c -o enum-signed
> $ ./enum-signed 0
> 1: not smaller
> 2: not smaller
> $ ./enum-signed -1
> 1: smaller
> 2: smaller
> $ ./enum-unsigned 0
> 1: not smaller
> 2: not smaller
> $ ./enum-unsigned -1
> 1: not smaller
> 2: smaller

Ah, that's a good example, however..

>
> This is only how GCC uses enums, other compilers have other rules. So
> it's better to avoid any assumptions about signedness of enums.
>
> In this specific case, because the enum does not have any negative
> items, it is unsigned if compiled with GCC. Unsigned items cannot be
> negative, thus the check is useless.

I agree it's useless, but saying that it is wrong is a bit of a
stretch in my opinion.  It actually depends on what the intent was --
if the intent was to be able to use the value as an array index, then
I think the check does the job independent of the signedness of the
operands.

>
> If the enum included also negative values (or compiled with a compiler
> using different rules), the check would succeed. Though then the check
> against 0 would be wrong because the author probably wanted to check
> against the smallest possible value.
>
> In both cases, the cast to int makes sure that the comparison is meaningful.
>
>>>>>> There are some projects that avoid using strcat for example, because,
>>>>>> if misused, it can cause crashes.
>>>>>
>>>>> We also do that, there's pstrcat() and others.
>>>>
>>>> I don't believe we avoid strcat everywhere.  strcat and pstrcat are
>>>> different functions to be used on different occasions.  It'd be the
>>>> same level of ridiculous as not using malloc or not using the number 5
>>>> (because pstrcat(NULL, 5, "a") causes a crash) with an added
>>>> performance penalty.
>>>
>>> There's no difference in using using strcat vs. pstrcat, or sprintf
>>> vs. snprintf. If the caller of strcat doesn't know the buffer size,
>>> someone down the call chain must know it, so it can be passed. The
>>> major benefit is that buffer overflows will be avoided, at the
>>> possible cost of extra parameter passing. Again, the benefit exceeds
>>> cost.
>>
>> Usually you'll allocate the buffer of the size that is needed, so you
>> can do for exmple
>>
>> buffer = qemu_malloc(strlen(this) + strlen(that) + 1);
>> and then call strcpy and strcat
>
> But then you can easily add
> buflen = strlen(this) + strlen(that) + 1;
> and use that for malloc and pstrcat. Cost: one additional variable.

Plus the cost of the strlen inside pstrcat.  pstrcat has to either
check the length of source string against the buffer size first, or do
it at the same time it copies the string from source to destination
character by character, but either way the penalty is of linear cost.
If it was an inline function, then perhaps the compiler could optimise
the second strlen away in some situations, specially since strcat,
strlen etc are now builtins.

So the reason I dislike using it blindly is that often you already
know that a buffer overflow is out of question, and secondly if
misused, it can hide a real bug where the string gets unintentionally
truncated and as a result something worse than an overflow happens
(e.g. a file on disk is overwritten) without noticing whereas a
segfault would be noticed.

Cheers