Infinite loop resulting in stack level too deep in Sinatra::Response#finish #612

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
@omikronn

If the call to super on line 86 returns a Rack::BodyProxy instance inside result, the equality comparison on line 87 fails. The end result will be an infinite loop when calling #each on body, which will cause a stack overflow.

Rather than add || Rack::BodyProxy === result on line 86, I suggest we overload the "==" operator in Sinatra::Response, to perform compare instances based solely on the status, headers, body and length attribute (of course, the comparison on line 86 needs to be flipped to self == result).

Sinatra::Response#== overloaded to perform comparisons based only on …
…the status, headers, body and length attributes
+ return false unless other.respond_to?(attribute)
+ return false unless self.__send__(attribute) == other.__send__(attribute)
+ end
+ true

This comment has been minimized.

Show comment Hide comment
@rkh

rkh Jan 13, 2013

Owner

seems bad

@rkh

rkh Jan 13, 2013

Owner

seems bad

This comment has been minimized.

Show comment Hide comment
@omikronn

omikronn Jan 14, 2013

Could you please elaborate a bit more?

It's one of the first the first times I'm submitting a pull request instead of packaging/releasing a hotfixed gem on my own gem server in prod, so I want to get a hang of this and turn contributing into a habit.

Are there any other attributes I should be checking equality for? Is there anything wrong with this style of coding (am I breaking any guidelines)? Is there something conceptually wrong with this snippet/approach that I'm not seeing?

Thanks in advance for an answer.

@omikronn

omikronn Jan 14, 2013

Could you please elaborate a bit more?

It's one of the first the first times I'm submitting a pull request instead of packaging/releasing a hotfixed gem on my own gem server in prod, so I want to get a hang of this and turn contributing into a habit.

Are there any other attributes I should be checking equality for? Is there anything wrong with this style of coding (am I breaking any guidelines)? Is there something conceptually wrong with this snippet/approach that I'm not seeing?

Thanks in advance for an answer.

This comment has been minimized.

Show comment Hide comment
@rkh

rkh Jan 14, 2013

Owner

Ah, sorry. It does not seem like good practice to me to 1. return true for == if the other is a body proxy and 2. call private methods on the body proxy in order to do so.

@rkh

rkh Jan 14, 2013

Owner

Ah, sorry. It does not seem like good practice to me to 1. return true for == if the other is a body proxy and 2. call private methods on the body proxy in order to do so.

@rkh

This comment has been minimized.

Show comment Hide comment
@rkh

rkh Jan 14, 2013

Owner

Do you have a test case or example code for me to reproduce this?

Owner

rkh commented Jan 14, 2013

Do you have a test case or example code for me to reproduce this?

@tobowers

This comment has been minimized.

Show comment Hide comment
@tobowers

tobowers Jan 18, 2013

We (@bglusman and I) have a small fix that worked for us: #615

We (@bglusman and I) have a small fix that worked for us: #615

@rkh rkh closed this Jan 26, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment