Re: [Xen-devel] [PATCH 1 of 2] tools/libxc: Remus Checkpoint Compression

[Shriram Rajagopalan <rshriram@xxxxxxxxx>]

On Mon, Jun 27, 2011 at 11:33 AM, Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx> wrote:

Shriram Rajagopalan writes ("[Xen-devel] [PATCH 1 of 2] tools/libxc: Remus Checkpoint Compression"):
> [stuff]

Thanks for this.  I have some comments.

TBH I don't think "remus" is the right name for this functionality.
This checkpoint compression may be useful in non-Remus applications
(we won't know until we get some numbers) so it would be sensible to
name the functions and files accordingly.

> +        if (!buf->pages) {
> +            if (!(buf->pages = malloc(buf->compbuf_size))) {
> +                ERROR("Could not allocate compression buffer");
> +                return -1;
> +            }
> +        } else {
> +            if (!(ptmp = realloc(buf->pages, buf->compbuf_size))) {
> +                ERROR("Could not reallocate compression buffer");
> +                return -1;
> +            }
> +            buf->pages = ptmp;

realloc(0, some_nonzero_size) is legal and does what you would hope.
Using that would simplify this.  (Yes, I know the original code does
it the verbose way later too...)

> +static
> +int __uncompress(xc_interface *xch, char *destpage, unsigned long *compbuf_pos,
> +                 char *compbuf, unsigned long compbuf_size)

Identifiers starting with "__" are reserved for the C implementation
and must not be used in Xen userspace libraries like libxc.  (The same
goes for identifiers starting with "_" with external linkage; although
those may be used for other purposes.)

> +typedef unsigned int data_t;

uint16_t surely.

> +    if (src[0].off == BEGIN)
> +    {
> +        *compbuf_pos += sizeof(struct marker);
> +        for (i = 1; (*compbuf_pos < compbuf_size) && (src[i].off >= 0);
> +             i++, *compbuf_pos += sizeof(struct marker))
> +            dest[src[i].off] = src[i].val;
> +    }
> +    else if (src[0].off == FULLPAGE)
> +    {
> +        *compbuf_pos += sizeof(struct marker) + XC_PAGE_SIZE;
> +        memcpy(destpage, (char *)&src[1], XC_PAGE_SIZE);
> +    }

I think there are some bugs here:

1. The accesses of src, and both arms of the conditional I quote
  above, seem to be able to read beyond the end of the compressed
  data buffer.

I dont think that is possible in the if() block , as the conditional in the
for statement (*compbuf_pos < compbuf_size) takes care of it. But I agree
that the else() block can use some checks.

2. The value of src[i].off is used without checking that it's within
  the allowable range.
 

3. The code seems to be full of unaligned accesses.

 Can you elaborate on this? When the data is compressed, it automatically
loses the alignments (no more page boundaries, or 4 byte boundaries).
Do you mean padding the struct marker to 8 bytes instead of 6 bytes ?
 (the counter argument is given in the comment section preceding the typedefs)

When you have prepared a draft revised patch, can you please have
another Remus developer review the code for security problems ?
I would like to see them provide a formal reviewed-by/acked-by on your
resubmission.

Thanks,
Ian.

shriram
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel