| Message ID | AANLkTimrAJ7fJzOhJRXLVpRjc8F+CoxWsETHss6tmZG3@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On 4 September 2010 16:17, Blue Swirl <blauwirbel@gmail.com> wrote: > Add various casts, adjust types etc. to make the warnings with > gcc flag -Wtype-limits disappear. > > Signed-off-by: Blue Swirl <blauwirbel@gmail.com> > --- > block/blkdebug.c | 2 +- > hw/mips_fulong2e.c | 2 +- > hw/omap1.c | 2 +- > hw/ppc405_boards.c | 23 +++++++++++++---------- > hw/ppc_newworld.c | 3 ++- > hw/ppc_prep.c | 3 ++- > hw/pxa2xx.c | 2 +- > hw/sm501.c | 4 ++-- > linux-user/flatload.c | 3 ++- > linux-user/mmap.c | 2 +- > linux-user/syscall.c | 20 +++++++++++++------- > 11 files changed, 39 insertions(+), 27 deletions(-) > > diff --git a/block/blkdebug.c b/block/blkdebug.c > index 2a63df9..b5083bc 100644 > --- a/block/blkdebug.c > +++ b/block/blkdebug.c > @@ -439,7 +439,7 @@ static void blkdebug_debug_event(BlockDriverState > *bs, BlkDebugEvent event) > struct BlkdebugRule *rule; > BlkdebugVars old_vars = s->vars; > > - if (event < 0 || event >= BLKDBG_EVENT_MAX) { > + if ((unsigned int)event >= BLKDBG_EVENT_MAX) { > return; > } > > diff --git a/hw/mips_fulong2e.c b/hw/mips_fulong2e.c > index cbe7156..ac82067 100644 > --- a/hw/mips_fulong2e.c > +++ b/hw/mips_fulong2e.c > @@ -258,7 +258,7 @@ static void mips_fulong2e_init(ram_addr_t > ram_size, const char *boot_device, > { > char *filename; > unsigned long ram_offset, bios_offset; > - unsigned long bios_size; > + long bios_size; > int64_t kernel_entry; > qemu_irq *i8259; > qemu_irq *cpu_exit_irq; > diff --git a/hw/omap1.c b/hw/omap1.c > index 06370b6..b04aa80 100644 > --- a/hw/omap1.c > +++ b/hw/omap1.c > @@ -3675,7 +3675,7 @@ static int omap_validate_emiff_addr(struct > omap_mpu_state_s *s, > static int omap_validate_emifs_addr(struct omap_mpu_state_s *s, > target_phys_addr_t addr) > { > - return addr >= OMAP_EMIFS_BASE && addr < OMAP_EMIFF_BASE; > + return addr < OMAP_EMIFF_BASE; I think this only doesn't break the code because it relies on a specific value for that #define, and if the value is changed, the check has to be re-added. Same applies to other modifications in this patch. Most of the gcc warnings should not be enabled by default and I think this is one of them. They make you do some really strange things in the code just to avoid using one construct of your programming language (fully valid in the language), and in the end result in bugs of other types. It's probably ok to enable them once to see if they hint on any bugs, but if enabled by default, they'll just cause other types of bugs. There are some projects that avoid using strcat for example, because, if misused, it can cause crashes. There's even a pretty big project by Nokia that does not use malloc(), because again, "malloc can cause memory leaks". They just allocate all memory statically, and it works.. but is a pain to work with, and the program requires much more memory than needed so it doesn't run on some embedded devices. I wouldn't want to go this way. Cheers
On Sat, Sep 4, 2010 at 3:40 PM, andrzej zaborowski <balrogg@gmail.com> wrote: > On 4 September 2010 16:17, Blue Swirl <blauwirbel@gmail.com> wrote: >> Add various casts, adjust types etc. to make the warnings with >> gcc flag -Wtype-limits disappear. >> >> Signed-off-by: Blue Swirl <blauwirbel@gmail.com> >> --- >> block/blkdebug.c | 2 +- >> hw/mips_fulong2e.c | 2 +- >> hw/omap1.c | 2 +- >> hw/ppc405_boards.c | 23 +++++++++++++---------- >> hw/ppc_newworld.c | 3 ++- >> hw/ppc_prep.c | 3 ++- >> hw/pxa2xx.c | 2 +- >> hw/sm501.c | 4 ++-- >> linux-user/flatload.c | 3 ++- >> linux-user/mmap.c | 2 +- >> linux-user/syscall.c | 20 +++++++++++++------- >> 11 files changed, 39 insertions(+), 27 deletions(-) >> >> diff --git a/block/blkdebug.c b/block/blkdebug.c >> index 2a63df9..b5083bc 100644 >> --- a/block/blkdebug.c >> +++ b/block/blkdebug.c >> @@ -439,7 +439,7 @@ static void blkdebug_debug_event(BlockDriverState >> *bs, BlkDebugEvent event) >> struct BlkdebugRule *rule; >> BlkdebugVars old_vars = s->vars; >> >> - if (event < 0 || event >= BLKDBG_EVENT_MAX) { >> + if ((unsigned int)event >= BLKDBG_EVENT_MAX) { >> return; >> } >> >> diff --git a/hw/mips_fulong2e.c b/hw/mips_fulong2e.c >> index cbe7156..ac82067 100644 >> --- a/hw/mips_fulong2e.c >> +++ b/hw/mips_fulong2e.c >> @@ -258,7 +258,7 @@ static void mips_fulong2e_init(ram_addr_t >> ram_size, const char *boot_device, >> { >> char *filename; >> unsigned long ram_offset, bios_offset; >> - unsigned long bios_size; >> + long bios_size; >> int64_t kernel_entry; >> qemu_irq *i8259; >> qemu_irq *cpu_exit_irq; >> diff --git a/hw/omap1.c b/hw/omap1.c >> index 06370b6..b04aa80 100644 >> --- a/hw/omap1.c >> +++ b/hw/omap1.c >> @@ -3675,7 +3675,7 @@ static int omap_validate_emiff_addr(struct >> omap_mpu_state_s *s, >> static int omap_validate_emifs_addr(struct omap_mpu_state_s *s, >> target_phys_addr_t addr) >> { >> - return addr >= OMAP_EMIFS_BASE && addr < OMAP_EMIFF_BASE; >> + return addr < OMAP_EMIFF_BASE; > > I think this only doesn't break the code because it relies on a > specific value for that #define, and if the value is changed, the > check has to be re-added. Same applies to other modifications in this > patch. I think you mean other modifications like this, that is: this change and the pxa2xx.c one. I'll try to invent a better solution. How about adding assert(OMAP_EMIFS_BASE == 0) and commenting out the check? Then if the value changes, the need to add the comparison back will be obvious. > Most of the gcc warnings should not be enabled by default and I think > this is one of them. They make you do some really strange things in > the code just to avoid using one construct of your programming > language (fully valid in the language), and in the end result in bugs > of other types. It's probably ok to enable them once to see if they > hint on any bugs, but if enabled by default, they'll just cause other > types of bugs. I don't see how comparison of an unsigned value for >= 0 or < 0 can be useful. I found two questionable cases or bugs. Also in the bios_size/kernel_size cases, failures (indicated by return value < 0) were ignored. Most of other cases are just matter of mixing incorrect types so they can be fixed easily. > 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. > There's even a pretty big project > by Nokia that does not use malloc(), because again, "malloc can cause > memory leaks". They just allocate all memory statically, and it > works.. but is a pain to work with, and the program requires much more > memory than needed so it doesn't run on some embedded devices. I > wouldn't want to go this way. I agree. We have other reasons to be careful with QEMU memory management, but this is not one of them.
On 4 September 2010 18:14, Blue Swirl <blauwirbel@gmail.com> wrote: > On Sat, Sep 4, 2010 at 3:40 PM, andrzej zaborowski <balrogg@gmail.com> wrote: >> On 4 September 2010 16:17, Blue Swirl <blauwirbel@gmail.com> wrote: >>> Add various casts, adjust types etc. to make the warnings with >>> gcc flag -Wtype-limits disappear. >>> >>> Signed-off-by: Blue Swirl <blauwirbel@gmail.com> >>> --- >>> block/blkdebug.c | 2 +- >>> hw/mips_fulong2e.c | 2 +- >>> hw/omap1.c | 2 +- >>> hw/ppc405_boards.c | 23 +++++++++++++---------- >>> hw/ppc_newworld.c | 3 ++- >>> hw/ppc_prep.c | 3 ++- >>> hw/pxa2xx.c | 2 +- >>> hw/sm501.c | 4 ++-- >>> linux-user/flatload.c | 3 ++- >>> linux-user/mmap.c | 2 +- >>> linux-user/syscall.c | 20 +++++++++++++------- >>> 11 files changed, 39 insertions(+), 27 deletions(-) >>> >>> diff --git a/block/blkdebug.c b/block/blkdebug.c >>> index 2a63df9..b5083bc 100644 >>> --- a/block/blkdebug.c >>> +++ b/block/blkdebug.c >>> @@ -439,7 +439,7 @@ static void blkdebug_debug_event(BlockDriverState >>> *bs, BlkDebugEvent event) >>> struct BlkdebugRule *rule; >>> BlkdebugVars old_vars = s->vars; >>> >>> - if (event < 0 || event >= BLKDBG_EVENT_MAX) { >>> + if ((unsigned int)event >= BLKDBG_EVENT_MAX) { >>> return; >>> } >>> >>> diff --git a/hw/mips_fulong2e.c b/hw/mips_fulong2e.c >>> index cbe7156..ac82067 100644 >>> --- a/hw/mips_fulong2e.c >>> +++ b/hw/mips_fulong2e.c >>> @@ -258,7 +258,7 @@ static void mips_fulong2e_init(ram_addr_t >>> ram_size, const char *boot_device, >>> { >>> char *filename; >>> unsigned long ram_offset, bios_offset; >>> - unsigned long bios_size; >>> + long bios_size; >>> int64_t kernel_entry; >>> qemu_irq *i8259; >>> qemu_irq *cpu_exit_irq; >>> diff --git a/hw/omap1.c b/hw/omap1.c >>> index 06370b6..b04aa80 100644 >>> --- a/hw/omap1.c >>> +++ b/hw/omap1.c >>> @@ -3675,7 +3675,7 @@ static int omap_validate_emiff_addr(struct >>> omap_mpu_state_s *s, >>> static int omap_validate_emifs_addr(struct omap_mpu_state_s *s, >>> target_phys_addr_t addr) >>> { >>> - return addr >= OMAP_EMIFS_BASE && addr < OMAP_EMIFF_BASE; >>> + return addr < OMAP_EMIFF_BASE; >> >> I think this only doesn't break the code because it relies on a >> specific value for that #define, and if the value is changed, the >> check has to be re-added. Same applies to other modifications in this >> patch. > > I think you mean other modifications like this, that is: this change > and the pxa2xx.c one. I'll try to invent a better solution. Well the other ones are probably just pointless, not wrong. For example the first change in this patch really makes you wonder, until you think of the famous gcc warnings. There can't be any better reason for such a change. > > How about adding assert(OMAP_EMIFS_BASE == 0) and commenting out the > check? Then if the value changes, the need to add the comparison back > will be obvious. This would work but it's weird. The thing is it's currently a correct code and the check may be useless but it's the optimiser's task to remove it, not ours. The compiler is not able to tell whether the check makes sense or nott, because the compiler only has access to preprocessed code. So why should you let the compiler have anything to say on it. > >> Most of the gcc warnings should not be enabled by default and I think >> this is one of them. They make you do some really strange things in >> the code just to avoid using one construct of your programming >> language (fully valid in the language), and in the end result in bugs >> of other types. It's probably ok to enable them once to see if they >> hint on any bugs, but if enabled by default, they'll just cause other >> types of bugs. > > I don't see how comparison of an unsigned value for >= 0 or < 0 can be > useful. I found two questionable cases or bugs. Also in the > bios_size/kernel_size cases, failures (indicated by return value < 0) > were ignored. Most of other cases are just matter of mixing incorrect > types so they can be fixed easily. That's why it may make sense to enable the warning to check for errors. But enabled by default it causes other types of bugs as people have to work around their compiler to avoid language constructs that are not kosher in the given compiler version. > >> 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. Cheers
On Sat, Sep 4, 2010 at 4:44 PM, andrzej zaborowski <balrogg@gmail.com> wrote: > On 4 September 2010 18:14, Blue Swirl <blauwirbel@gmail.com> wrote: >> On Sat, Sep 4, 2010 at 3:40 PM, andrzej zaborowski <balrogg@gmail.com> wrote: >>> On 4 September 2010 16:17, Blue Swirl <blauwirbel@gmail.com> wrote: >>>> Add various casts, adjust types etc. to make the warnings with >>>> gcc flag -Wtype-limits disappear. >>>> >>>> Signed-off-by: Blue Swirl <blauwirbel@gmail.com> >>>> --- >>>> block/blkdebug.c | 2 +- >>>> hw/mips_fulong2e.c | 2 +- >>>> hw/omap1.c | 2 +- >>>> hw/ppc405_boards.c | 23 +++++++++++++---------- >>>> hw/ppc_newworld.c | 3 ++- >>>> hw/ppc_prep.c | 3 ++- >>>> hw/pxa2xx.c | 2 +- >>>> hw/sm501.c | 4 ++-- >>>> linux-user/flatload.c | 3 ++- >>>> linux-user/mmap.c | 2 +- >>>> linux-user/syscall.c | 20 +++++++++++++------- >>>> 11 files changed, 39 insertions(+), 27 deletions(-) >>>> >>>> diff --git a/block/blkdebug.c b/block/blkdebug.c >>>> index 2a63df9..b5083bc 100644 >>>> --- a/block/blkdebug.c >>>> +++ b/block/blkdebug.c >>>> @@ -439,7 +439,7 @@ static void blkdebug_debug_event(BlockDriverState >>>> *bs, BlkDebugEvent event) >>>> struct BlkdebugRule *rule; >>>> BlkdebugVars old_vars = s->vars; >>>> >>>> - if (event < 0 || event >= BLKDBG_EVENT_MAX) { >>>> + if ((unsigned int)event >= BLKDBG_EVENT_MAX) { >>>> return; >>>> } >>>> >>>> diff --git a/hw/mips_fulong2e.c b/hw/mips_fulong2e.c >>>> index cbe7156..ac82067 100644 >>>> --- a/hw/mips_fulong2e.c >>>> +++ b/hw/mips_fulong2e.c >>>> @@ -258,7 +258,7 @@ static void mips_fulong2e_init(ram_addr_t >>>> ram_size, const char *boot_device, >>>> { >>>> char *filename; >>>> unsigned long ram_offset, bios_offset; >>>> - unsigned long bios_size; >>>> + long bios_size; >>>> int64_t kernel_entry; >>>> qemu_irq *i8259; >>>> qemu_irq *cpu_exit_irq; >>>> diff --git a/hw/omap1.c b/hw/omap1.c >>>> index 06370b6..b04aa80 100644 >>>> --- a/hw/omap1.c >>>> +++ b/hw/omap1.c >>>> @@ -3675,7 +3675,7 @@ static int omap_validate_emiff_addr(struct >>>> omap_mpu_state_s *s, >>>> static int omap_validate_emifs_addr(struct omap_mpu_state_s *s, >>>> target_phys_addr_t addr) >>>> { >>>> - return addr >= OMAP_EMIFS_BASE && addr < OMAP_EMIFF_BASE; >>>> + return addr < OMAP_EMIFF_BASE; >>> >>> I think this only doesn't break the code because it relies on a >>> specific value for that #define, and if the value is changed, the >>> check has to be re-added. Same applies to other modifications in this >>> patch. >> >> I think you mean other modifications like this, that is: this change >> and the pxa2xx.c one. I'll try to invent a better solution. > > Well the other ones are probably just pointless, not wrong. For > example the first change in this patch really makes you wonder, until > you think of the famous gcc warnings. There can't be any better > reason for such a change. In the unsigned number space, the checks can be merged into one, assuming that BLKDBG_EVEN_MAX is less than INT_MAX. Alternatively we could have: - if (event < 0 || event >= BLKDBG_EVENT_MAX) { + if ((int)event < 0 || event >= BLKDBG_EVENT_MAX) { This would also implement the check that the writer of this code was trying to make. The important thing to note is however is that the check as it is now is not correct. >> How about adding assert(OMAP_EMIFS_BASE == 0) and commenting out the >> check? Then if the value changes, the need to add the comparison back >> will be obvious. > > This would work but it's weird. The thing is it's currently a correct > code and the check may be useless but it's the optimiser's task to > remove it, not ours. The compiler is not able to tell whether the > check makes sense or nott, because the compiler only has access to > preprocessed code. So why should you let the compiler have anything > to say on it. Good point. I'll try to invent something better. >>> Most of the gcc warnings should not be enabled by default and I think >>> this is one of them. They make you do some really strange things in >>> the code just to avoid using one construct of your programming >>> language (fully valid in the language), and in the end result in bugs >>> of other types. It's probably ok to enable them once to see if they >>> hint on any bugs, but if enabled by default, they'll just cause other >>> types of bugs. >> >> I don't see how comparison of an unsigned value for >= 0 or < 0 can be >> useful. I found two questionable cases or bugs. Also in the >> bios_size/kernel_size cases, failures (indicated by return value < 0) >> were ignored. Most of other cases are just matter of mixing incorrect >> types so they can be fixed easily. > > That's why it may make sense to enable the warning to check for > errors. But enabled by default it causes other types of bugs as > people have to work around their compiler to avoid language constructs > that are not kosher in the given compiler version. This extra effort should be balanced against the benefits. I still think the benefits in the bug detection outweigh the efforts to work around the (somewhat artificial, I agree) problems. >>> 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.
On 4 September 2010 19:21, Blue Swirl <blauwirbel@gmail.com> wrote: > On Sat, Sep 4, 2010 at 4:44 PM, andrzej zaborowski <balrogg@gmail.com> wrote: >> On 4 September 2010 18:14, Blue Swirl <blauwirbel@gmail.com> wrote: >>> On Sat, Sep 4, 2010 at 3:40 PM, andrzej zaborowski <balrogg@gmail.com> wrote: >>>> On 4 September 2010 16:17, Blue Swirl <blauwirbel@gmail.com> wrote: >>>>> Add various casts, adjust types etc. to make the warnings with >>>>> gcc flag -Wtype-limits disappear. >>>>> >>>>> Signed-off-by: Blue Swirl <blauwirbel@gmail.com> >>>>> --- >>>>> block/blkdebug.c | 2 +- >>>>> hw/mips_fulong2e.c | 2 +- >>>>> hw/omap1.c | 2 +- >>>>> hw/ppc405_boards.c | 23 +++++++++++++---------- >>>>> hw/ppc_newworld.c | 3 ++- >>>>> hw/ppc_prep.c | 3 ++- >>>>> hw/pxa2xx.c | 2 +- >>>>> hw/sm501.c | 4 ++-- >>>>> linux-user/flatload.c | 3 ++- >>>>> linux-user/mmap.c | 2 +- >>>>> linux-user/syscall.c | 20 +++++++++++++------- >>>>> 11 files changed, 39 insertions(+), 27 deletions(-) >>>>> >>>>> diff --git a/block/blkdebug.c b/block/blkdebug.c >>>>> index 2a63df9..b5083bc 100644 >>>>> --- a/block/blkdebug.c >>>>> +++ b/block/blkdebug.c >>>>> @@ -439,7 +439,7 @@ static void blkdebug_debug_event(BlockDriverState >>>>> *bs, BlkDebugEvent event) >>>>> struct BlkdebugRule *rule; >>>>> BlkdebugVars old_vars = s->vars; >>>>> >>>>> - if (event < 0 || event >= BLKDBG_EVENT_MAX) { >>>>> + if ((unsigned int)event >= BLKDBG_EVENT_MAX) { >>>>> return; >>>>> } >>>>> >>>>> diff --git a/hw/mips_fulong2e.c b/hw/mips_fulong2e.c >>>>> index cbe7156..ac82067 100644 >>>>> --- a/hw/mips_fulong2e.c >>>>> +++ b/hw/mips_fulong2e.c >>>>> @@ -258,7 +258,7 @@ static void mips_fulong2e_init(ram_addr_t >>>>> ram_size, const char *boot_device, >>>>> { >>>>> char *filename; >>>>> unsigned long ram_offset, bios_offset; >>>>> - unsigned long bios_size; >>>>> + long bios_size; >>>>> int64_t kernel_entry; >>>>> qemu_irq *i8259; >>>>> qemu_irq *cpu_exit_irq; >>>>> diff --git a/hw/omap1.c b/hw/omap1.c >>>>> index 06370b6..b04aa80 100644 >>>>> --- a/hw/omap1.c >>>>> +++ b/hw/omap1.c >>>>> @@ -3675,7 +3675,7 @@ static int omap_validate_emiff_addr(struct >>>>> omap_mpu_state_s *s, >>>>> static int omap_validate_emifs_addr(struct omap_mpu_state_s *s, >>>>> target_phys_addr_t addr) >>>>> { >>>>> - return addr >= OMAP_EMIFS_BASE && addr < OMAP_EMIFF_BASE; >>>>> + return addr < OMAP_EMIFF_BASE; >>>> >>>> I think this only doesn't break the code because it relies on a >>>> specific value for that #define, and if the value is changed, the >>>> check has to be re-added. Same applies to other modifications in this >>>> patch. >>> >>> I think you mean other modifications like this, that is: this change >>> and the pxa2xx.c one. I'll try to invent a better solution. >> >> Well the other ones are probably just pointless, not wrong. For >> example the first change in this patch really makes you wonder, until >> you think of the famous gcc warnings. There can't be any better >> reason for such a change. > > In the unsigned number space, the checks can be merged into one, > assuming that BLKDBG_EVEN_MAX is less than INT_MAX. Alternatively we > could have: > - if (event < 0 || event >= BLKDBG_EVENT_MAX) { > + if ((int)event < 0 || event >= BLKDBG_EVENT_MAX) { > > This would also implement the check that the writer of this code was > trying to make. > > The important thing to note is however is that the check as it is now > is not correct. 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. > >>>> 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 You know the size of the buffer, but there is no risk of an overflow. Yet, some projects prohibit code like this. Using pstrcpy, pstrcat, snprintf unavoidably add the overhead of looping over the input to find the length again. On other ocasions your input buffer and output buffer both have constant length and if the input is smaller, then there's no need to truncate. On other occasions you want to return an error if the input is too long. pstrcpy is used when the input should be truncated in case it's too long, and no error returned. Cheers
On Sat, Sep 4, 2010 at 5:57 PM, andrzej zaborowski <balrogg@gmail.com> wrote: > On 4 September 2010 19:21, Blue Swirl <blauwirbel@gmail.com> wrote: >> On Sat, Sep 4, 2010 at 4:44 PM, andrzej zaborowski <balrogg@gmail.com> wrote: >>> On 4 September 2010 18:14, Blue Swirl <blauwirbel@gmail.com> wrote: >>>> On Sat, Sep 4, 2010 at 3:40 PM, andrzej zaborowski <balrogg@gmail.com> wrote: >>>>> On 4 September 2010 16:17, Blue Swirl <blauwirbel@gmail.com> wrote: >>>>>> Add various casts, adjust types etc. to make the warnings with >>>>>> gcc flag -Wtype-limits disappear. >>>>>> >>>>>> Signed-off-by: Blue Swirl <blauwirbel@gmail.com> >>>>>> --- >>>>>> block/blkdebug.c | 2 +- >>>>>> hw/mips_fulong2e.c | 2 +- >>>>>> hw/omap1.c | 2 +- >>>>>> hw/ppc405_boards.c | 23 +++++++++++++---------- >>>>>> hw/ppc_newworld.c | 3 ++- >>>>>> hw/ppc_prep.c | 3 ++- >>>>>> hw/pxa2xx.c | 2 +- >>>>>> hw/sm501.c | 4 ++-- >>>>>> linux-user/flatload.c | 3 ++- >>>>>> linux-user/mmap.c | 2 +- >>>>>> linux-user/syscall.c | 20 +++++++++++++------- >>>>>> 11 files changed, 39 insertions(+), 27 deletions(-) >>>>>> >>>>>> diff --git a/block/blkdebug.c b/block/blkdebug.c >>>>>> index 2a63df9..b5083bc 100644 >>>>>> --- a/block/blkdebug.c >>>>>> +++ b/block/blkdebug.c >>>>>> @@ -439,7 +439,7 @@ static void blkdebug_debug_event(BlockDriverState >>>>>> *bs, BlkDebugEvent event) >>>>>> struct BlkdebugRule *rule; >>>>>> BlkdebugVars old_vars = s->vars; >>>>>> >>>>>> - if (event < 0 || event >= BLKDBG_EVENT_MAX) { >>>>>> + if ((unsigned int)event >= BLKDBG_EVENT_MAX) { >>>>>> return; >>>>>> } >>>>>> >>>>>> diff --git a/hw/mips_fulong2e.c b/hw/mips_fulong2e.c >>>>>> index cbe7156..ac82067 100644 >>>>>> --- a/hw/mips_fulong2e.c >>>>>> +++ b/hw/mips_fulong2e.c >>>>>> @@ -258,7 +258,7 @@ static void mips_fulong2e_init(ram_addr_t >>>>>> ram_size, const char *boot_device, >>>>>> { >>>>>> char *filename; >>>>>> unsigned long ram_offset, bios_offset; >>>>>> - unsigned long bios_size; >>>>>> + long bios_size; >>>>>> int64_t kernel_entry; >>>>>> qemu_irq *i8259; >>>>>> qemu_irq *cpu_exit_irq; >>>>>> diff --git a/hw/omap1.c b/hw/omap1.c >>>>>> index 06370b6..b04aa80 100644 >>>>>> --- a/hw/omap1.c >>>>>> +++ b/hw/omap1.c >>>>>> @@ -3675,7 +3675,7 @@ static int omap_validate_emiff_addr(struct >>>>>> omap_mpu_state_s *s, >>>>>> static int omap_validate_emifs_addr(struct omap_mpu_state_s *s, >>>>>> target_phys_addr_t addr) >>>>>> { >>>>>> - return addr >= OMAP_EMIFS_BASE && addr < OMAP_EMIFF_BASE; >>>>>> + return addr < OMAP_EMIFF_BASE; >>>>> >>>>> I think this only doesn't break the code because it relies on a >>>>> specific value for that #define, and if the value is changed, the >>>>> check has to be re-added. Same applies to other modifications in this >>>>> patch. >>>> >>>> I think you mean other modifications like this, that is: this change >>>> and the pxa2xx.c one. I'll try to invent a better solution. >>> >>> Well the other ones are probably just pointless, not wrong. For >>> example the first change in this patch really makes you wonder, until >>> you think of the famous gcc warnings. There can't be any better >>> reason for such a change. >> >> In the unsigned number space, the checks can be merged into one, >> assuming that BLKDBG_EVEN_MAX is less than INT_MAX. Alternatively we >> could have: >> - if (event < 0 || event >= BLKDBG_EVENT_MAX) { >> + if ((int)event < 0 || event >= BLKDBG_EVENT_MAX) { >> >> This would also implement the check that the writer of this code was >> trying to make. >> >> The important thing to note is however is that the check as it is now >> is not correct. > > 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 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. 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. > You know the size of the buffer, but there is no risk of an overflow. > Yet, some projects prohibit code like this. Using pstrcpy, pstrcat, > snprintf unavoidably add the overhead of looping over the input to > find the length again. Only if you don't store the length. > On other ocasions your input buffer and output buffer both have > constant length and if the input is smaller, then there's no need to > truncate. On other occasions you want to return an error if the input > is too long. pstrcpy is used when the input should be truncated in > case it's too long, and no error returned. I don't see why you wouldn't want to use pstrcpy even for the error check case given the small additional overhead. Anyway, this has very little to do with the patch.
Hi, On 4 September 2010 21:45, Blue Swirl <blauwirbel@gmail.com> wrote: > On Sat, Sep 4, 2010 at 5:57 PM, andrzej zaborowski <balrogg@gmail.com> 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
On Sat, Sep 4, 2010 at 8:30 PM, andrzej zaborowski <balrogg@gmail.com> wrote: > Hi, > > On 4 September 2010 21:45, Blue Swirl <blauwirbel@gmail.com> wrote: >> On Sat, Sep 4, 2010 at 5:57 PM, andrzej zaborowski <balrogg@gmail.com> 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. No. If BLKDBG_EVENT_MAX is < 0x80000000, it does not matter if there is the check or not. Because of unsigned arithmetic, only one comparison is enough. With a non-GCC compiler (or if there were negative enum items), the check may have the desired effect, just like with int cast. If BLKDBG_EVENT_MAX is >= 0x80000000, the first check is wrong, because you want to allow array access beyond 0x80000000, which will be blocked by the first check. A non-GCC compiler may actually reject an enum bigger than 0x80000000. Then the checks should be rewritten. The version with int cast is correct in more cases than the version which relies on enum signedness. > >> >> 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. void pstrcpy(char *buf, int buf_size, const char *str) { int c; char *q = buf; if (buf_size <= 0) return; for(;;) { c = *str++; if (c == 0 || q >= buf + buf_size - 1) break; *q++ = c; } *q = '\0'; } There is one extra check, q >= buf + buf_size - 1. No strlen(). 0000000000000000 <pstrcpy>: 0: 48 83 ec 18 sub $0x18,%rsp 4: 64 48 8b 04 25 28 00 mov %fs:0x28,%rax b: 00 00 d: 48 89 44 24 10 mov %rax,0x10(%rsp) 12: 31 c0 xor %eax,%eax 14: 85 f6 test %esi,%esi 16: 7e 39 jle 51 <pstrcpy+0x51> 18: 0f b6 0a movzbl (%rdx),%ecx 1b: 84 c9 test %cl,%cl 1d: 74 2f je 4e <pstrcpy+0x4e> 1f: 48 63 c6 movslq %esi,%rax 22: 48 8d 44 07 ff lea -0x1(%rdi,%rax,1),%rax 27: 48 39 c7 cmp %rax,%rdi 2a: 73 22 jae 4e <pstrcpy+0x4e> 2c: 48 83 c2 01 add $0x1,%rdx 30: eb 0b jmp 3d <pstrcpy+0x3d> 32: 66 0f 1f 44 00 00 nopw 0x0(%rax,%rax,1) The copying loop follows until 4c: 38: 48 39 c7 cmp %rax,%rdi 3b: 73 11 jae 4e <pstrcpy+0x4e> 3d: 88 0f mov %cl,(%rdi) 3f: 0f b6 0a movzbl (%rdx),%ecx 42: 48 83 c7 01 add $0x1,%rdi 46: 48 83 c2 01 add $0x1,%rdx 4a: 84 c9 test %cl,%cl 4c: 75 ea jne 38 <pstrcpy+0x38> 4e: c6 07 00 movb $0x0,(%rdi) 51: 48 8b 44 24 10 mov 0x10(%rsp),%rax 56: 64 48 33 04 25 28 00 xor %fs:0x28,%rax 5d: 00 00 5f: 75 05 jne 66 <pstrcpy+0x66> 61: 48 83 c4 18 add $0x18,%rsp 65: c3 retq 66: 66 90 xchg %ax,%ax 68: e8 00 00 00 00 callq 6d <pstrcpy+0x6d> 6d: 0f 1f 00 nopl (%rax) The extra check in the loop adds the instructions at 38 and 3b. Those are not memory accesses, so the cost should be marginal. > 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. The 'segfault' can be much, much worse: http://cve.mitre.org/cgi-bin/cvekey.cgi?keyword=buffer+overflow It's true that truncation can also cause bugs without proper checks, but still there shouldn't be a direct route to 'allow remote attackers to execute arbitrary code'.
On Sat, 4 Sep 2010, Blue Swirl wrote: > On Sat, Sep 4, 2010 at 5:57 PM, andrzej zaborowski <balrogg@gmail.com> wrote: > > On 4 September 2010 19:21, Blue Swirl <blauwirbel@gmail.com> wrote: > >> On Sat, Sep 4, 2010 at 4:44 PM, andrzej zaborowski <balrogg@gmail.com> wrote: > >>> On 4 September 2010 18:14, Blue Swirl <blauwirbel@gmail.com> wrote: > >>>> On Sat, Sep 4, 2010 at 3:40 PM, andrzej zaborowski <balrogg@gmail.com> wrote: > >>>>> On 4 September 2010 16:17, Blue Swirl <blauwirbel@gmail.com> wrote: > >>>>>> Add various casts, adjust types etc. to make the warnings with > >>>>>> gcc flag -Wtype-limits disappear. [..snip..] > 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 > > 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. > > 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. > While enumerated types are indeed implementation defined (6.7.2.2), enumeration constants themselves are of type int (6.4.4.3), so all one has to do is not to declare variables with enumeration type but use plain int instead (Which i guess goes against the desire to be gentle with the reader of the code) [..snip..]
On Sat, Sep 04, 2010 at 05:21:24PM +0000, Blue Swirl wrote: > In the unsigned number space, the checks can be merged into one, > assuming that BLKDBG_EVEN_MAX is less than INT_MAX. Alternatively we > could have: > - if (event < 0 || event >= BLKDBG_EVENT_MAX) { > + if ((int)event < 0 || event >= BLKDBG_EVENT_MAX) { > > This would also implement the check that the writer of this code was > trying to make. > The important thing to note is however is that the check as it is now > is not correct. I agree. But it seems to indicate a bigger problem. If we are trying to pass in a negative value, which is not one of enum values, using BlkDebugEvent as type is just confusing, we should just pass int instead. > >> How about adding assert(OMAP_EMIFS_BASE == 0) and commenting out the > >> check? Then if the value changes, the need to add the comparison back > >> will be obvious. > > > > This would work but it's weird. The thing is it's currently a correct > > code and the check may be useless but it's the optimiser's task to > > remove it, not ours. The compiler is not able to tell whether the > > check makes sense or nott, because the compiler only has access to > > preprocessed code. So why should you let the compiler have anything > > to say on it. > > Good point. I'll try to invent something better. Use #pragma to supress the warning? Maybe we could wrap this in a macro ..
On Sun, Sep 5, 2010 at 7:54 AM, Michael S. Tsirkin <mst@redhat.com> wrote: > On Sat, Sep 04, 2010 at 05:21:24PM +0000, Blue Swirl wrote: >> In the unsigned number space, the checks can be merged into one, >> assuming that BLKDBG_EVEN_MAX is less than INT_MAX. Alternatively we >> could have: >> - if (event < 0 || event >= BLKDBG_EVENT_MAX) { >> + if ((int)event < 0 || event >= BLKDBG_EVENT_MAX) { >> >> This would also implement the check that the writer of this code was >> trying to make. >> The important thing to note is however is that the check as it is now >> is not correct. > > I agree. But it seems to indicate a bigger problem. > > If we are trying to pass in a negative value, which is not one > of enum values, using BlkDebugEvent as type is just confusing, > we should just pass int instead. AFAICT it's only possible to use the values listed in event_names in blkdebug.c, other values are rejected. So the check should actually be an assert() or it could even be removed. >> >> How about adding assert(OMAP_EMIFS_BASE == 0) and commenting out the >> >> check? Then if the value changes, the need to add the comparison back >> >> will be obvious. >> > >> > This would work but it's weird. The thing is it's currently a correct >> > code and the check may be useless but it's the optimiser's task to >> > remove it, not ours. The compiler is not able to tell whether the >> > check makes sense or nott, because the compiler only has access to >> > preprocessed code. So why should you let the compiler have anything >> > to say on it. >> >> Good point. I'll try to invent something better. > > Use #pragma to supress the warning? Maybe we could wrap this in a macro .. Those lines may also desynch silently with changes to OMAP_EMIFS_BASE. I think the assertion is still the best way, it ensures that something will happen if OMAP_EMIFS_BASE changes. We could for example remove OMAP_EMIFS_BASE entirely (it's only used for the check), but someone adding a new define could still forget to adjust the check anyway.
On Sun, Sep 05, 2010 at 09:06:10AM +0000, Blue Swirl wrote: > On Sun, Sep 5, 2010 at 7:54 AM, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Sat, Sep 04, 2010 at 05:21:24PM +0000, Blue Swirl wrote: > >> In the unsigned number space, the checks can be merged into one, > >> assuming that BLKDBG_EVEN_MAX is less than INT_MAX. Alternatively we > >> could have: > >> - if (event < 0 || event >= BLKDBG_EVENT_MAX) { > >> + if ((int)event < 0 || event >= BLKDBG_EVENT_MAX) { > >> > >> This would also implement the check that the writer of this code was > >> trying to make. > >> The important thing to note is however is that the check as it is now > >> is not correct. > > > > I agree. But it seems to indicate a bigger problem. > > > > If we are trying to pass in a negative value, which is not one > > of enum values, using BlkDebugEvent as type is just confusing, > > we should just pass int instead. > > AFAICT it's only possible to use the values listed in event_names in > blkdebug.c, other values are rejected. So the check should actually be > an assert() or it could even be removed. Sounds good. > >> >> How about adding assert(OMAP_EMIFS_BASE == 0) and commenting out the > >> >> check? Then if the value changes, the need to add the comparison back > >> >> will be obvious. > >> > > >> > This would work but it's weird. The thing is it's currently a correct > >> > code and the check may be useless but it's the optimiser's task to > >> > remove it, not ours. The compiler is not able to tell whether the > >> > check makes sense or nott, because the compiler only has access to > >> > preprocessed code. So why should you let the compiler have anything > >> > to say on it. > >> > >> Good point. I'll try to invent something better. > > > > Use #pragma to supress the warning? Maybe we could wrap this in a macro .. > > Those lines may also desynch silently with changes to OMAP_EMIFS_BASE. > > I think the assertion is still the best way, it ensures that something > will happen if OMAP_EMIFS_BASE changes. We could for example remove > OMAP_EMIFS_BASE entirely (it's only used for the check), but someone > adding a new define could still forget to adjust the check anyway. We could replace it with a macro #define OMAP_EMIFS_VALID(addr) ((target_phys_addr_t)addr < OMAP_EMIFF_BASE) but all this does look artificial. And of course using type casts is always scary ... Would it help to have some inline functions that do the range checking correctly? We have a couple of range helpers in pci.h, these could be moved out to range.h and we could add some more. As there act on u64 this will get the type limits mostly automatically right.
Am 04.09.2010 23:07, schrieb Blue Swirl: > On Sat, Sep 4, 2010 at 8:30 PM, andrzej zaborowski <balrogg@gmail.com> wrote: >> Hi, >> >> On 4 September 2010 21:45, Blue Swirl <blauwirbel@gmail.com> wrote: >>> On Sat, Sep 4, 2010 at 5:57 PM, andrzej zaborowski <balrogg@gmail.com> 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. > > No. > > If BLKDBG_EVENT_MAX is < 0x80000000, it does not matter if there is > the check or not. Because of unsigned arithmetic, only one comparison > is enough. With a non-GCC compiler (or if there were negative enum > items), the check may have the desired effect, just like with int > cast. > If BLKDBG_EVENT_MAX is >= 0x80000000, the first check is wrong, > because you want to allow array access beyond 0x80000000, which will > be blocked by the first check. A non-GCC compiler may actually reject > an enum bigger than 0x80000000. Then the checks should be rewritten. > > The version with int cast is correct in more cases than the version > which relies on enum signedness. If the value isn't explicitly specified, it's defined to start at 0 and increment by 1 for each enum constant. So it's very unlikely to hit an interesting case - we would have to add some billions of events first. And isn't it the int cast that would lead to (event < 0) == true if BLKDBG_EVENT_MAX was >= 0x8000000 and falsely return an error? The old version should do this right. Anyway, even though this specific code shouldn't misbehave today, I'm all for silencing warnings and enabling -Wtype-limits. We regularly have real bugs of this type. Kevin
On Mon, Sep 6, 2010 at 1:04 PM, Kevin Wolf <kwolf@redhat.com> wrote: > Am 04.09.2010 23:07, schrieb Blue Swirl: >> On Sat, Sep 4, 2010 at 8:30 PM, andrzej zaborowski <balrogg@gmail.com> wrote: >>> Hi, >>> >>> On 4 September 2010 21:45, Blue Swirl <blauwirbel@gmail.com> wrote: >>>> On Sat, Sep 4, 2010 at 5:57 PM, andrzej zaborowski <balrogg@gmail.com> 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. >> >> No. >> >> If BLKDBG_EVENT_MAX is < 0x80000000, it does not matter if there is >> the check or not. Because of unsigned arithmetic, only one comparison >> is enough. With a non-GCC compiler (or if there were negative enum >> items), the check may have the desired effect, just like with int >> cast. >> If BLKDBG_EVENT_MAX is >= 0x80000000, the first check is wrong, >> because you want to allow array access beyond 0x80000000, which will >> be blocked by the first check. A non-GCC compiler may actually reject >> an enum bigger than 0x80000000. Then the checks should be rewritten. >> >> The version with int cast is correct in more cases than the version >> which relies on enum signedness. > > If the value isn't explicitly specified, it's defined to start at 0 and > increment by 1 for each enum constant. So it's very unlikely to hit an > interesting case - we would have to add some billions of events first. The discussion about BLKDBG_EVENT_MAX being absurdly high value was theoretical. In reality, even the check for < 0 is useless at the moment, since those values can't be produced. It may be useful to catch internal inconsistencies later if the code changes. > And isn't it the int cast that would lead to (event < 0) == true if > BLKDBG_EVENT_MAX was >= 0x8000000 and falsely return an error? The old > version should do this right. No, because if you want to allow event >= 0x80000000, you shouldn't also check for event < 0 when using 32 bit integers on modern hardware. > Anyway, even though this specific code shouldn't misbehave today, I'm > all for silencing warnings and enabling -Wtype-limits. We regularly have > real bugs of this type. Thank you!
diff --git a/block/blkdebug.c b/block/blkdebug.c index 2a63df9..b5083bc 100644 --- a/block/blkdebug.c +++ b/block/blkdebug.c @@ -439,7 +439,7 @@ static void blkdebug_debug_event(BlockDriverState *bs, BlkDebugEvent event) struct BlkdebugRule *rule; BlkdebugVars old_vars = s->vars; - if (event < 0 || event >= BLKDBG_EVENT_MAX) { + if ((unsigned int)event >= BLKDBG_EVENT_MAX) { return; } diff --git a/hw/mips_fulong2e.c b/hw/mips_fulong2e.c index cbe7156..ac82067 100644 --- a/hw/mips_fulong2e.c +++ b/hw/mips_fulong2e.c @@ -258,7 +258,7 @@ static void mips_fulong2e_init(ram_addr_t ram_size, const char *boot_device, { char *filename; unsigned long ram_offset, bios_offset; - unsigned long bios_size; + long bios_size; int64_t kernel_entry; qemu_irq *i8259; qemu_irq *cpu_exit_irq; diff --git a/hw/omap1.c b/hw/omap1.c index 06370b6..b04aa80 100644 --- a/hw/omap1.c +++ b/hw/omap1.c @@ -3675,7 +3675,7 @@ static int omap_validate_emiff_addr(struct omap_mpu_state_s *s, static int omap_validate_emifs_addr(struct omap_mpu_state_s *s, target_phys_addr_t addr) { - return addr >= OMAP_EMIFS_BASE && addr < OMAP_EMIFF_BASE; + return addr < OMAP_EMIFF_BASE; } static int omap_validate_imif_addr(struct omap_mpu_state_s *s, diff --git a/hw/ppc405_boards.c b/hw/ppc405_boards.c index 662d7c4..db8e5ec 100644 --- a/hw/ppc405_boards.c +++ b/hw/ppc405_boards.c @@ -182,10 +182,12 @@ static void ref405ep_init (ram_addr_t ram_size, qemu_irq *pic; ram_addr_t sram_offset, bios_offset, bdloc; target_phys_addr_t ram_bases[2], ram_sizes[2]; - target_ulong sram_size, bios_size; + target_ulong sram_size; + long bios_size; //int phy_addr = 0; //static int phy_addr = 1; - target_ulong kernel_base, kernel_size, initrd_base, initrd_size; + target_ulong kernel_base, initrd_base; + long kernel_size, initrd_size; int linux_boot; int fl_idx, fl_sectors, len; DriveInfo *dinfo; @@ -221,8 +223,8 @@ static void ref405ep_init (ram_addr_t ram_size, bios_offset = qemu_ram_alloc(NULL, "ef405ep.bios", bios_size); fl_sectors = (bios_size + 65535) >> 16; #ifdef DEBUG_BOARD_INIT - printf("Register parallel flash %d size " TARGET_FMT_lx - " at offset %08lx addr " TARGET_FMT_lx " '%s' %d\n", + printf("Register parallel flash %d size %lx" + " at offset %08lx addr %lx '%s' %d\n", fl_idx, bios_size, bios_offset, -bios_size, bdrv_get_device_name(dinfo->bdrv), fl_sectors); #endif @@ -308,7 +310,7 @@ static void ref405ep_init (ram_addr_t ram_size, kernel_filename); exit(1); } - printf("Load kernel size " TARGET_FMT_ld " at " TARGET_FMT_lx, + printf("Load kernel size %ld at " TARGET_FMT_lx, kernel_size, kernel_base); /* load initrd */ if (initrd_filename) { @@ -503,8 +505,9 @@ static void taihu_405ep_init(ram_addr_t ram_size, qemu_irq *pic; ram_addr_t bios_offset; target_phys_addr_t ram_bases[2], ram_sizes[2]; - target_ulong bios_size; - target_ulong kernel_base, kernel_size, initrd_base, initrd_size; + long bios_size; + target_ulong kernel_base, initrd_base; + long kernel_size, initrd_size; int linux_boot; int fl_idx, fl_sectors; DriveInfo *dinfo; @@ -534,8 +537,8 @@ static void taihu_405ep_init(ram_addr_t ram_size, fl_sectors = (bios_size + 65535) >> 16; bios_offset = qemu_ram_alloc(NULL, "taihu_405ep.bios", bios_size); #ifdef DEBUG_BOARD_INIT - printf("Register parallel flash %d size " TARGET_FMT_lx - " at offset %08lx addr " TARGET_FMT_lx " '%s' %d\n", + printf("Register parallel flash %d size %lx" + " at offset %08lx addr %lx '%s' %d\n", fl_idx, bios_size, bios_offset, -bios_size, bdrv_get_device_name(dinfo->bdrv), fl_sectors); #endif @@ -576,7 +579,7 @@ static void taihu_405ep_init(ram_addr_t ram_size, bios_size = 32 * 1024 * 1024; fl_sectors = (bios_size + 65535) >> 16; #ifdef DEBUG_BOARD_INIT - printf("Register parallel flash %d size " TARGET_FMT_lx + printf("Register parallel flash %d size %lx" " at offset %08lx addr " TARGET_FMT_lx " '%s'\n", fl_idx, bios_size, bios_offset, (target_ulong)0xfc000000, bdrv_get_device_name(dinfo->bdrv)); diff --git a/hw/ppc_newworld.c b/hw/ppc_newworld.c index 809a1cf..fb07c83 100644 --- a/hw/ppc_newworld.c +++ b/hw/ppc_newworld.c @@ -135,7 +135,8 @@ static void ppc_core99_init (ram_addr_t ram_size, int unin_memory; int linux_boot, i; ram_addr_t ram_offset, bios_offset, vga_bios_offset; - uint32_t kernel_base, kernel_size, initrd_base, initrd_size; + uint32_t kernel_base, initrd_base; + long kernel_size, initrd_size; PCIBus *pci_bus; MacIONVRAMState *nvr; int nvram_mem_index; diff --git a/hw/ppc_prep.c b/hw/ppc_prep.c index 52fa9b6..0e5b88c 100644 --- a/hw/ppc_prep.c +++ b/hw/ppc_prep.c @@ -572,7 +572,8 @@ static void ppc_prep_init (ram_addr_t ram_size, int PPC_io_memory; int linux_boot, i, nb_nics1, bios_size; ram_addr_t ram_offset, bios_offset; - uint32_t kernel_base, kernel_size, initrd_base, initrd_size; + uint32_t kernel_base, initrd_base; + long kernel_size, initrd_size; PCIBus *pci_bus; qemu_irq *i8259; qemu_irq *cpu_exit_irq; diff --git a/hw/pxa2xx.c b/hw/pxa2xx.c index 26b9205..3c06bf9 100644 --- a/hw/pxa2xx.c +++ b/hw/pxa2xx.c @@ -125,7 +125,7 @@ static void pxa2xx_pm_write(void *opaque, target_phys_addr_t addr, break; default: /* Read-write registers */ - if (addr >= PMCR && addr <= PCMD31 && !(addr & 3)) { + if (addr <= PCMD31 && !(addr & 3)) { s->pm_regs[addr >> 2] = value; break; } diff --git a/hw/sm501.c b/hw/sm501.c index 8e6932d..1eb6b63 100644 --- a/hw/sm501.c +++ b/hw/sm501.c @@ -814,7 +814,7 @@ static uint32_t sm501_palette_read(void *opaque, target_phys_addr_t addr) /* TODO : consider BYTE/WORD access */ /* TODO : consider endian */ - assert(0 <= addr && addr < 0x400 * 3); + assert(addr < 0x400 * 3); return *(uint32_t*)&s->dc_palette[addr]; } @@ -828,7 +828,7 @@ static void sm501_palette_write(void *opaque, /* TODO : consider BYTE/WORD access */ /* TODO : consider endian */ - assert(0 <= addr && addr < 0x400 * 3); + assert(addr < 0x400 * 3); *(uint32_t*)&s->dc_palette[addr] = value; } diff --git a/linux-user/flatload.c b/linux-user/flatload.c index 8ad130a..8f9f4a5 100644 --- a/linux-user/flatload.c +++ b/linux-user/flatload.c @@ -383,7 +383,8 @@ static int load_flat_file(struct linux_binprm * bprm, struct lib_info *libinfo, int id, abi_ulong *extra_stack) { struct flat_hdr * hdr; - abi_ulong textpos = 0, datapos = 0, result; + abi_ulong textpos = 0, datapos = 0; + abi_long result; abi_ulong realdatastart = 0; abi_ulong text_len, data_len, bss_len, stack_len, flags; abi_ulong memp = 0; /* for finding the brk area */ diff --git a/linux-user/mmap.c b/linux-user/mmap.c index e10a6ef..67d020d 100644 --- a/linux-user/mmap.c +++ b/linux-user/mmap.c @@ -342,7 +342,7 @@ abi_ulong mmap_find_vma(abi_ulong start, abi_ulong size) munmap(ptr, size); /* ENOMEM if we checked the whole of the target address space. */ - if (addr == -1ul) { + if (addr == (abi_ulong)-1ul) { return (abi_ulong)-1; } else if (addr == 0) { if (wrapped) { diff --git a/linux-user/syscall.c b/linux-user/syscall.c index 0ebe7e1..d44f512 100644 --- a/linux-user/syscall.c +++ b/linux-user/syscall.c @@ -1551,8 +1551,9 @@ static abi_long do_bind(int sockfd, abi_ulong target_addr, void *addr; abi_long ret; - if (addrlen < 0) + if ((int)addrlen < 0) { return -TARGET_EINVAL; + } addr = alloca(addrlen+1); @@ -1570,8 +1571,9 @@ static abi_long do_connect(int sockfd, abi_ulong target_addr, void *addr; abi_long ret; - if (addrlen < 0) + if ((int)addrlen < 0) { return -TARGET_EINVAL; + } addr = alloca(addrlen);
Add various casts, adjust types etc. to make the warnings with gcc flag -Wtype-limits disappear. Signed-off-by: Blue Swirl <blauwirbel@gmail.com> --- block/blkdebug.c | 2 +- hw/mips_fulong2e.c | 2 +- hw/omap1.c | 2 +- hw/ppc405_boards.c | 23 +++++++++++++---------- hw/ppc_newworld.c | 3 ++- hw/ppc_prep.c | 3 ++- hw/pxa2xx.c | 2 +- hw/sm501.c | 4 ++-- linux-user/flatload.c | 3 ++- linux-user/mmap.c | 2 +- linux-user/syscall.c | 20 +++++++++++++------- 11 files changed, 39 insertions(+), 27 deletions(-) @@ -1656,8 +1658,9 @@ static abi_long do_accept(int fd, abi_ulong target_addr, if (get_user_u32(addrlen, target_addrlen_addr)) return -TARGET_EINVAL; - if (addrlen < 0) + if ((int)addrlen < 0) { return -TARGET_EINVAL; + } if (!access_ok(VERIFY_WRITE, target_addr, addrlen)) return -TARGET_EINVAL; @@ -1684,8 +1687,9 @@ static abi_long do_getpeername(int fd, abi_ulong target_addr, if (get_user_u32(addrlen, target_addrlen_addr)) return -TARGET_EFAULT; - if (addrlen < 0) + if ((int)addrlen < 0) { return -TARGET_EINVAL; + } if (!access_ok(VERIFY_WRITE, target_addr, addrlen)) return -TARGET_EFAULT; @@ -1712,8 +1716,9 @@ static abi_long do_getsockname(int fd, abi_ulong target_addr, if (get_user_u32(addrlen, target_addrlen_addr)) return -TARGET_EFAULT; - if (addrlen < 0) + if ((int)addrlen < 0) { return -TARGET_EINVAL; + } if (!access_ok(VERIFY_WRITE, target_addr, addrlen)) return -TARGET_EFAULT; @@ -1753,8 +1758,9 @@ static abi_long do_sendto(int fd, abi_ulong msg, size_t len, int flags, void *host_msg; abi_long ret; - if (addrlen < 0) + if ((int)addrlen < 0) { return -TARGET_EINVAL; + } host_msg = lock_user(VERIFY_READ, msg, len, 1); if (!host_msg) @@ -1792,7 +1798,7 @@ static abi_long do_recvfrom(int fd, abi_ulong msg, size_t len, int flags, ret = -TARGET_EFAULT; goto fail; } - if (addrlen < 0) { + if ((int)addrlen < 0) { ret = -TARGET_EINVAL; goto fail; }