Skip to content

Conversation

@ioquatix
Copy link
Member

@ioquatix ioquatix commented Feb 20, 2025

I was running irb over a drb-like system implemented on Async.

The problem is, some calls may be transparently sent over the network, including obj.pp.

In such cases, push_inspect_key could be called without Thread.current[:__recursive_key__][:inspect] being set up correctly, resulting in NoMethodError.

This change ensures that if we are printing an object, the state is set correctly by introducing guard_inspect(object) do ... end. This is an alternative to push_inspect_key and pop_inspect_key that (1) ensures the thread state is set up correctly and (2) can be implemented slightly more efficiently because we don't need to pop the state if the :inspect key is going to be discarded.

This change retains compatibility with all the existing methods.

@ioquatix
Copy link
Member Author

ioquatix commented Feb 21, 2025

I was able to create a test that reproduces the issue, but it's A/B as in, it was too tricky to create an isolated test using a distributed object system.

However, here is the same error I'm experiencing:

> ruby ./test.rb
/Users/samuel/.rubies/ruby-3.4.2/lib/ruby/3.4.0/pp.rb:183:in 'PP::PPMethods#pop_inspect_key': undefined method 'delete' for nil (NoMethodError)

      Thread.current[:__recursive_key__][:inspect].delete id
                                                  ^^^^^^^
	from /Users/samuel/.rubies/ruby-3.4.2/lib/ruby/3.4.0/pp.rb:209:in 'PP::PPMethods#pp'
	from ./test.rb:7:in '<main>'
/Users/samuel/.rubies/ruby-3.4.2/lib/ruby/3.4.0/pp.rb:178:in 'PP::PPMethods#push_inspect_key': undefined method '[]=' for nil (NoMethodError)

      Thread.current[:__recursive_key__][:inspect][id] = true
                                                  ^^^^^^
	from /Users/samuel/.rubies/ruby-3.4.2/lib/ruby/3.4.0/pp.rb:202:in 'PP::PPMethods#pp'
	from ./test.rb:7:in '<main>'

It happens from the following test:

require "pp"

a = []
a << a

q = PP::SingleLine.new($stderr)
q.pp(a)
q.flush

Now the current design expects users to write this:

q.guard_inspect_key{q.pp(a)}

But it's possible that even if you execute that code, q.pp(a) may be executed on a different thread or even a different process (remote object), in which Thread.current[:__recursive_key__][:inspect] isn't yet set up resulting in NoMethodError. By ensuring it's set up in guard_key, we avoid this problem.

@ioquatix ioquatix force-pushed the guard_inspect_key-safety branch from d12c269 to 928aa10 Compare February 21, 2025 00:46
@ioquatix ioquatix marked this pull request as ready for review February 21, 2025 00:47
@ioquatix ioquatix requested review from akr and nobu February 21, 2025 00:47
@ioquatix ioquatix merged commit 5b5d483 into ruby:master Feb 25, 2025
25 checks passed
@ioquatix ioquatix deleted the guard_inspect_key-safety branch February 25, 2025 03:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants