Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Reflective methods should not create symbols for nonexistent elements #5169

Closed
headius opened this issue May 13, 2018 · 2 comments
Closed

Reflective methods should not create symbols for nonexistent elements #5169

headius opened this issue May 13, 2018 · 2 comments
Milestone
JRuby 9.2.6.0

Comments

@headius
Copy link
Member

headius commented May 13, 2018

Currently our logic for reflective methods like Object#instance_variable_get, Struct#[], etc goes like this:

  • If it's a symbol, look up the value
  • If it's a string, turn it into a hard symbol and look up the value

The logic we need to match in MRI is as follows:

  • If it's a symbol, look up the value
  • If it's a string, see if a symbol exists for that string
  • If there is no such symbol, do not create it and do not search (return nil)

MRI only creates symbols for nonexistent key strings passed to the write methods, like instance_variable_set. Our logic has the following issues that should be addressed in some way before 9.2 (or if possible, we should match MRI's logic exactly):

  • Hard symbols will leak. This means arbitrary strings passed into reflective read methods could cause a symbol leak.
  • Soft symbols pollute the symbol table but would not introduce a symbol leak.

In general MRI will assume that if a string is passed in, and no such symbol exists, no such entry can exists in whatever is being searched.
@headius headius added this to the JRuby 9.2.0.0 milestone May 13, 2018
@headius
Copy link
Member Author

headius commented May 13, 2018

We all agree that the hard symbol issue should be fixed for 9.2.

The simple fix is to make them soft, and I mention above that although it would not leak it would temporarily pollute the symbol table with useless entries.

However, the surface area for this case seems very small:

  • You must be doing a reflective read using a string name
  • The name must not exist as an identifer anywhere in the system

And the impact of this would be fairly limited: if it's a check to see if the reflective value has been set, so it can be initialized, it would almost immediately be made a proper hard identifier symbol. So for the 9.2 release I believe the risk is low in the soft symbol option.

@enebo enebo modified the milestones: JRuby 9.2.0.0, JRuby 9.2.1.0 May 24, 2018
@headius
Copy link
Member Author

headius commented Oct 11, 2018

@enebo We should evaluate if this is still really an issue. Your work on symbols landed for 9.2, and the surface area for this bug remains small: you must be accessing non-existing identifiers using the Java-facing reflective methods like instance_variable_get.

The dance that's a problem is the use of validateInstanceVariable, which always creates a symbol whether the variable exists or not.

@headius headius modified the milestones: JRuby 9.2.1.0, JRuby 9.2.2.0 Oct 26, 2018
@enebo enebo modified the milestones: JRuby 9.2.5.0, JRuby 9.2.6.0 Dec 6, 2018
headius added a commit to headius/jruby that referenced this issue Dec 18, 2018
@headius
Validate ivar and const symbols from Java before adding to table.
fd68c5f 
Fixes jruby#5169.
@headius headius mentioned this issue Dec 18, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
JRuby 9.2.6.0
Development

No branches or pull requests
2 participants
@headius @enebo