code review 114810043: compress/bzip2: fix panics on malformed input.

compress/bzip2: fix panics on malformed input.

Fixes 8363.

[bradfitz, 2014-07-14 20:46:43]

Add crashing test from the bug?



On Mon, Jul 14, 2014 at 1:42 PM, <agl@golang.org> wrote:

> Reviewers: bradfitz,
>
> Description:
> compress/bzip2: fix panics on malformed input.
>
> Fixes 8363.
>
> Please review this at https://codereview.appspot.com/114810043/
>
> Affected files (+19, -2 lines):
>   M src/pkg/compress/bzip2/bzip2.go
>
>
> Index: src/pkg/compress/bzip2/bzip2.go
> ===================================================================
> --- a/src/pkg/compress/bzip2/bzip2.go
> +++ b/src/pkg/compress/bzip2/bzip2.go
> @@ -261,6 +261,11 @@
>                 }
>         }
>
> +       if numSymbols == 0 {
> +               // There must be an EOF symbol.
> +               return StructuralError("no symbols in input")
> +       }
> +
>         // A block uses between two and six different Huffman trees.
>         numHuffmanTrees := br.ReadBits(3)
>         if numHuffmanTrees < 2 || numHuffmanTrees > 6 {
> @@ -307,10 +312,10 @@
>
>         // Now we decode the arrays of code-lengths for each tree.
>         lengths := make([]uint8, numSymbols)
> -       for i := 0; i < numHuffmanTrees; i++ {
> +       for i := range huffmanTrees {
>                 // The code lengths are delta encoded from a 5-bit base
> value.
>                 length := br.ReadBits(5)
> -               for j := 0; j < numSymbols; j++ {
> +               for j := range lengths {
>                         for {
>                                 if !br.ReadBit() {
>                                         break
> @@ -333,6 +338,12 @@
>         }
>
>         selectorIndex := 1 // the next tree index to use
> +       if len(treeIndexes) == 0 {
> +               return StructuralError("no tree selectors given")
> +       }
> +       if int(treeIndexes[0]) >= len(huffmanTrees) {
> +               return StructuralError("tree selector out of range")
> +       }
>         currentHuffmanTree := huffmanTrees[treeIndexes[0]]
>         bufIndex := 0 // indexes bz2.buf, the output buffer.
>         // The output of the move-to-front transform is run-length encoded
> and
> @@ -350,6 +361,12 @@
>         decoded := 0 // counts the number of symbols decoded by the
> current tree.
>         for {
>                 if decoded == 50 {
> +                       if selectorIndex >= numSelectors {
> +                               return StructuralError("insufficient
> selector indices for number of symbols")
> +                       }
> +                       if int(treeIndexes[selectorIndex]) >=
> len(huffmanTrees) {
> +                               return StructuralError("tree selector out
> of range")
> +                       }
>                         currentHuffmanTree = huffmanTrees[treeIndexes[
> selectorIndex]]
>                         selectorIndex++
>                         decoded = 0
>
>
>

[agl1, 2014-07-16 01:36:39]

On 2014/07/14 20:46:43, bradfitz wrote:
> Add crashing test from the bug?

Done.

[bradfitz, 2014-07-16 01:43:31]

LGTM
 On Jul 15, 2014 6:36 PM, <agl@golang.org> wrote:

> On 2014/07/14 20:46:43, bradfitz wrote:
>
>> Add crashing test from the bug?
>>
>
> Done.
>
> https://codereview.appspot.com/114810043/
>

[agl1, 2014-07-16 01:44:32]

Hello bradfitz@golang.org (cc: golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go

[agl1, 2014-07-16 01:44:52]

*** Submitted as https://code.google.com/p/go/source/detail?r=395bf97d72a1 ***

compress/bzip2: fix panics on malformed input.

Fixes 8363.

LGTM=bradfitz
R=bradfitz
CC=golang-codereviews
https://codereview.appspot.com/114810043

[gobot, 2014-07-16 02:05:57]

This CL appears to have broken the nacl-amd64p32 builder.
See http://build.golang.org/log/3513218d696362fe3f6e5bc5eed6584fe0faff8d