avoid multiple errors even if `params` contains special values #1526

namusyaka · 2019-04-11T08:31:02Z

The original fixes are #1506 and #1517.
This commit considers FrozenError and TypeEror for the case.

Object#dup has a possibility of raising TypeError when ruby <= 2.3 and the receiver is a special const.
What's special const? It depends on the ruby version. Currently, it's defined here.
In the ruby-2.3.8, it's defined in other place.

To fix this issue, I had an option which is managing the special const mapping in our code, but it's very hard to manage, and there is a possibility of increasing complexity. . So I would like to handle the issue by rescueing TypeError in this case.

What do you think? Any opinions or any ideas?

dentarg · 2019-04-11T09:01:41Z

test/middleware_test.rb

+      req = Rack::Request.new(env)
+      req.update_param('s', :s)
+      req.update_param('i', 1)
+      req.update_param('c', 3.to_c)


also add Rational to this list? (1/1).to_r

What's the motivation? Fully testing special consts?
If so, it's the same complexity to have that as I mentiond. We need to add more examples such as BigDecimal, Float etc. I think it's not reasonable.
Otherwise, lmk.

I see, never mind then

namusyaka · 2019-04-11T17:33:30Z

To narrow the rescue scope down, I changed the fix a bit.

jkowens · 2019-04-12T13:54:31Z

lib/sinatra/base.rb

@@ -1089,7 +1089,16 @@ def invoke

    # Dispatch a request with error handling.
    def dispatch!
-      @params.merge!(@request.params).each { |key, val| @params[key] = val && force_encoding(val.dup) }
+      @params.merge!(@request.params).each do |key, val|


Can this logic be moved into the force_encoding method or its own method?

force_encoding should only be responsible for force encoding. I was thinking about moving it into its own method, like dup..., but I couldn't imagine that the change leads to bring happiness to us. Can you elaborate about the motivation?

I think extracting a method would help explain the need for the slightly unusual looking code. This all started to avoid a FrozenError right? The code doesn't communicate that. I'm not entirely sure if I understand the force_encoding method but take a look at this line:

sinatra/lib/sinatra/base.rb

Line 1756 in 7b7043c

if data.respond_to? :force_encoding

It looks like the root issue is that a frozen string responds to force_encoding. I think the following would be a legitimate update to that method:

if data.respond_to? :force_encoding data = data.dup if data.frozen? data.force_encoding(encoding).encode!

I don't agree with the change on force_encoding because of two reasons.

force_encoding is only responsible for force encoding. In other words, it doesn't mean duping value, it should be managed in caller place, right? For example, force_encoding is also called in other places such as:

sinatra/lib/sinatra/base.rb

Line 1026 in 7b7043c

force_encoding(params)

sinatra/lib/sinatra/base.rb

Line 1314 in 7b7043c

force_encoding data, encoding

In general, our implementation does not always depend on the returned value. Using dup can only effect the returned value. Adopting your idea effects not only this issue, but other non-issue code. For example, a similar change with your idea had already been tested in Fix for FrozenError when force_encode a frozen string literal #1479, and got some failures.
Of course, your code won't reproduce the failure, but the change has a possibility that leading the similar issue in the future.

For the above reasons, I wouldn't like to dup the value inside force_encoding.

I think extracting a method would help explain the need for the slightly unusual looking code.

Yeah I knew. But I couldn't imagine good name for that, and I don't think this is an unusual looking code. However, if you think so, we need to think about improving the readability.
I think we have two options for that:

Extracting method.

Adding comments

What do you think?

Shouldn't force_encoding guard or handle invalid scenarios? I think the fact that it is called in multiple locations only adds to that need. Are we sure that a FrozenError isn't a possible issue for them as well?

Anyway, I think a descriptive method name might be dup_frozen_params:

def dup_frozen_params(params) params.each do |key, val| if val.frozen? params[key] = begin val = val.dup rescue TypeError val end end end end

@params.merge!(@request.params) dup_frozen_params(@params) force_encoding(@params)

I'm starting to wonder if this is necessary at all. What if we just changed the following line:

sinatra/lib/sinatra/base.rb

Line 1756 in 7b7043c

if data.respond_to? :force_encoding

# This seems like a fair guard right? if !data.frozen && data.respond_to? :force_encoding

Is there a need to force encode copies of frozen params? We could just leave it at that ;)

No, I did not say that.

If it is invalid to call force_encoding on a particular object, then I think we should account for that. Hence my last suggestion...a really simple fix. Don't dup at all and don't attempt to force_encode frozen strings.

sinatra/lib/sinatra/base.rb

Line 1756 in 7b7043c

if data.respond_to? :force_encoding

# This seems like a fair guard right? if !data.frozen? && data.respond_to? :force_encoding

Also, if that change is made it will be easy to write a method dup_frozen_params which could be called before attempting pass params to force_encoding. That was my main suggestion in that last post.

You meant not frozen but frozen?, right? As I said earlier, it's just skipping force_encoding. In other words, it can't force encoding when the value is frozen.
Why did you think the proposal is correct?
It's not same behavior with the current implementation and my proposal.

mock_app do use Class.new { def initialize(app) @app = app end def call(env) req = Rack::Request.new(env) req.update_param('bar', 'baz'.freeze) @app.call(env) end } set :default_encoding ,'ISO-8859-1' get '/' do params[:bar].encoding.to_s end end get '/' assert_equal 'ISO-8859-1', body

As you can see, the value won't be forced encoding.

If it is invalid to call force_encoding on a particular object, then I think we should account for that.

Yes, exactly. It is not possible at this time to predict perfectly what kind of situation can be considered in the future, but it will be possible to create a method other than force_encoding or to make corrections at the caller as this time, for example.
At the very least, your proposal is focused on skipping exceptions, which is not at all rational.

Also, if that change is made it will be easy to write a method dup_frozen_params which could be called before attempting pass params to force_encoding. That was my main suggestion in that last post.

Again, ignoring frozen objects can not be a solution. It is not rational.

Ok forget that.

How about something like this for extracting a dup method:

def dup_params! @params.each do |key, val| @params[key] = begin val.dup rescue TypeError val end end end # Dispatch a request with error handling. def dispatch! @params.merge!(@request.params) force_encoding(dup_params!)

dentarg · 2019-04-12T19:49:04Z

lib/sinatra/base.rb

+        @params[key] =
+          begin
+            val = val.dup
+          rescue TypeError


What value of val causes #dup to raise TypeError? Played around in irb and couldn't come up with anything.

Ah, this is code for Ruby 2.3:

$ docker run --rm -it ruby:2.3.8-slim-jessie bash root@f19c6072d969:/# irb irb(main):001:0> nil.dup TypeError: can't dup NilClass from (irb):1:in `dup' from (irb):1 from /usr/local/bin/irb:11:in `<main>'

Such as true, false etc.. It can be reproduced in ruby <= 2.3.
See: https://github.com/ruby/ruby/blob/v2_3_7/object.c#L398

The original fixes are #1506 and #1517. This commit considers `FrozenError` and `TypeEror` for the case.

jkowens · 2019-04-23T02:06:33Z

@namusyaka I like your current iteration of this update 👍

dentarg reviewed Apr 11, 2019

View reviewed changes

namusyaka force-pushed the avoid-error branch from 070e3cc to 7cc4b31 Compare April 11, 2019 17:30

jkowens reviewed Apr 12, 2019

View reviewed changes

dentarg reviewed Apr 12, 2019

View reviewed changes

avoid multiple errors even if params contains special values

a542983

The original fixes are #1506 and #1517. This commit considers `FrozenError` and `TypeEror` for the case.

namusyaka force-pushed the avoid-error branch from 7cc4b31 to a542983 Compare April 22, 2019 13:39

namusyaka added the release-note label May 4, 2019

namusyaka added this to the v2.0.6 milestone May 4, 2019

namusyaka merged commit f288b65 into master May 4, 2019

namusyaka deleted the avoid-error branch May 4, 2019 09:18

lewis-fidlers mentioned this pull request Jul 4, 2019

🐛 Resolve the broken builds for older ruby versions Smashing/smashing#141

Merged

dentarg mentioned this pull request Dec 30, 2022

Rack 3 support #1857

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

avoid multiple errors even if `params` contains special values #1526

avoid multiple errors even if `params` contains special values #1526

namusyaka commented Apr 11, 2019

dentarg Apr 11, 2019

namusyaka Apr 11, 2019

dentarg Apr 11, 2019

namusyaka Apr 11, 2019

namusyaka commented Apr 11, 2019

jkowens Apr 12, 2019

namusyaka Apr 12, 2019

jkowens Apr 12, 2019

namusyaka Apr 12, 2019

jkowens Apr 12, 2019

jkowens Apr 12, 2019 •

edited

jkowens Apr 12, 2019 •

edited

namusyaka Apr 12, 2019

namusyaka Apr 12, 2019

jkowens Apr 12, 2019

dentarg Apr 12, 2019

dentarg Apr 12, 2019

namusyaka Apr 12, 2019

jkowens commented Apr 23, 2019

avoid multiple errors even if params contains special values #1526

avoid multiple errors even if params contains special values #1526

Conversation

namusyaka commented Apr 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

namusyaka commented Apr 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkowens Apr 12, 2019 • edited

Choose a reason for hiding this comment

jkowens Apr 12, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkowens commented Apr 23, 2019

avoid multiple errors even if `params` contains special values #1526

avoid multiple errors even if `params` contains special values #1526

jkowens Apr 12, 2019 •

edited

jkowens Apr 12, 2019 •

edited