Closed Bug 1274867 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: jimb, Assigned: fitzgen)

Details

Attachments

(1 file, 1 obsolete file)

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.
> 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
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+
Attachment #8755460 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/112f2f385e46
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.