vt82c686: fix coverity warning about out-of-bounds write

Refactor superio_ioport_writeb to fix the out of bounds write warning.

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
---
 hw/isa/vt82c686.c | 39 ++++++++++++++++++---------------------
 1 file changed, 18 insertions(+), 21 deletions(-)

Am 09.12.2014 um 06:44 schrieb zhanghailiang:
> Refactor superio_ioport_writeb to fix the out of bounds write warning.
> 
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> ---
>  hw/isa/vt82c686.c | 39 ++++++++++++++++++---------------------
>  1 file changed, 18 insertions(+), 21 deletions(-)
> 
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index e0c235c..4516af0 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -50,13 +50,13 @@ typedef struct VT82C686BState {
>  static void superio_ioport_writeb(void *opaque, hwaddr addr, uint64_t data,
>                                    unsigned size)
>  {
> -    int can_write;
>      SuperIOConfig *superio_conf = opaque;
>  
>      DPRINTF("superio_ioport_writeb  address 0x%x  val 0x%x\n", addr, data);
>      if (addr == 0x3f0) {
>          superio_conf->index = data & 0xff;
>      } else {
> +        int can_write = 1;


IMHO using bool instead of int would be better here (and false, true in
the following code).

>          /* 0x3f1 */
>          switch (superio_conf->index) {
>          case 0x00 ... 0xdf:
> @@ -70,28 +70,25 @@ static void superio_ioport_writeb(void *opaque, hwaddr addr, uint64_t data,
>          case 0xfd ... 0xff:
>              can_write = 0;
>              break;
> -        default:
> -            can_write = 1;
> -
> -            if (can_write) {
> -                switch (superio_conf->index) {
> -                case 0xe7:
> -                    if ((data & 0xff) != 0xfe) {
> -                        DPRINTF("chage uart 1 base. unsupported yet\n");
> -                    }
> -                    break;
> -                case 0xe8:
> -                    if ((data & 0xff) != 0xbe) {
> -                        DPRINTF("chage uart 2 base. unsupported yet\n");
> -                    }
> -                    break;
> -
> -                default:
> -                    superio_conf->config[superio_conf->index] = data & 0xff;
> -                }
> +        case 0xe7:
> +            if ((data & 0xff) != 0xfe) {
> +                DPRINTF("chage uart 1 base. unsupported yet\n");

This text looks strange. Maybe a typo?

> +                can_write = 0;
> +            }
> +            break;
> +        case 0xe8:
> +            if ((data & 0xff) != 0xbe) {
> +                DPRINTF("chage uart 2 base. unsupported yet\n");

This text looks strange. Maybe a typo?

> +                can_write = 0;
>              }
> +            break;
> +        default:
> +            break;
> +
> +        }
> +        if (can_write) {
> +            superio_conf->config[superio_conf->index] = data & 0xff;
>          }
> -        superio_conf->config[superio_conf->index] = data & 0xff;
>      }
>  }


Regards
Stefan

On 2014/12/9 14:04, Stefan Weil wrote:
> Am 09.12.2014 um 06:44 schrieb zhanghailiang:
>> Refactor superio_ioport_writeb to fix the out of bounds write warning.
>>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> ---
>>   hw/isa/vt82c686.c | 39 ++++++++++++++++++---------------------
>>   1 file changed, 18 insertions(+), 21 deletions(-)
>>
>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>> index e0c235c..4516af0 100644
>> --- a/hw/isa/vt82c686.c
>> +++ b/hw/isa/vt82c686.c
>> @@ -50,13 +50,13 @@ typedef struct VT82C686BState {
>>   static void superio_ioport_writeb(void *opaque, hwaddr addr, uint64_t data,
>>                                     unsigned size)
>>   {
>> -    int can_write;
>>       SuperIOConfig *superio_conf = opaque;
>>
>>       DPRINTF("superio_ioport_writeb  address 0x%x  val 0x%x\n", addr, data);
>>       if (addr == 0x3f0) {
>>           superio_conf->index = data & 0xff;
>>       } else {
>> +        int can_write = 1;
>
>
> IMHO using bool instead of int would be better here (and false, true in
> the following code).

OK, will fix in v2. Thanks.

>
>>           /* 0x3f1 */
>>           switch (superio_conf->index) {
>>           case 0x00 ... 0xdf:
>> @@ -70,28 +70,25 @@ static void superio_ioport_writeb(void *opaque, hwaddr addr, uint64_t data,
>>           case 0xfd ... 0xff:
>>               can_write = 0;
>>               break;
>> -        default:
>> -            can_write = 1;
>> -
>> -            if (can_write) {
>> -                switch (superio_conf->index) {
>> -                case 0xe7:
>> -                    if ((data & 0xff) != 0xfe) {
>> -                        DPRINTF("chage uart 1 base. unsupported yet\n");
>> -                    }
>> -                    break;
>> -                case 0xe8:
>> -                    if ((data & 0xff) != 0xbe) {
>> -                        DPRINTF("chage uart 2 base. unsupported yet\n");
>> -                    }
>> -                    break;
>> -
>> -                default:
>> -                    superio_conf->config[superio_conf->index] = data & 0xff;
>> -                }
>> +        case 0xe7:
>> +            if ((data & 0xff) != 0xfe) {
>> +                DPRINTF("chage uart 1 base. unsupported yet\n");
>
> This text looks strange. Maybe a typo?
>

Ah, Good catch, a typo, should be 'change'. will fix in v2, Thanks ;)

>> +                can_write = 0;
>> +            }
>> +            break;
>> +        case 0xe8:
>> +            if ((data & 0xff) != 0xbe) {
>> +                DPRINTF("chage uart 2 base. unsupported yet\n");
>
> This text looks strange. Maybe a typo?


>> +                can_write = 0;
>>               }
>> +            break;
>> +        default:
>> +            break;
>> +
>> +        }
>> +        if (can_write) {
>> +            superio_conf->config[superio_conf->index] = data & 0xff;
>>           }
>> -        superio_conf->config[superio_conf->index] = data & 0xff;
>>       }
>>   }
>
>
> Regards
> Stefan
>
>
> .
>