Closed Bug 1234177 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: XPConnect, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: andi, Assigned: andi)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: CID 1325687 )

Attachments

(1 file, 2 obsolete files)

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.
Attached patch Bug 1234177.diff (obsolete) — Splinter Review
Attachment #8700565 - Flags: review?(bobbyholley)
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.
Attachment #8700565 - Flags: review?(bobbyholley) → review-
Attached patch Bug 1234177.diff (obsolete) — Splinter Review
The else is useless, you are right, i put it for readability.
Attachment #8700565 - Attachment is obsolete: true
Attachment #8700941 - Flags: review?(bobbyholley)
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?
Attachment #8700941 - Flags: review?(bobbyholley) → review-
(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
Attached patch Bug 1234177.diffSplinter Review
Forgot to not the append.
Attachment #8700941 - Attachment is obsolete: true
Attachment #8701365 - Flags: review?(bobbyholley)
Comment on attachment 8701365 [details] [diff] [review]
Bug 1234177.diff

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

Looks good, thanks!
Attachment #8701365 - Flags: review?(bobbyholley) → review+
Keywords: checkin-needed
Next time please make sure the patch has the right author info!
https://hg.mozilla.org/mozilla-central/rev/f40c0da4e4ce
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.