[PATCHES] MSZIP support for cabinet files

[Andreas Schneider]

On Thursday, 13 July 2017 01:22:02 CEST Jeremy Allison via samba-technical 
wrote:
> On Mon, Jul 03, 2017 at 07:11:07PM +0200, Aurélien Aptel via samba-technical 
wrote:
> > After more careful reading I think code will fail early already, I've
> > switched some variables to size_t for counts and offsets just in case.
> > 
> > In the pull function I've added more remaining byte checks for when we
> > are raw-pulling CFDATAs.
> > 
> > I've also changed the magic 15 constant to MAX_WBITS, like it was
> > changed in other code paths.
> > 
> > Feel free to squash if you think it's appropriate. I'm attaching the
> > whole thing again with my last 3 commits appended.
> 
> Getting better. This part concerns me:
> 
> +                * similarly, we don't know the compressed
> +                * size yet, remember offset, write zeros,
> +                * fill out later
> +                */
> +               compressed_offset = ndr->offset;
> +               NDR_CHECK(ndr_push_uint16(ndr, NDR_SCALARS, 0));
> +               NDR_CHECK(ndr_push_uint16(ndr, NDR_SCALARS, r->cbUncomp));
> +
> +
> +               switch (cab_ctype) {
> +               case CF_COMPRESS_NONE:
> +                       /* just copy the data */
> +                       NDR_CHECK(ndr_push_bytes(ndr, r->ab.data,
> r->ab.length)); +                       compressed_length = r->cbUncomp;
> +
> +                       break;
> +
> 
> We write a 16-bit value: r->cbUncomp, then write out
> r->ab.length bytes and set compressed_length = r->cbUncomp - a totally
> different (and client supplied I think) field.
> 
> What if they don't match ? The other case statement uses the
> offset from struct ndr_push * after the push has been done.
> Can we do the same in the uncompressed case too ? Or maybe
> abort if r->cbUncomp != r->ab.length in the uncompressed
> case (as they should be the same).
> 
> Inside:
> 
> +static enum ndr_err_code ndr_push_compression_mszip_cab_chunk(struct
> ndr_push *ndrpush, +                                                       
>      struct ndr_pull *ndrpull, +                                           
>                  struct ndr_compression_state *state) +{
> +       DATA_BLOB comp_chunk;
> +       uint32_t comp_chunk_size;
> +       DATA_BLOB plain_chunk;
> +       uint32_t plain_chunk_size;
> +       uint32_t plain_chunk_offset;
> +       uint32_t max_plain_size = 0x00008000;
> +       /*
> +        * The maximum compressed size of each MSZIP block is 32k + 12 bytes
> +        * header size.
> +        */
> +       uint32_t max_comp_size = 0x00008000 + 12;
> +       int z_ret;
> +       z_stream *z;
> +
> +       plain_chunk_size = MIN(max_plain_size, ndrpull->data_size -
> ndrpull->offset);
> 
> Can you add a check that 'ndrpull->data_size > ndrpull->offset'.
> 
> I know this will never happen, but I really hate seeing raw
> unchecked arithmetic on values in marshalling/unmarshalling code.
> It makes me nervous :-). I need to feel calm when reading this
> code :-).
> 
> Also here:
> 
> +	comp_chunk_size = 2 + z->total_out;
> 
> Jeremy.

Aurelien,

I've marked your commits where they should be squashed and I have an addional 
commit. See

https://git.samba.org/?p=asn/samba.git;a=shortlog;h=refs/heads/master-cab

-- 
Andreas Schneider                   GPG-ID: CC014E3D
Samba Team                             asn at samba.org
www.samba.org