Re: [Qemu-devel] [PATCH 02/10] qapi: fail hard on stack imbalance

[Michael Roth]

On Mon, Mar 26, 2012 at 11:15:46AM -0300, Luiz Capitulino wrote:
> On Thu, 22 Mar 2012 12:51:04 +0100
> Paolo Bonzini <address@hidden> wrote:
> 
> > QmpOutputVisitor will segfault if an imbalanced end function is
> > called.  So we can abort in QmpInputVisitor too.
> > 
> > Signed-off-by: Paolo Bonzini <address@hidden>
> > ---
> >  qapi/qmp-input-visitor.c |    5 +----
> >  1 files changed, 1 insertions(+), 4 deletions(-)
> > 
> > diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
> > index e6b6152..b4013cc 100644
> > --- a/qapi/qmp-input-visitor.c
> > +++ b/qapi/qmp-input-visitor.c
> > @@ -77,11 +77,8 @@ static void qmp_input_push(QmpInputVisitor *qiv, const 
> > QObject *obj, Error **err
> >  
> >  static void qmp_input_pop(QmpInputVisitor *qiv, Error **errp)
> >  {
> > +    assert(qiv->nb_stack > 0);
> >      qiv->nb_stack--;
> > -    if (qiv->nb_stack < 0) {
> > -        error_set(errp, QERR_BUFFER_OVERRUN);
> > -        return;
> > -    }
> >  }
> 
> Just to confirm: this can't be triggered by malicious clients, right?
> 
> The original series submitted by Michael had this, but I asked him to
> change because I thought clients could trigger it. But by reading the code
> now it seems to me that the end_struct() function is only generated by types
> we know about.
> 

Yah, looked into this as well. I can't see anything outside of a
bug triggering this: even if they did manage to get past the parser and
have some wonky qobject fed to us, the generated visitor code should
still always be balanced, and end_list/end_struct are the only way to
induce a pop so nb_stack should never go below 0. There may be other areas
where we need to watch out for this kind of stuff though.

> >  
> >  static void qmp_input_start_struct(Visitor *v, void **obj, const char 
> > *kind,
> 
>