[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2] libxc: add LZ4 decompression support
>>> 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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |