1306963 - Compressor fails to inflate when the deflated data is longer than the half of input data.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tooru Fujisawa [:arai]]

Attachment: testcase with long random string.

I'm sure this is almost a dupe of bug 1306700.
but I wanted to post a standalone testcase, and I'm not sure whether the testcase is safe or not.
so filing as a separated bug marking as security sensitive.


With attached code, the following assertion hits in js shell.

https://dxr.mozilla.org/mozilla-central/rev/7c576fe3279d87543f0a03b844eba7bc215e17f1/js/src/vm/Compression.cpp#248
>         MOZ_RELEASE_ASSERT(ret == Z_STREAM_END);

looks like the issue happens if the input string has almost no redundancy and deflated string gets longer than input (not sure the actual threshold tho...).

such case happens when the input JS code is mostly consisted from data: URL with base64 encoded JPEG, like the google image result that contains many data URLs in JS.

the attached code contains long random string and lazy function, that also gets longer with deflate operation.

Comment 1

[Jan de Mooij [:jandem]]

Thanks for the testcase! Finishing a patch for this.

The compressed string is not longer than the input string (the input is in two byte chars), but it's more than half the size of the input so it does trigger the MOREOUTPUT case while flushing.

Comment 2

[Tooru Fujisawa [:arai]]

(In reply to Jan de Mooij [:jandem] from comment #1)
> the input is in two byte chars

wow, I overlooked that.
indeed, I was checking the number of characters.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Patch

This fixes a couple of issues:

(1) In Compressor::compressMore, we did:

  bool done = left <= MAX_INPUT_SIZE;
  if (done)
      zs.avail_in = left;

But later, we could cap zs.avail_in to the chunk limit and |done == true| was no longer correct. I fixed this by setting |done| after that.

(2) The following code we always had, but is wrong since chunked compression:

  if (ret == Z_BUF_ERROR || (done && ret == Z_OK)) {
      MOZ_ASSERT(zs.avail_out == 0);
      return MOREOUTPUT;
  }

I changed this to:

  if (ret == Z_BUF_ERROR || (ret == Z_OK && zs.avail_out == 0)) {

We incorrectly finished a chunk before it was actually finished by zlib. Now we return MOREOUTPUT, allocate more output, and then we finish the chunk.

(3) I wrote some tests for this and got an assertion failure in Compressor::~Compressor, there we are trying to assert we aborted the compression based on some state. I added a |bool aborted| that we set to |true| when we abort, so now we can just check that instead.

I wrote a test based on the one arai attached, but it uses a deterministic Math.random so we don't have to add a large test file.

Comment 4

[Tooru Fujisawa [:arai]]

Comment on attachment 8796957 [details] [diff] [review]
Patch

Review of attachment 8796957 [details] [diff] [review]:
-----------------------------------------------------------------

Looks almost good :)
I have one question about |aborted| below.

::: js/src/jit-test/tests/basic/compression-random-data.js
@@ +22,5 @@
> +    eval(s);
> +}
> +for (var i=32740; i<32780; i += 5) {
> +    test(i);
> +}

This test might hit timeout, on cgc build with certain runtime option.

::: js/src/jsscript.cpp
@@ +1809,5 @@
>      bool cont = true;
>      bool reallocated = false;
>      while (cont) {
> +        if (abort_) {
> +            comp.setAborted();

there are some other cases that this loop exits with returning Aborted or OOM, without setting |aborted|.
won't them hit |MOZ_ASSERT(aborted);| assertion in Compressor dtor?

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Patch

Good point. Here's a patch that changes |bool aborted| to |bool finished|, and sets it in finish(). I like this more because we now don't have to change the Compressor's public interface.

Comment 6

[Tooru Fujisawa [:arai]]

Comment on attachment 8796958 [details] [diff] [review]
Patch

Review of attachment 8796958 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you again :)

about cgc build, I was wrong. it's using --enable-optimize and it should be fast enough for this test.

Comment 7

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a92b2f6ab6ad5279682451506da866f926d27be9

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a92b2f6ab6ad