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

[Michal Suchánek]

On Mon, 30 Jan 2017 13:11:40 +0100
Christophe Fergeau <cfergeau at redhat.com> wrote:

> On Fri, Jan 27, 2017 at 03:37:17PM +0100, Michal Suchánek wrote:
> > 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.  
> 
> I did not imply there was such an intent/such a time, however from a
> maintainability perspective (easier review, more accurate git bisect
> in case of problems, ..), I think it's better to have first a
> code-movement only patch not changing behaviour, and then adjusting
> the size checks, with proper documentation in the log as to why they
> are valid changes.

Indeed, there was no such intent. The size checks were found hard to
read and bogus so the commit rewrites the size checks completely based
on the protocol structure definitions in the header. I added text to the
commit message in v7 to make it clear that there is intended functional
change.

> 
> This way, the only big patch is the code movement one, and the
> reviewer can focus on seeing if everything is still the same, rather
> than having to figure that some part are the same, and then guess the
> reasons why other parts are not the same. And if git bisect ever
> point at this commit, it's clear that before/after this patch, there
> should be *no* behaviour changes.
> Then you can have smaller patches on top of that adjusting the new
> function to add missing size checks, adjust too lax ones, ...
> I've tried to do this in the attached patches.

Of course, if you split the commit and bisection points to one of the
commits that clearly change the behavior for a few message types it is
then easy to check handling of those messages and fix it. If the
bisection points to the supposedly noop commit you will still have to
check everything. If you wanted to ease the bisection even more you
could make commits for every changed line of code or message type or
whatever.

It does not improve manageability and review in my view. You want to
ensure that the size check is correct so you have one function to do
checks and one header to compare it with. If something stops working
the size check function should produce a message for you to hint what
is broken anyway.

Either way, if you prefer splitting the commit arbitrarily I am not
against it.

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/20170210/9adf3253/attachment-0001.sig>