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

RESOLVED FIXED in Firefox 52



JavaScript Engine
2 years ago
a year ago


(Reporter: arai, Assigned: jandem)



Dependency tree / graph

Firefox Tracking Flags

(firefox51 unaffected, firefox52 fixed)



(2 attachments, 1 obsolete attachment)



2 years ago
Created attachment 8796946 [details]
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.

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

2 years ago
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

2 years ago
(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.
Summary: Compressor fails to inflate when the deflated string is longer than input string. → Compressor fails to inflate when the deflated data is longer than the half of input data.

Comment 3

2 years ago
Created attachment 8796957 [details] [diff] [review]

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.
Assignee: nobody → jdemooij
Attachment #8796957 - Flags: review?(arai.unmht)

Comment 4

2 years ago
Comment on attachment 8796957 [details] [diff] [review]

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

2 years ago
Created attachment 8796958 [details] [diff] [review]

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.
Attachment #8796957 - Attachment is obsolete: true
Attachment #8796957 - Flags: review?(arai.unmht)
Attachment #8796958 - Flags: review?(arai.unmht)

Comment 6

2 years ago
Comment on attachment 8796958 [details] [diff] [review]

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.
Attachment #8796958 - Flags: review?(arai.unmht) → review+


2 years ago
Blocks: 1304390
status-firefox51: --- → unaffected


2 years ago
Blocks: 1306700
status-firefox52: affected → fixed
Target Milestone: --- → mozilla52


2 years ago
Last Resolved: 2 years ago
Resolution: --- → FIXED
Group: core-security → core-security-release


2 years ago
Duplicate of this bug: 1306700
Group: core-security-release
Keywords: regression
You need to log in before you can comment on or make changes to this bug.