ActionDispatch::Routing::RouteSet::Generator mutates request.path_parameters

[bquorning]

I’m having some trouble upgrading to Rails 5.0.0.beta3, and it looks like #21057 is to blame.

In a controller, I can trigger an ActionController::UrlGenerationError exception by calling e.g. root_path right before calling url_for. In a show action, I would get something like this message: No route matches {:action=>"index", :controller=>"foo", :id=>"42"}. Notice how url_for thinks the current show action is an index action… This is because ActionDispatch::Routing::RouteSet::Generator is mutating request.path_parameters.

The commits 1993e2c and 1a14074 remove the dup of the recall and options hashes, and changes a subsequent call to @recall.delete.

But there are still several ways to mutate the original objects. I’m seeing these calls:

@options.delete
@options[:action]= 
@options[:controller]=
@options[key]=
@recall[:action]=

My problems are fixed by changing only @recall = recall into @recall = recall.dup, but I would rather we fixed the mutation problems instead.

r? @schneems

[maclover7]

Can you create an executable test script using one of the templates here, that would demonstrate the problem you are seeing?

[bquorning]

I've tried writing a test case, and failed. And will absolutely try again :-)

But just looking at the Generator code, it's pretty obviously buggy. Even the linked commits’ messages and that PRs comments mention the risk of introducing subtle bugs.

[bquorning]

Executable test script here: https://gist.github.com/bquorning/83a7689c50500500fd94773c14808b74

I’ve marked the 1 line you can remove to make the test pass.

[maclover7]

Bug script fails for me on master. Would you be willing to work on a pull request for this?

[bquorning]

The options attribute is probably ok not to dup, as @schneems wrote in 1a14074:

This change removes one dup, since it's literally right after we just dup-d the hash to pass into this constructor.

For the recall attribute, I can see two ways of fixing it:

1. Call dup in the initializer. Loses the performance benefits described in 1993e2c.
2. Substitute recall for a dupe right before mutating it. Should keep most of those performance gains intact.

I’ll try going with 2).

[pixeltrix]

@bquorning this a duplicate of #23019 - I've tried doing a fix in #23103 but found some regressions in the changes that aren't tested in the current test suite. Will take another look at it this week.

[bquorning]

I think a fix might be as easy as calling @recall = @recall.dup right before mutating here:

def normalize_action!
   if @options[:action] == 'index'.freeze
     @recall[:action] = @options.delete(:action)
   end
 end

Test coverage is pretty sparse, though, so I'm not sure if this introduces new regressions.

[pixeltrix]

@bquorning yeah, I sure we could just dup something but I'd rather we fixed it properly 😄

[bquorning]

Yeah, I agree. My observation is just that @recall has the same object_id as the controller's request.path_parameters.

[schneems]

Thanks for the report. I was OOO last week. I'll try to take a look soon. That hash dup was pretty expensive :(

[TikiTDO]

@schneems Are you sure the recall dup was expensive? I just tested the example script above with memory_profile, both with and without dup, and the difference was 300 bytes. Is there a test case where this recall hash is significantly bigger?

[bquorning]

I think the concern is that the dup happens for every URL generation on the page, and the dup'ed hashes are identical (except they're not, since we mutate them – I'm confused).

[TikiTDO]

I did some investigation on the code base, and here are my results. Please let me know if this makes sense:

• Investigation
  • ActionDispatch::Routing::RouteSet#url_for calls into ActionDispatch::Routing::RouteSet#generate here
  • ActionDispatch::Routing::RouteSet#generate creates an ininialized ActionDispatch::Routing::RouteSet::Generator, and calls #generate here
    • This is the only place in the code base where ActionDispatch::Routing::RouteSet::Generator is used
  • ActionDispatch::Routing::RouteSet::Generator#generate calls ActionDispatch::Journey::Formatter#generate here
    • No Problems:
      • ActionDispatch::Routing::RouteSet::Generator#generate dose constraints = path_parameters.merge(options) which is equivalent to constraints = recall.merge(options) here
      • ActionDispatch::Routing::RouteSet::Generator#generate calls ActionDispatch::Routing::RouteSet::Generator#extract_parameterized_parts which does parameterized_parts = recall.merge(options) here
    • Possible Problems:
      • Problem 1: ActionDispatch::Routing::RouteSet::Generator#extract_parameterized_parts does route.parts.reverse_each.drop_while { |part| !options.key?(part) || (options[part] || recall[part]).nil?} here
      • Problem 2: ActionDispatch::Routing::RouteSet::Generator#generate iterates does params = options.dup.delete_if do |key, _| here
• Issues
  • ActionDispatch::Routing::RouteSet::Generator writes to @recall in two places: normalize_recall! and normalize_action!
    • Root Cause 1: normalize_recall! does @recall[:action] ||= 'index'.
    • Root Cause 2: normalize_action! does @recall[:action] = @options.delete(:action) if @options[:action] == 'index'.freeze (The freeze is also not useful here)
• Conclusion
  • With the current setup, during ActionDispatch::Routing::RouteSet::Generator#generate if the :action is index, then recall will always have the :action parameter, while options will not. Otherwise recall will remain unchanged.
  • Fix 1 for Problem 1: Since option should never have an :action parameter, the block should always return true if (part == :action && options[:action] == 'index)
  • Fix 2 for Problem 2: Since option should never have an :action parameter, the delete_if block can check if (part == :action && options[:action] == 'index)
  • Removing Root Cause 1 and 2, and adding Fix 1 and 2 should maintain identical behavior characteristics, while not needing the dup

[jeremy]

Fix in #23103