Skip to content

Freeze Regexp literals - #2705

Merged
mame merged 1 commit into
ruby:masterfrom
Shopify:freeze-regexp-literal
Jan 15, 2020
Merged

Freeze Regexp literals#2705
mame merged 1 commit into
ruby:masterfrom
Shopify:freeze-regexp-literal

Conversation

@casperisfine

@casperisfine casperisfine commented Nov 27, 2019

Copy link
Copy Markdown
Contributor

Since Regexp literals always reference the same instance,
allowing to mutate them can lead to state leak.

The following script:

def mutate
  re = /foo/
  state = re.instance_variable_get(:@state)
  re.instance_variable_set(:@state, state.to_i + 1)
  state
end

3.times do
  p mutate
end

Output this on 2.6:

nil
1
2

IMHO, you shouldn't be able to mutate an unduplicated literal.

cc @methodmissing

@casperisfine
casperisfine force-pushed the freeze-regexp-literal branch 4 times, most recently from 0aae08e to 16471e9 Compare November 27, 2019 13:00
@tenderlove

tenderlove commented Nov 27, 2019

Copy link
Copy Markdown
Member

This makes sense to me. Instructions are clearly using a putobject:

irb(main):001:0> RubyVM::InstructionSequence.new "def foo(x); x =~ /bar/; end"
=> <RubyVM::InstructionSequence:<compiled>@<compiled>:1>
irb(main):002:0> puts _.disasm
== disasm: #<ISeq:<compiled>@<compiled>:1 (1,0)-(1,27)> (catch: FALSE)
0000 definemethod                           :foo, foo                 (   1)[Li]
0003 putobject                              :foo
0005 leave

== disasm: #<ISeq:foo@<compiled>:1 (1,0)-(1,27)> (catch: FALSE)
local table (size: 1, argc: 1 [opts: 0, rest: -1, post: 0, block: -1, kw: -1@-1, kwrest: -1])
[ 1] x@0<Arg>
0000 getlocal_WC_0                          x@0                       (   1)[LiCa]
0002 putobject                              /bar/
0004 opt_regexpmatch2                       <calldata!mid:=~, argc:1, ARGS_SIMPLE>
0006 leave                                  [Re]
=> nil

This is technically not backwards compatible, so I think we may need to discuss more (at least get a 👍 from matz). Can you open a ticket in Redmine?

@amatsuda

amatsuda commented Nov 28, 2019

Copy link
Copy Markdown
Member

This ticket already exists. https://bugs.ruby-lang.org/issues/8948
Please just add a comment there so we can move forward!

@byroot

byroot commented Nov 28, 2019

Copy link
Copy Markdown
Member

This ticket already exists. https://bugs.ruby-lang.org/issues/8948

I didn't know about it so I created https://bugs.ruby-lang.org/issues/16377 right after this PR.

(at least get a 👍 from matz).

I'll add it to the developers meeting ticket.

@byroot

byroot commented Nov 28, 2019

Copy link
Copy Markdown
Member

Actually according to https://bugs.ruby-lang.org/issues/8948#note-14 Matz already agreed to freeze regexp literals.

The discussion was still ongoing because some wanted to freeze all regexps.

Not sure if we can rely on a 2 years old 👍 from Matz though.

@mame

mame commented Dec 26, 2019

Copy link
Copy Markdown
Member

Hi @casperisfine @byroot

Could you rebase the pull request onto master?
And, I'd like you to add [Feature #8948] [Feature #16377] to the commit message. The references will close the tickets automatically when merged.

After that, I'll merge this.

@casperisfine
casperisfine force-pushed the freeze-regexp-literal branch 2 times, most recently from ecb7202 to 57fc058 Compare December 26, 2019 17:09
@casperisfine

casperisfine commented Dec 26, 2019

Copy link
Copy Markdown
Contributor Author

@mame done!

@byroot
Freeze Regexp literals
c43f99f 
[Feature ruby#8948] [Feature ruby#16377]

Since Regexp literals always reference the same instance,
allowing to mutate them can lead to state leak.
@casperisfine
casperisfine force-pushed the freeze-regexp-literal branch from 57fc058 to c43f99f Compare January 6, 2020 11:53
@casperisfine

casperisfine commented Jan 6, 2020

Copy link
Copy Markdown
Contributor Author

Another conflict happened, so I rebased again.

@mame
mame merged commit 98ef38a into ruby:master Jan 15, 2020
@mame

mame commented Jan 15, 2020

Copy link
Copy Markdown
Member

Sorry for my late action. Thank you for your contribution!

@byroot

byroot commented Jan 15, 2020

Copy link
Copy Markdown
Member

No worries, thanks for merging.

@casperisfine casperisfine mentioned this pull request Dec 6, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@casperisfine @tenderlove @amatsuda @byroot @mame