Skip to content

a fresh node startup has populated RegExp statics #43740

Closed
#43741
Closed
a fresh node startup has populated RegExp statics#43740
#43741

Description

@ljharb
ljharb
opened on Jul 8, 2022

Version

v18.5.0

Platform

n/a

Subsystem

No response

What steps will reproduce the bug?

node -pe 'RegExp.$_' will print out WeakRef.

node, and then RegExp.$_, will print out '\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00'.

This also applies to $&.

How often does it reproduce? Is there a required condition?

No response

What is the expected behavior?

No response

What do you see instead?

I expect the empty string for all RegExp statics.

This is a problem for a few reasons:

  1. it exposes internal regular expression usage in node core
  2. the value might change from any version to any version, so if anyone's foolish enough to rely on it, they'll break in a non-major
  3. it's unseemly

If there's a point, right before node starts running user code, and right before the repl becomes available, then we could execute a trivial regular expression operation to clear all the statics.

Alternatively, this may be exposing non-primordial regex usage, and it's likely that switching to primordials (since I believe they'd update the statics on the primordial RegExp instead of the global RegExp) would solve this as well.

Additional information

No response

Activity

  1. ljharb commented on Jul 8, 2022

    @ljharb
    ljharb
    on Jul 8, 2022
    SponsorMemberAuthor

    cc @aduh95

  2. hemanth commented on Jul 8, 2022

    @hemanth
    hemanth
    on Jul 8, 2022
    Contributor

    I would like to work on the fix for this. Looks like it is a side of this.

  3. ljharb commented on Jul 8, 2022

    @ljharb
    ljharb
    on Jul 8, 2022
    SponsorMemberAuthor

    ah, maybe the statics are populated by the same realm then :-/ that would be great for the repl, but wouldn't address node itself.

  4. richardlau commented on Jul 8, 2022

    @richardlau
    richardlau
    on Jul 8, 2022
    Member

    Duplicate of #18931?

  5. ljharb commented on Jul 8, 2022

    @ljharb
    ljharb
    on Jul 8, 2022
    SponsorMemberAuthor

    The repl part, yes - but not the node -pe part.

  6. aduh95 commented on Jul 8, 2022

    @aduh95
    aduh95
    on Jul 8, 2022
    Contributor

    it's likely that switching to primordials (since I believe they'd update the statics on the primordial RegExp instead of the global RegExp) would solve this as well.

    Before user code is executed, primordials.RegExp === globalThis.RegExp, so that wouldn't help. Creating an empty RegExp sounds fine though.

    @hemanth if you want to work on the REPL side, that'd be great, I have a fix ready for the other problem.

  7. hemanth commented on Jul 8, 2022

    @hemanth
    hemanth
    on Jul 8, 2022
    Contributor

    @aduh95 Trying to understand the intent behind this.

    regExMatcher.exec(savedRegExMatches.join(sep)) results in:

    [
  '\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00',
  '',
  '',
  '',
  '',
  '',
  '',
  '',
  '',
  '',
  index: 0,
  input: '\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00',
  groups: undefined
]

  8. aduh95 commented on Jul 8, 2022

    @aduh95
    aduh95
    on Jul 8, 2022
    Contributor

    My understanding is it's trying to clear up whatever RegEx node internals may have run between two REPL entries if that makes sense.

    1. The user enters a regex operation, and hit ENTER to eval their entry.
    2. Node.js internals run other regex operations that overrides RegExp statics.
    3. The user expect to find RegExp statics in the same state their original operation should have left them.

    I think that works for RegExp.$1 to RegExp.$9, but not the other ones. Not sure this could be achieve without re-running the user code.

  9. hemanth commented on Jul 8, 2022

    @hemanth
    hemanth
    on Jul 8, 2022
    Contributor

    Node.js internals run other regex operations that overrides RegExp statics.

    If we use primordial SafeRegExp we can avoid this side effect?

  10. aduh95 commented on Jul 8, 2022

    @aduh95
    aduh95
    on Jul 8, 2022
    Contributor

    Using SafeRegExp is not very practical because that would mean we can no longer use regex literals in core. Trying to migrate all of core to SafeRegExp seems like an unreasonable amount of work.

  11. hemanth commented on Jul 9, 2022

    @hemanth
    hemanth
    on Jul 9, 2022
    Contributor

    @aduh95 Hmm, We should revive #20549 ?

  12. aduh95 commented on Jul 9, 2022

    @aduh95
    aduh95
    on Jul 9, 2022
    Contributor

    From my point of view #20549 (comment) and #20549 (comment) is a good TL;DR for this thread. I'd say let's try this approach and run benchmark to see how it performs. IMO it would be worth creating a SafeRegExp class in primordials rather than the getInternalGlobal hack.

    I wonder if ShadowRealms integration gives a new solution to this problem, allowing us to run REPL code on another Realm sounds like something that should also fix the issue. Should we move this discussion to #18931 since only the REPL is at stake now?

  13. benjamingr commented on Jul 9, 2022

    @benjamingr
    benjamingr
    on Jul 9, 2022
    Member

    Isn't RegExp.$_ a standard web-compatibility legacy feature? Spec wise are we safe to remove and not support it?

  14. aduh95 commented on Jul 9, 2022

    @aduh95
    aduh95
    on Jul 9, 2022
    Contributor

    It's implemented by V8, I'm sure we can easily disable it – but yes, as far as ECMAScript is concerned, it doesn't exist.

  15. 6 remaining items

  16. aduh95
    added a commit that references this issue on Aug 4, 2022

    lib: reset `RegExp` statics before running user code

    55ba397
  17. aduh95
    closed this as completedin #43741on Aug 5, 2022
  18. aduh95
    added a commit that references this issue on Aug 5, 2022

    lib: reset `RegExp` statics before running user code

    464d1c1
  19. aduh95
    added a commit that references this issue on Aug 23, 2022

    lib: reset `RegExp` statics before running user code

    aa76869
  20. RafaelGSS
    added 2 commits that reference this issue on Sep 23, 2022

    lib: reset `RegExp` statics before running user code

    687ffcc

    lib: reset `RegExp` statics before running user code

    d8d34ae
  21. juanarbol
    added 5 commits that reference this issue on Oct 3, 2022

    lib: reset `RegExp` statics before running user code

    7b659bc

    lib: reset `RegExp` statics before running user code

    6654b00

    lib: reset `RegExp` statics before running user code

    ebc76b6

    lib: reset `RegExp` statics before running user code

    ac8d2de
  22. guangwong
    added 2 commits that reference this issue on Jan 3, 2023

    lib: reset `RegExp` statics before running user code

    8510d39

    lib: reset `RegExp` statics before running user code

    a2d55ae
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions