Re: [PATCH 2/4] tools/libxc: Tolerate zero-length records in migration v2 streams

[David Vrabel <david.vrabel@citrix.com>]

On 21/07/16 18:17, Andrew Cooper wrote:
> Under some circumstances, the migration v2 save code would generate valid
> records with zero content, when the intended behaviour was to omit the record
                                      ^^^^^^^^
As explained, this is not the intended behaviour.  I would appreciate it
if you did not misrepresent me here.

> entirely.
> 
> As the stream is otherwise fine, tolerate these records and avoid failing the
> migration.
[...]
> --- a/tools/libxc/xc_sr_restore_x86_hvm.c
> +++ b/tools/libxc/xc_sr_restore_x86_hvm.c
[...]
> +    /*
> +     * Tolerate empty records.  Older sending sides used to accidentally
> +     * generate them.
> +     */
> +    if ( hdr->count == 0 )
> +    {
> +        DBGPRINTF("Skipping empty HVM_PARAMS record\n");
> +        return 0;
> +    }
> +
>      for ( i = 0; i < hdr->count; i++, entry++ )

This loop already handles hdr->count == 0.  The additional check is not
required.

> --- a/tools/libxc/xc_sr_restore_x86_pv.c
> +++ b/tools/libxc/xc_sr_restore_x86_pv.c
> @@ -753,15 +753,26 @@ static int handle_x86_pv_vcpu_blob(struct xc_sr_context *ctx,
>      }
>  
>      /* Confirm that there is a complete header. */
> -    if ( rec->length <= sizeof(*vhdr) )
> +    if ( rec->length < sizeof(*vhdr) )
>      {
> -        ERROR("%s record truncated: length %u, min %zu",
> -              rec_name, rec->length, sizeof(*vhdr) + 1);
> +        ERROR("%s record truncated: length %u, header size %zu",
> +              rec_name, rec->length, sizeof(*vhdr));
>          goto out;
>      }
>  
>      blobsz = rec->length - sizeof(*vhdr);
>  
> +    /*
> +     * Tolerate empty records.  Older sending sides used to accidentally
> +     * generate them.
> +     */
> +    if ( blobsz == 0 )
> +    {
> +        DBGPRINTF("Skipping empty %s record for vcpu %u\n",
> +                  rec_type_to_str(rec->type), vhdr->vcpu_id);
> +        goto out;
> +    }

This check for a zero-sized blob should be immediately prior to the

   blob = malloc(blobsz);

So all the other length validation is not skipped.  In particular, some
record types may wish to make zero-length blobs invalid.

David

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