834989 - Use of RegExp in self-hosted code can be observed through RegExp statics

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Norbert Lindenberg]

Attachment: Test case - do not check in

The use of RegExp methods or of regular expressions in String methods can be observed by client code through "RegExp statics", properties of the RegExp constructor. These properties are documented at
https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Deprecated_and_obsolete_features#RegExp_Properties

The attached test case calls self-hosted functions that use the RegExp methods test and exec, and the String method match, each separately with regular expressions that include or don't include capturing parentheses. The test output shows that all methods change at least some of the properties of the RegExp constructor; more properties if capturing parentheses are used.

The use of regular expressions within self-hosted code should not be observable by client code at all.

Comment 1

[Norbert Lindenberg]

Attachment: Use alternatives to RegExp.p.exec and RegExp.p.test that don't modify RegExp statics

This patch adds intrinsics regexp_exec_no_statics and regexp_test_no_statics that self-hosted code can use instead of RegExp.prototype.exec and RegExp.prototype.test and that don't modify the RegExp statics.

The patch does not add such intrinsics for the String methods that accept RegExp arguments:
- String.prototype.match can be trivially replaced with regexp_exec_no_statics.
- String.prototype.replace has so many different usage patterns that inserting a str_replace_no_statics would require serious testing, and the only call in existing self-hosted code was easy to rewrite based on regexp_exec_no_statics.
- String.prototype.split has no caller in current self-hosted code that passes in a RegExp object.
- String.prototype.search has no caller in current self-hosted code at all.

Comment 2

[Norbert Lindenberg]

Attachment: Updated test case - do not check in

This updated test case uses the new intrinsics instead of RegExp.prototype.exec and RegExp.prototype.test. It passes for the four individual tests testing the new intrinsics; still fails for the two individual tests testing String.prototype.match.

Note that the real test for the new intrinsics will be the ECMA-402 conformance tests testing the behavior relying on the intrinsics - see bug 854320.

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 769977 [details] [diff] [review]
Use alternatives to RegExp.p.exec and RegExp.p.test that don't modify RegExp statics

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

::: js/src/builtin/RegExp.h
@@ +23,5 @@
>  namespace js {
>  
>  RegExpRunStatus
>  ExecuteRegExp(JSContext *cx, HandleObject regexp, HandleString string,
> +              MatchConduit &matches, bool setStatics);

We try to avoid having unadorned bools like this, when it's not obvious at the call site what the meanings are.  (setVisible(true/false) is obvious; ExecuteRegExp(..., true/false) is not.)  Please add something like (better name suggestions welcome):

  enum AffectsRegExpStatics { UpdateRegExpStatics, DontUpdateRegExpStatics };

and use that as the argument type here.

Comment 4

[Norbert Lindenberg]

Attachment: Use alternatives to RegExp.p.exec and RegExp.p.test that don't modify RegExp statics

Updated per comment 3. Carrying r+jwalden.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 773353 [details] [diff] [review]
Use alternatives to RegExp.p.exec and RegExp.p.test that don't modify RegExp statics

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c000e0df112

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9c000e0df112