Re: [Xen-devel] [PATCH 3/4] tools/libxc: Avoid generating inappropriate zero-length records

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Mon, Jul 25, 2016 at 06:15:37PM +0100, Ian Jackson wrote:
> David Vrabel writes ("Re: [Xen-devel] [PATCH 3/4] tools/libxc: Avoid 
> generating inappropriate zero-length records"):
> > On 21/07/16 18:17, Andrew Cooper wrote:
> > > It was never intended for records such as these to be sent with zero 
> > > content.
> > 
> > As the original author of the specification I'm perhaps best placed to
> > say what the original intention is.
> 
> I think this discussion of `the intent' is not particularly helpful,
> when the authors of the spec document, and of the code, disagree.  It
> is in any case not necessary to decide what `the original intent' is
> or was.  Accordingly, can all participants please stop referring to
> `the intent' in this way.  (It is of course fine to write `_my_
> intent'.)
> 
> What is necessary is to decide what should be done now.
> 
> I am going to quote liberally from the rest of David's mail but adjust
> some of the wording to try to make it something I can agree with:
> 
>   For records such as HVM_PARAMS which consist of a set of N items,
>   David's intention in the spec, was to most definitely send a record
>   with 0 items.
> 
>   For records that fetch an opaque blob from the hypervisor, again,
>   David's intention was to send this blob as-is with no sort of
>   processing or other checking. i.e., if the hypervisor gives us a
>   zero-length blob we sent that as-is.
> 
>   This makes all the streams look the same with all the same records,
>   regardless of what hardware platform it was run on.  Including
>   zero-length/count records also makes diagnosing problems easier -- the
>   empty record is visible in the stream instead of having to remember that
>   sometimes these records are deliberately omitted.
> 
>   As such, IMO this series should be limited to making the restore
>   side handle the zero count sets or zero length blobs if it does not
>   do so already.
> 
>   The specification should be clarified to note that some records may have
>   zero-length blobs or contain zero items.
> 
> I reviewed the spec in detail at the time and I agree with David's
> point of view as I have rephrased (hopefully without annoying David)
> above.
> 
> I see no reason why zero-content-length records should be treated as
> any kind of special case.
> 
> Is the ultimate bug that we are tripping over here simply that the
> code calls malloc(0) and then bails if the libc produces NULL (as it
> is entitled to do) ?
> 

No, it isn't.

AIUI the issue is receiving end can't deal with zero-length record.  To
to more precise, it is the hypervisor that chokes when toolstack issues
an hypercall with the "malformed" data.

Hence the two approaches presented: one is to omit zero-length record,
the other is to tolerate zero-length record.

If we go with David's approach, I think hypervisor should be made
tolerant to zero-length record. That would be symmetric on both ends --
hv can spit out as well as accept zero-length records. Toolstack should
transparently send and receive records.

Wei.


> Thanks,
> Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel