Catch non-string subject in RegExpExecStub.

Catch non-string subject in RegExpExecStub.

There is no test case to trigger any crash. This is only to guard against the case that the native function is called with unsafe arguments.

Committed: http://code.google.com/p/v8/source/detail?r=10064

[Yang, 2011-11-21 14:27:35]

Please take a look.

This patch adds a guard against non-string subjects when executing regexp in
generated code, something that would only happen if the native function is
called with unsafe arguments. The price is that sliced string subjects are
handled marginally slower. This safeguard used to be in place before the
introduction of string slices.

[Yang, 2011-11-23 14:17:09]

On 2011/11/21 14:27:35, Yang wrote:
> Please take a look.
> 
> This patch adds a guard against non-string subjects when executing regexp in
> generated code, something that would only happen if the native function is
> called with unsafe arguments. The price is that sliced string subjects are
> handled marginally slower. This safeguard used to be in place before the
> introduction of string slices.

This CL is unrelated to the external strings changes btw.

[Lasse Reichstein, 2011-11-24 09:31:57]

I'm not sure this is the correct place to put the safeguard.
If nothing else, it should be earlier in the function, not after doing other
tests that expect a string.

http://codereview.chromium.org/8554004/diff/1/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/8554004/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:4568: // regexp_data: RegExp data (FixedArray)
If r1 contains the instance type, then make a comment about it here with the
other register invariants.

http://codereview.chromium.org/8554004/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:4584: __ tst(ebx, Immediate(kIsNotStringMask));
If it should have been guarded against, then make this an Assert check guarded
by FLAG_debug_code.
And if it there is actually a way to get here with a non-string, it should be
found and eliminated. We control the arguments to stubs completely, and are
expected to handle them safely.
If it's called from %_RegExpExec, then the test should be performed there (all
%-functions must check their arguments thoroughly).

[Yang, 2011-11-24 15:02:10]

On 2011/11/24 09:31:57, Lasse Reichstein wrote:
> I'm not sure this is the correct place to put the safeguard.
> If nothing else, it should be earlier in the function, not after doing other
> tests that expect a string.

If the subject is not a string, it would have kIsNotStringTag set and the tests
checking for sequential string and cons string would fail. We would fall through
until we explicitly check for the kIsNotStringTag. Since we do not expect the
subject not to be a string first place (as this is just a safeguard), checking
it this late would only affect the performance in the case of sliced string.

> 
> http://codereview.chromium.org/8554004/diff/1/src/arm/code-stubs-arm.cc
> File src/arm/code-stubs-arm.cc (right):
> 
>
http://codereview.chromium.org/8554004/diff/1/src/arm/code-stubs-arm.cc#newco...
> src/arm/code-stubs-arm.cc:4568: // regexp_data: RegExp data (FixedArray)
> If r1 contains the instance type, then make a comment about it here with the
> other register invariants.
> 
>
http://codereview.chromium.org/8554004/diff/1/src/arm/code-stubs-arm.cc#newco...
> src/arm/code-stubs-arm.cc:4584: __ tst(ebx, Immediate(kIsNotStringMask));
> If it should have been guarded against, then make this an Assert check guarded
> by FLAG_debug_code.
> And if it there is actually a way to get here with a non-string, it should be
> found and eliminated. We control the arguments to stubs completely, and are
> expected to handle them safely.
> If it's called from %_RegExpExec, then the test should be performed there (all
> %-functions must check their arguments thoroughly).

Whenever %_RegExpExec is called from the natives javascript, the subject is
indeed already converted into a string, but I was under the impression that we
do not completely rely on arguments passed in the natives to be safe. Also, this
string-check has already been there until it was messed up by one of my previous
changes related to string slices. Obviously it had no visible effect since in
fact only strings reach here. Should I turn this into an Assert check?

[Lasse Reichstein, 2011-11-24 17:08:35]

The previous checks indeed, subtly, include the kNotStringTag bit in the
comparison, and should be safe. If doing it this way, at least put a comment
explaining that the lt(r1, ...) comparison can't succeed if the object is a
non-string.

Apart from that, the optimal (IMO) place to check for the input being a string
is in the code for %_RegExpExec, not in the stub that it happens to call.

In either case, LGTM.