[Spice-devel] [PATCH v6 4/5] vdagentd: early return on bad message size

[Michal Suchánek]

On Wed, 25 Jan 2017 08:24:55 +0100
Christophe Fergeau <cfergeau at redhat.com> wrote:

> On Tue, Jan 24, 2017 at 07:52:11PM +0100, Michal Suchánek wrote:
> > On Tue, 24 Jan 2017 09:45:37 +0100
> > Christophe Fergeau <cfergeau at redhat.com> wrote:  
> > > 
> > > In this big switch, VD_AGENT_FILE_XFER_* and
> > > VD_AGENT_CLIENT_DISCONNECTED were previously not subject to
> > > a size check, VD_AGENT_DISPLAY_CONFIG and VD_AGENT_REPLY did not
> > > appear in the switch, and  
> > 
> > They appear in the min_size array, however. So they should be
> > handled. This is now a separate size check function that can be
> > used to check check size of any message, even messages the
> > dispatcher does not handle.
> > 
> > Also the XFER messages need to be size checked later when they are
> > swapped. Actually the fields of the messages are accessed already
> > so it is an error to not check they are actually present.
> >   
> > > VD_AGENT_CLIPBOARD_RELEASE/VD_AGENT_CLIPBOARD_RELEASE were doing
> > > a < comparison, not a !=. I'm not necessarily opposed to these
> > > changes, but I'd keep them for an additional commit, or at least
> > > explain why this is ok to do in the commit log.  
> > 
> > This is definitely because they were lumped together with the
> > variable length clipboard messages.  
> 
> Yeah, as I said, I don't think it's wrong to have them there, but I'd
> add them in a separate message, so that there's a mostly mechanical
> commit which adds the new function, but is strictly equivalent to the
> old code in terms of what is tested and how it's tested. Then once you
> have the function, you can refine things, add missing enum values,
> refine the <= vs != tests, ...

There was never a point when the code was equivalent after removing the
size checks from the dispatch switch and putting them into a separate
piece of code.

There were unintentional errors in the first attempt taking out the
pieces one by one and collecting them elsewhere.

So here I just implement new separate check which is based on the
headers which define the messages as variable or fixed size and remove
the old checks which were based on historical evolution and were
obviously more or less incorrect in places.

There was never an intent to make to code bug to bug equivalent.

Thanks

Michal
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 473 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170127/9540514a/attachment.sig>