Re: [Xen-devel] [PATCH v2] libxc: add LZ4 decompression support

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 24.09.13 at 18:53, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> On Tue, 2013-09-24 at 17:29 +0100, Jan Beulich wrote:
>> @@ -573,6 +575,124 @@ int xc_try_xz_decode(struct xc_dom_image
>>  
>>  #endif /* !__MINIOS__ */
>>  
>> +#define CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
>> +#define STATIC static
>> +#define u8 uint8_t
>> +#define u16 uint16_t
>> +#define u32 uint32_t
>> +#define u64 uint64_t
>> +#define INIT
>> +#define unlikely(x) (x)
> 
> I think rather than pollute this file with this sort of thing (which
> might have unexpected consequences in the future) this would be better
> of placed in a separate file compiled for both regular and stub use.

So you mean to move the whole thing quoted above down to ...

>> +
>> +static inline uint_fast16_t le16_to_cpup(const unsigned char *buf)
>> +{
>> +    return buf[0] | (buf[1] << 8);
>> +}
>> +
>> +static inline uint_fast32_t le32_to_cpup(const unsigned char *buf)
>> +{
>> +    return le16_to_cpup(buf) | ((uint32_t)le16_to_cpup(buf + 2) << 16);
>> +}
>> +
>> +#include "../../xen/common/lz4/decompress.c"

... here out into a separate file. I can surely do that. Any
preference for the name (xc_dom_decompress_unsafe_lz4.c
would seem the prime candidate, albeit the other similarly
named files have a slightly different purpose).

The main problem doing so is going to be the uses of
get_unaligned_le32() in xc_try_lz4_decode() - they'd need to
be adjusted, increasing the amount of change from the original.
And lz4_decompress() would the also need to be declared in
some (new?) header. All of which didn't seem very desirable...

>> +
>> +/*
>> + * Note: Uncompressed chunk size is used in the compressor side
>> + * (userspace side for compression).
>> + * It is hardcoded because there is not proper way to extract it
>> + * from the binary stream which is generated by the preliminary
>> + * version of LZ4 tool so far.
>> + */
>> +#define LZ4_DEFAULT_UNCOMPRESSED_CHUNK_SIZE (8 << 20)
>> +#define ARCHIVE_MAGICNUMBER 0x184C2102
>> +
>> +static int xc_try_lz4_decode(
>> +    struct xc_dom_image *dom, void **blob, size_t *psize)
>> +{
>> +    int ret = -1;
>> +    size_t uncomp_chunksize = LZ4_DEFAULT_UNCOMPRESSED_CHUNK_SIZE;
>> +    unsigned char *inp = *blob, *output, *outp;
>> +    ssize_t size = *psize - 4;
>> +    size_t out_size, out_len, dest_len, chunksize;
>> +    const char *msg;
>> +
>> +    if (size < 4) {
>> +            msg = "input too small";
>> +            goto exit_0;
> 
> The exit_0 loop is after the error print, so these errors don't get
> printed.

Oops - fixed.

> I don't really like exit_0 and exit_2 as names (what happened to
> exit_1?). "err" and "err_free" would be more usual.

I didn't like those too, but tried to modify the original as little
as possible. exit_1 disappeared as having got orphaned in the
process of the adaptation.

>> +    }
>> +
>> +    out_size = out_len = get_unaligned_le32(inp + size);
>> +    if (xc_dom_kernel_check_size(dom, out_len)) {
>> +            msg = "Decompressed image too large";
>> +            goto exit_0;
>> +    }
>> +
>> +    output = malloc(out_len);
>> +    if (!output) {
>> +            msg = "Could not allocate output buffer";
>> +            goto exit_0;
>> +    }
>> +    outp = output;
>> +
>> +    chunksize = get_unaligned_le32(inp);
>> +    if (chunksize == ARCHIVE_MAGICNUMBER) {
>> +            inp += 4;
>> +            size -= 4;
>> +    } else {
>> +            msg = "invalid header";
>> +            goto exit_2;
>> +    }
>> +
>> +    for (;;) {
>> +            if (size < 4) {
>> +                    msg = "missing data";
>> +                    goto exit_2;
>> +            }
>> +            chunksize = get_unaligned_le32(inp);
>> +            if (chunksize == ARCHIVE_MAGICNUMBER) {
>> +                    inp += 4;
>> +                    size -= 4;
>> +                    continue;
>> +            }
>> +            inp += 4;
>> +            size -= 4;
> 
> We know size is at least 4 from the check at the head of the loop, but
> now we subtracted 4 so size could be 0, but lz4_decompress doesn't take
> size as an argument so whatever it does it isn't basing the return value
> in &chunksize on it, so I assume it reads over the end of the buffer via
> inp.
> 
> I didn't look to see what the minimum number of bytes which
> lz4_decompress will consume is, but I think we need a check of some
> description.

Sadly there doesn't appear to be a minimum - the main loop in
lz4_uncompress() has just a single exit point, which isn't
dependent upon the input size at all (the function doesn't even
know how much input there is).

Immediately before the call to lz4_decompress() chunksize gets
read, but I can't tell whether we could expect that to be the
required amount of bytes to follow. Yann - can you clarify this?

Jan

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