String.prototype.lastIndexOf needs to evaluate both arguments

RESOLVED FIXED in Firefox 52

Status

()

RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: anba, Assigned: till)

Tracking

Trunk
mozilla52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Test case:
---
"".lastIndexOf("a", {valueOf:function(){throw 0}})
---

Expected: second argument is evaluated and `0` is thrown

Actual: lastIndexOf() returns -1
(Assignee)

Comment 1

5 years ago
Created attachment 788242 [details] [diff] [review]
never skip evaluating String#lastIndexOf's second argument.

Yeah, we're a bit over-eager, here. Easy enough to fix, though.
Attachment #788242 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

5 years ago
Assignee: general → till
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Reporter)

Comment 2

5 years ago
`"".lastIndexOf("\0", 0)` returns 0 instead of -1 with the attached patch. 

Actually even `"".lastIndexOf("\0" + "\uDADA".repeat(7), -1)` returns 0 for me in a debug build, but I cannot tell whether it's always 0xDADA, because I don't know which magic numbers are used in SpiderMonkey. So basically it's possible to access memory after `textstr`.
(Assignee)

Comment 3

5 years ago
Comment on attachment 788242 [details] [diff] [review]
never skip evaluating String#lastIndexOf's second argument.

Clearly, I'm not smart enough to do this without painstakingly following the spec steps. *sigh*.

I'll upload a new patch doing just that once I get around to it. That won't be before my pto next week, though.
Attachment #788242 - Flags: review?(jwalden+bmo)
(Reporter)

Updated

3 years ago
Component: JavaScript Engine → JavaScript: Standard Library
(Reporter)

Comment 4

2 years ago
Created attachment 8802257 [details] [diff] [review]
bug903391.patch

New patch. Also added steps references and renamed variables to match current spec.
Attachment #788242 - Attachment is obsolete: true
Attachment #8802257 - Flags: review?(till)
(Assignee)

Comment 6

2 years ago
Comment on attachment 8802257 [details] [diff] [review]
bug903391.patch

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

Excellent, thanks.

::: js/src/jsstr.cpp
@@ +1756,5 @@
>          return true;
>      }
> +    MOZ_ASSERT(0 <= start && size_t(start) < len);
> +
> +    JSLinearString* text = str->ensureLinear(cx);

Not in this bug, but I wonder if it'd make sense to not require a linear string here and operate on ropes instead. Could well be that in any relevant case the cost of linearization is amortized pretty quickly, though.
Attachment #8802257 - Flags: review?(till) → review+
(Reporter)

Updated

2 years ago
Keywords: checkin-needed

Comment 7

2 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f0a1da93428
Evaluate both arguments in String.prototype.lastIndexOf. r=till
Keywords: checkin-needed

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6f0a1da93428
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.