1234177 - [Static Analysis][Unchecked return value] Function CustomWriteHandler from ExportHelpers.cpp

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Andi [:andi]]

The Static Analysis tool Coverity added that return type of mFunctions.append(aObj) is not checked:

>>if (mOptions->cloneFunctions) { 
>>    mFunctions.append(aObj);
>>    return JS_WriteUint32Pair(aWriter, SCTAG_FUNCTION, mFunctions.length() - 1);
>>} else {

mFunctions.append(aObj) can return fail only when it's OOM, but still i think we should check it, because in case when mFunctions is empty JS_WriteUint32Pair will be called with:

>> JS_WriteUint32Pair(aWriter, SCTAG_FUNCTION, -1);

The last parameter (-1) will be casted to uint32 and it will get a value of 2^32-1 that could lead to undefined behavior later on.

Comment 1

[Andi [:andi]]

Attachment: Bug 1234177.diff

Comment 2

[Bobby Holley (:bholley)]

Comment on attachment 8700565 [details] [diff] [review]
Bug 1234177.diff

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

::: js/xpconnect/src/ExportHelpers.cpp
@@ +205,5 @@
>  
>          if (JS::IsCallable(aObj)) {
>              if (mOptions->cloneFunctions) {
> +                if (mFunctions.append(aObj)) {
> +                    JS_ReportError(aCx, "OOM when appending aObj to mFunction.");

In JSAPI, returning null without reporting an error is the way to signal OOM, so we don't need this JS_ReporterError here.

@@ +207,5 @@
>              if (mOptions->cloneFunctions) {
> +                if (mFunctions.append(aObj)) {
> +                    JS_ReportError(aCx, "OOM when appending aObj to mFunction.");
> +                    return false;
> +                } else

Given the early return above, we should omit the |else| here.

Comment 3

[Andi [:andi]]

Attachment: Bug 1234177.diff

The else is useless, you are right, i put it for readability.

Comment 4

[Bobby Holley (:bholley)]

Comment on attachment 8700941 [details] [diff] [review]
Bug 1234177.diff

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

::: js/xpconnect/src/ExportHelpers.cpp
@@ +205,5 @@
>  
>          if (JS::IsCallable(aObj)) {
>              if (mOptions->cloneFunctions) {
> +                if (mFunctions.append(aObj))
> +                    return false;

This is going to return false if append _succeeds_, right?

Comment 5

[Bobby Holley (:bholley)]

(In reply to Bogdan Postelnicu from comment #3)
> Created attachment 8700941 [details] [diff] [review]
> Bug 1234177.diff
> 
> The else is useless, you are right, i put it for readability.

Yeah, it's a style thing. See https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

Comment 6

[Andi [:andi]]

Attachment: Bug 1234177.diff

Forgot to not the append.

Comment 7

[Bobby Holley (:bholley)]

Comment on attachment 8701365 [details] [diff] [review]
Bug 1234177.diff

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

Looks good, thanks!

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1f862b370ab3

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Next time please make sure the patch has the right author info!

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7b60d08cf7c8 (backout)
https://hg.mozilla.org/integration/mozilla-inbound/rev/f40c0da4e4ce

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f40c0da4e4ce