js::SourceCompressionTask::work can leak a block on OOM

RESOLVED FIXED in Firefox 49

Status

()

Core
JavaScript Engine
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jimb, Assigned: fitzgen)

Tracking

unspecified
mozilla49
Points:
---

Firefox Tracking Flags

(firefox49 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
js::SourceCompressionTask::work doesn't properly manage reallocing a UniquePtr, and can leak a block on OOM.

    // The compressed output is greater than half the size of the
    // original string. Reallocate to the full size.
    compressed.reset(static_cast<char*>(js_realloc(compressed.release(), inputBytes)));
    if (!compressed)
        return OOM;

js_realloc returns nullptr on OOM, but the block passed in is still allocated. During the call to js_realloc above, js_realloc has the only pointer to the block; if it returns nullptr, even that has disappeared, and the block has been leaked.

The code below manages the same situation correctly:

    if (auto newCompressed = static_cast<char*>(js_realloc(compressed.get(), compressedBytes))) {
        // If the realloc succeeded, compressed is now a freed pointer.
        mozilla::Unused << compressed.release();
        compressed.reset(newCompressed);
    }

Since js_realloc may or may not free the block, the call to reset must be conditional.

I just noticed this, trying to figure out a better idiom for realloc'ing a UniquePtr. I should have caught it in the initial review; sorry about that.

This is a super-ugly idiom. I wonder if std::unique_ptr has something better for this.
(Reporter)

Comment 1

2 years ago
> This is a super-ugly idiom. I wonder if std::unique_ptr has something better for this.

It doesn't seem to. :(
Assignee: nobody → nfitzgerald
Status: NEW → ASSIGNED
Created attachment 8755460 [details] [diff] [review]
Fix OOM handling of js_realloc in SourceCompressionTask::work
Attachment #8755460 - Flags: review?(jimb)
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=537828911a83
(Reporter)

Comment 4

2 years ago
Comment on attachment 8755460 [details] [diff] [review]
Fix OOM handling of js_realloc in SourceCompressionTask::work

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

::: js/src/jsscript.cpp
@@ +2060,5 @@
> +    if (auto newPtr = static_cast<char*>(js_realloc(unique.get(), size))) {
> +        // If the realloc succeeded, unique is now holding a freed pointer.
> +        mozilla::Unused << unique.release();
> +        unique.reset(newPtr);
> +        return true;

It's SpiderMonkey style to handle errors early, and make the success path the spine of the function.
Attachment #8755460 - Flags: review?(jimb) → review+
Created attachment 8755523 [details] [diff] [review]
Fix OOM handling of js_realloc in SourceCompressionTask::work
Attachment #8755523 - Flags: review+
Attachment #8755460 - Attachment is obsolete: true

Comment 6

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/112f2f385e46

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/112f2f385e46
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.