1380204 - XZStream does not cleanly handle xz files with multiple xz blocks

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Brian J Murray]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/59.0.3071.115 Safari/537.36

Steps to reproduce:

Build Fennec on  system with xz version 5.2 or higher. Configure xz to use multithreaded compression with small lazma blocks.  For example, set XZ_OPT='--threads=4 --block-size=1MiB'. Build and package Fennec


Actual results:

Gecklinker crashes when loading libmozglue.so


Expected results:

Fennec should load correctly

Comment 1

[Brian J Murray]

Attachment: Bug 1380204: Improve error handling in XZStream.cpp

Report init failure if uncompressed stream size is 0.
Check for overflows when casting.
Verify LZMA stream only has a single block.

Review commit: https://reviewboard.mozilla.org/r/156396/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/156396/

Comment 2

[Brian J Murray]

Possible duplicate of Bug 1062916.

Comment 3

[Mike Hommey [:glandium]]

Comment on attachment 8885528 [details]
Bug 1380204: Improve error handling in XZStream.cpp

https://reviewboard.mozilla.org/r/156396/#review163188

::: mozglue/linker/XZStream.cpp:1
(Diff revision 1)
> +
> +

You're adding empty lines.

::: mozglue/linker/XZStream.cpp:76
(Diff revision 1)
>    if (!mDec) {
>      return false;
>    }
>  
>    mUncompSize = ParseUncompressedSize();
> +  if(!mUncompSize) {

space after if

::: mozglue/linker/XZStream.cpp:176
(Diff revision 1)
>    const uint32_t backwardSize = *(footer + 4);
> +  // Check for overflow.
> +  if(backwardSize > (SIZE_MAX  / 4) - 1) {
> +    return 0;
> +  }

I'd rather just use a CheckedInt (see mfbt/CheckedInt.h)

::: mozglue/linker/XZStream.cpp:197
(Diff revision 1)
>    }
>    // The footer follows directly the index, so we can use it as a reference.
>    const uint8_t* end = mInBuf + mBuffers.in_size;
>    const uint8_t* index = end - kFooterSize - indexSize;
>  
> -  // The index consists of a one byte indicator followed by a VLI field for the
> +  // The xz stream index consists of three concatnated elements:

typo: concatenated

::: mozglue/linker/XZStream.cpp:226
(Diff revision 1)
>      return 0;
>    }
>    uint64_t uncompressedSize = 0;
>    index += ParseVarLenInt(index, end - index, &uncompressedSize);
> +  // Check for integer overflow
> +  if(uncompressedSize>SIZE_MAX) {

spaces around >

Comment 4

[Brian J Murray]

Comment on attachment 8885528 [details]
Bug 1380204: Improve error handling in XZStream.cpp

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156396/diff/1-2/

Comment 5

[Brian J Murray]

Comment on attachment 8885528 [details]
Bug 1380204: Improve error handling in XZStream.cpp

https://reviewboard.mozilla.org/r/156396/#review163188

> spaces around >

Entire line deleted.

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 8885528 [details]
Bug 1380204: Improve error handling in XZStream.cpp

https://reviewboard.mozilla.org/r/156396/#review165374

::: mozglue/linker/XZStream.cpp:176
(Diff revision 2)
>      return 0;
>    }
>    // Backward size is a 32 bit LE integer field positioned after the 32 bit CRC32
>    // code. It encodes the index size as a multiple of 4 bytes with a minimum
>    // size of 4 bytes.
> -  const uint32_t backwardSize = *(footer + 4);
> +  const uint32_t bBackwardSizeRaw = *(footer + 4);

why the bB?

::: mozglue/linker/XZStream.cpp:179
(Diff revision 2)
>    // code. It encodes the index size as a multiple of 4 bytes with a minimum
>    // size of 4 bytes.
> -  const uint32_t backwardSize = *(footer + 4);
> -  return (backwardSize + 1) * 4;
> +  const uint32_t bBackwardSizeRaw = *(footer + 4);
> +  // Check for overflow.
> +  mozilla::CheckedInt<size_t> backwardSizeBytes;
> +  backwardSizeBytes = (bBackwardSizeRaw + 1) * 4;

I don't think this works. You should initialize backwardSizeBytes with the raw size, and then add one and multiply.

::: mozglue/linker/XZStream.cpp:232
(Diff revision 2)
>    }
>    uint64_t uncompressedSize = 0;
>    index += ParseVarLenInt(index, end - index, &uncompressedSize);
> +  mozilla::CheckedInt<size_t> checkedSize(uncompressedSize);
> +  if (!checkedSize.isValid()) {
> +    ERROR("XZ parsing: Uncompressed stream size is to large");

typo: too.

Comment 7

[Brian J Murray]

Comment on attachment 8885528 [details]
Bug 1380204: Improve error handling in XZStream.cpp

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156396/diff/2-3/

Comment 8

[Mike Hommey [:glandium]]

Comment on attachment 8885528 [details]
Bug 1380204: Improve error handling in XZStream.cpp

https://reviewboard.mozilla.org/r/156396/#review168656

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Autoland can't push this until all pending issues in MozReview are marked as resolved.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0d4f763f4080
Improve error handling in XZStream.cpp r=glandium

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/0d4f763f4080