Closed Bug 1323183 Opened 3 years ago Closed 3 years ago

[Static Analysis][Resource leak] In function XPCConvert::JSTypedArray2Native

Categories

(Core :: XPConnect, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: andi, Assigned: andi)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: CID 1397074)

Attachments

(1 file)

The Static Analysis tool Coverity detected that there could be a memory leak in the the following switch statement:

>>    case js::Scalar::Int8:
>>        if (!CheckTargetAndPopulate(nsXPTType::T_I8, type,
>>                                    sizeof(int8_t), count,
>>                                    jsArray, &output, pErr)) {
>>            return false;
>>        }
>>        break;

This can happen only if function CheckTargetAndPopulate exits with pErr - NS_ERROR_XPC_BAD_CONVERT_JS. I'm not sure that this could ever happen, but we still could prevent a potential issue by adding a null check and freeing the memory.
Note that `moz_xmalloc` is infallible.

If we want to keep that property, a better solution would be to delay calling `moz_xmalloc` in `CheckTargetAndPopulate` until right before the `memcpy`.

Alternatively, if we want this allocation to be fallible (on the one hand, this is a script-controlled allocation; on the other hand, the script is probably chrome and already has an array buffer of the same size on hand), we should free and null out the allocated buffer in `CheckTargetAndPopulate`, and make it clear that the returned buffer will always be null on failure.

In either case, we can make this even clearer by eliminating the outparam in favour of returning the point to the newly allocated buffer.
Comment on attachment 8818242 [details]
Bug 1323183 - prevent memory leak from CheckTargetAndPopulate.

https://reviewboard.mozilla.org/r/98392/#review98642

Yeah, I agree with Ms2ger. Simplest fix would be to just reorder things in the callee. Bonus points for returning the pointer directly instead of via an outparam.

I'm fine with leaving the allocation as infallible.
Attachment #8818242 - Flags: review?(bobbyholley) → review-
Will do that and repudiate the patch.
Comment on attachment 8818242 [details]
Bug 1323183 - prevent memory leak from CheckTargetAndPopulate.

https://reviewboard.mozilla.org/r/98392/#review98902

::: js/xpconnect/src/XPCConvert.cpp:1358
(Diff revision 2)
> +    size_t max = UINT32_MAX / typeSize;
> +
> +    // This could overflow on 32-bit systems so check max first.
> +    size_t byteSize = count * typeSize;
> +    if (count > max) {
> +        if (pErr)
> +            *pErr = NS_ERROR_OUT_OF_MEMORY;
> +
> +        return nullptr;
> +    }

Is there a good reason to move this section? It doesn't really matter, but no sense in increasing churn, so please move it back unless I'm missing something.

::: js/xpconnect/src/XPCConvert.cpp:1369
(Diff revision 2)
> +            *pErr = NS_ERROR_OUT_OF_MEMORY;
> +
> +        return nullptr;
> +    }
> +
> +    output = moz_xmalloc(byteSize);

Please just declare this here rather than forward-declaring it at the top of this function.

::: js/xpconnect/src/XPCConvert.cpp:1410
(Diff revision 2)
>  
>      void* output = nullptr;
>  
>      switch (JS_GetArrayBufferViewType(jsArray)) {
>      case js::Scalar::Int8:
> -        if (!CheckTargetAndPopulate(nsXPTType::T_I8, type,
> +        if (!(output = CheckTargetAndPopulate(nsXPTType::T_I8, type,

For readability, please separate out the assignment from the nullcheck.
Attachment #8818242 - Flags: review?(bobbyholley) → review-
Comment on attachment 8818242 [details]
Bug 1323183 - prevent memory leak from CheckTargetAndPopulate.

https://reviewboard.mozilla.org/r/98392/#review98912

Looks good, thanks.
Attachment #8818242 - Flags: review?(bobbyholley) → review+
Pushed by bpostelnicu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e3b82debff88
prevent memory leak from CheckTargetAndPopulate. r=bholley
https://hg.mozilla.org/mozilla-central/rev/e3b82debff88
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.