Closed Bug 1193564 Opened 6 years ago Closed 6 years ago

Check result of Read32 in nsSupportsArray::Read

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: mccr8, Assigned: mccr8)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

No description provided.
This is a simple patch, but it would be good if you double checked that it is okay to bail out in the middle of the method.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6fe40a45f6e7
Attachment #8646653 - Flags: review?(nfroyd)
Comment on attachment 8646653 [details] [diff] [review]
Check result of Read32 in nsSupportsArray::Read.

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

Eric wanted to review more code, so Eric can decide whether bailing out in the middle here is OK. :)
Attachment #8646653 - Flags: review?(nfroyd) → review?(erahm)
Comment on attachment 8646653 [details] [diff] [review]
Check result of Read32 in nsSupportsArray::Read.

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

r=me. We return early elsewhere in the function [1], so this doesn't change the overall behavior of bailing on failure.

[1] https://dxr.mozilla.org/mozilla-central/rev/d4f3a8a75577e4af2914a4e899ca2e724f9715c4/xpcom/ds/nsSupportsArray.cpp#213-216,223-228

::: xpcom/ds/nsSupportsArray.cpp
@@ +188,5 @@
>    nsresult rv;
>  
>    uint32_t newArraySize;
>    rv = aStream->Read32(&newArraySize);
> +  if (NS_FAILED(rv)) return rv;

nit: Lets use the predominant style here, multiline and braced.
Attachment #8646653 - Flags: review?(erahm) → review+
(In reply to Eric Rahm [:erahm] from comment #3)
> nit: Lets use the predominant style here, multiline and braced.

Fixed.
https://hg.mozilla.org/mozilla-central/rev/0051268bfb18
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.