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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Shriram Rajagopalan writes ("Re: [Xen-devel] [PATCH 1 of 2] tools/libxc: Remus 
Checkpoint Compression"):
> On Mon, Jun 27, 2011 at 11:33 AM, Ian Jackson 
> <Ian.Jackson@xxxxxxxxxxxxx>wrote:
> > 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.

No.  Also the for loop's use of two variables which have to be kept
in step to avoid overrunning is poor style.

> > 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)

I mean the decompressor makes many accesses to larger-than-char
objects using addresses computed using arbitrary offsets.  Unaligned
accesses are not legal portable C and even on architectures where they
don't trap they are often slow.

> > 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.

Perhaps you can get another of the Remus developers to help you
develop your C programming skills.

Ian.

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