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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jim Blandy :jimb]

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.

Comment 1

[Jim Blandy :jimb]

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

It doesn't seem to. :(

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Fix OOM handling of js_realloc in SourceCompressionTask::work

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=537828911a83

Comment 4

[Jim Blandy :jimb]

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.

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Fix OOM handling of js_realloc in SourceCompressionTask::work

Comment 6

[Pulsebot]

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

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/112f2f385e46