798065 - integer underflow in nsTArray::LastIndexOf with empty array

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[William Chen [:wchen]]

The offending line of code is

start = Length() - 1;

both start and Length() are uint32_t. When we have an empty array Length() = 0 and Length() - 1 results in UINT32_MAX that causes an invalid memory read.

Fortunately I only found usage of LastIndexOf in test cases.

Comment 1

[William Chen [:wchen]]

Attachment: Fix integer underflow in nsTArray::LastIndexOf

Comment 2

[William Chen [:wchen]]

Attachment: Bug 798065 - Fix integer underflow in nsTArray::LastIndexOf.

No code changes. Added r=cjones to patch description.

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 668526 [details] [diff] [review]
Bug 798065 - Fix integer underflow in nsTArray::LastIndexOf.

You don't need to request re-review for that.

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8c53914f34b0

Should this have a test?

Comment 5

[William Chen [:wchen]]

(In reply to Ryan VanderMeulen from comment #4)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/8c53914f34b0
> 
> Should this have a test?

There is a compiled code test included in the patch.

Comment 6

[[e-mail]]

(In reply to William Chen from comment #0)
> start = Length() - 1;
So if the code had been written iter = end + (start + 1) then it would have been correct?

Comment 7

[William Chen [:wchen]]

(In reply to [e-mail] from comment #6)
> (In reply to William Chen from comment #0)
> > start = Length() - 1;
> So if the code had been written iter = end + (start + 1) then it would have
> been correct?

Yes

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/8c53914f34b0