This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
Home Products Support Community News


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

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.


Xen-devel mailing list