Re: [Qemu-trivial] [Qemu-devel] [PATCH] vt82c686: fix coverity warning about out-of-bounds write

[zhanghailiang]

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 <address@hidden>
> > ---
> >   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
>
>
> .