Re: [Qemu-devel] [PATCH] qapi: Fix crash with enum dealloc when kind is invalid

[Fam Zheng]

On Wed, 09/10 15:01, Paolo Bonzini wrote:
> Il 10/09/2014 14:30, Fam Zheng ha scritto:
> > We shouldn't do anything in the switch block in enum's visit_type_
> > function, when the enum data's ->kind is not valid at all. This happens
> > when the dealloc visitor is called, after qmp input visitor returned
> > error.
> > 
> > Now, the input visitor will set ->kind to <TYPE>_MAX if the value is not
> > found, so that in dealloc, the switch block knows to skip calling into
> > specific type's visiting functions.
> > 
> > The added test case would trigger SIGSEGV without this fix.
> > 
> > This crash is introduced since commit b1de5f43 (QMP: Add support for
> > Archipelago).
> > 
> > Signed-off-by: Fam Zheng <address@hidden>
> > ---
> >  qapi/qapi-visit-core.c     | 12 +++++-------
> >  scripts/qapi-visit.py      |  6 ++++++
> >  tests/qemu-iotests/087     | 17 +++++++++++++++++
> >  tests/qemu-iotests/087.out | 13 +++++++++++++
> >  4 files changed, 41 insertions(+), 7 deletions(-)
> > 
> > diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
> > index 55f8d40..6c46e0e 100644
> > --- a/qapi/qapi-visit-core.c
> > +++ b/qapi/qapi-visit-core.c
> > @@ -276,23 +276,21 @@ void input_type_enum(Visitor *v, int *obj, const char 
> > *strings[],
> >  
> >      visit_type_str(v, &enum_str, name, &local_err);
> >      if (local_err) {
> > -        error_propagate(errp, local_err);
> > -        return;
> > +        enum_str = NULL;
> >      }
> >  
> >      while (strings[value] != NULL) {
> > -        if (strcmp(strings[value], enum_str) == 0) {
> > +        if (enum_str && strcmp(strings[value], enum_str) == 0) {
> >              break;
> >          }
> >          value++;
> >      }
> >  
> > -    if (strings[value] == NULL) {
> > -        error_set(errp, QERR_INVALID_PARAMETER, enum_str);
> > -        g_free(enum_str);
> > -        return;
> > +    if (!local_err && strings[value] == NULL) {
> > +        error_set(&local_err, QERR_INVALID_PARAMETER, enum_str);
> >      }
> >  
> >      g_free(enum_str);
> >      *obj = value;
> > +    error_propagate(errp, local_err);
> >  }
> > diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> > index c129697..dad7561 100644
> > --- a/scripts/qapi-visit.py
> > +++ b/scripts/qapi-visit.py
> > @@ -379,6 +379,12 @@ void visit_type_%(name)s(Visitor *m, %(name)s **obj, 
> > const char *name, Error **e
> >                  c_name=c_fun(key))
> >  
> >      ret += mcgen('''
> > +        case %(enum_full_value)s:
> > +            break;
> > +''',
> > +                enum_full_value = generate_enum_full_value(disc_type, 
> > 'MAX'))
> > +
> > +    ret += mcgen('''
> >          default:
> >              abort();
> >          }
> > diff --git a/tests/qemu-iotests/087 b/tests/qemu-iotests/087
> > index 82c56b1..d7454d1 100755
> > --- a/tests/qemu-iotests/087
> > +++ b/tests/qemu-iotests/087
> > @@ -218,6 +218,23 @@ run_qemu <<EOF
> >  { "execute": "quit" }
> >  EOF
> >  
> > +echo
> > +echo === Missing driver ===
> > +echo
> > +
> > +_make_test_img -o encryption=on $size
> > +run_qemu -S <<EOF
> > +{ "execute": "qmp_capabilities" }
> > +{ "execute": "blockdev-add",
> > +  "arguments": {
> > +      "options": {
> > +        "id": "disk"
> > +      }
> > +    }
> > +  }
> > +{ "execute": "quit" }
> > +EOF
> > +
> >  # success, all done
> >  echo "*** done"
> >  rm -f $seq.full
> > diff --git a/tests/qemu-iotests/087.out b/tests/qemu-iotests/087.out
> > index 7fbee3f..f16bad0 100644
> > --- a/tests/qemu-iotests/087.out
> > +++ b/tests/qemu-iotests/087.out
> > @@ -64,4 +64,17 @@ QMP_VERSION
> >  {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, 
> > "event": "DEVICE_TRAY_MOVED", "data": {"device": "ide1-cd0", "tray-open": 
> > true}}
> >  {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, 
> > "event": "DEVICE_TRAY_MOVED", "data": {"device": "floppy0", "tray-open": 
> > true}}
> >  
> > +
> > +=== Missing driver ===
> > +
> > +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 encryption=on 
> > +Testing: -S
> > +QMP_VERSION
> > +{"return": {}}
> > +{"error": {"class": "GenericError", "desc": "Invalid parameter type for 
> > 'driver', expected: string"}}
> > +{"return": {}}
> > +{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, 
> > "event": "SHUTDOWN"}
> > +{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, 
> > "event": "DEVICE_TRAY_MOVED", "data": {"device": "ide1-cd0", "tray-open": 
> > true}}
> > +{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, 
> > "event": "DEVICE_TRAY_MOVED", "data": {"device": "floppy0", "tray-open": 
> > true}}
> > +
> >  *** done
> > 
> 
> A bit hackish, but I don't have any better idea.
> 
> Hmm... what about adding a new member to the visitors for "invalid enum"
> value?  The dealloc visitor could override it to do nothing, while the
> default could abort or set an error.  Would that work?
> 

The invalid state of enum still needs to be saved in the data.  It is detected
by the input visitor, but should be checked by other visitors (output, dealloc)
later.

Fam