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.

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

Fastimage raises on broken JPG #152

Closed
fschwahn opened this issue Feb 12, 2024 · 4 comments · Fixed by #153
Closed

Fastimage raises on broken JPG #152

fschwahn opened this issue Feb 12, 2024 · 4 comments · Fixed by #153

Comments

@fschwahn
Copy link

We have a file (I unfortunately cannot share) which causes fastimage to raise the following error:

/Users/fabian/.rbenv/versions/3.2.3/lib/ruby/gems/3.2.0/gems/fastimage-2.3.0/lib/fastimage.rb:518:in `skip': undefined method `size' for #<Net::ReadAdapter> (NoMethodError)

        fetched += new_string.size
                             ^^^^^

and the following backtrace:

/Users/fabian/.rbenv/versions/3.2.3/lib/ruby/gems/3.2.0/gems/fastimage-2.3.0/lib/fastimage.rb:518:in `skip'
/Users/fabian/.rbenv/versions/3.2.3/lib/ruby/gems/3.2.0/gems/fastimage-2.3.0/lib/fastimage.rb:879:in `block in parse_size_for_jpeg'
/Users/fabian/.rbenv/versions/3.2.3/lib/ruby/gems/3.2.0/gems/fastimage-2.3.0/lib/fastimage.rb:849:in `loop'
/Users/fabian/.rbenv/versions/3.2.3/lib/ruby/gems/3.2.0/gems/fastimage-2.3.0/lib/fastimage.rb:849:in `parse_size_for_jpeg'
/Users/fabian/.rbenv/versions/3.2.3/lib/ruby/gems/3.2.0/gems/fastimage-2.3.0/lib/fastimage.rb:434:in `parse_size'
/Users/fabian/.rbenv/versions/3.2.3/lib/ruby/gems/3.2.0/gems/fastimage-2.3.0/lib/fastimage.rb:414:in `parse_packets'
/Users/fabian/.rbenv/versions/3.2.3/lib/ruby/gems/3.2.0/gems/fastimage-2.3.0/lib/fastimage.rb:341:in `block in fetch_using_http_from_parsed_uri'
/Users/fabian/.rbenv/versions/3.2.3/lib/ruby/3.2.0/net/http.rb:2353:in `block in transport_request'
/Users/fabian/.rbenv/versions/3.2.3/lib/ruby/3.2.0/net/http/response.rb:320:in `reading_body'
/Users/fabian/.rbenv/versions/3.2.3/lib/ruby/3.2.0/net/http.rb:2352:in `transport_request'
/Users/fabian/.rbenv/versions/3.2.3/lib/ruby/3.2.0/net/http.rb:2306:in `request'
/Users/fabian/.rbenv/versions/3.2.3/lib/ruby/3.2.0/net/http.rb:2299:in `block in request'
/Users/fabian/.rbenv/versions/3.2.3/lib/ruby/3.2.0/net/http.rb:1570:in `start'
/Users/fabian/.rbenv/versions/3.2.3/lib/ruby/3.2.0/net/http.rb:2297:in `request'
/Users/fabian/.rbenv/versions/3.2.3/lib/ruby/3.2.0/net/http.rb:2177:in `request_get'
/Users/fabian/.rbenv/versions/3.2.3/lib/ruby/gems/3.2.0/gems/fastimage-2.3.0/lib/fastimage.rb:301:in `fetch_using_http_from_parsed_uri'
/Users/fabian/.rbenv/versions/3.2.3/lib/ruby/gems/3.2.0/gems/fastimage-2.3.0/lib/fastimage.rb:283:in `fetch_using_http'
/Users/fabian/.rbenv/versions/3.2.3/lib/ruby/gems/3.2.0/gems/fastimage-2.3.0/lib/fastimage.rb:249:in `initialize'
/Users/fabian/.rbenv/versions/3.2.3/lib/ruby/gems/3.2.0/gems/fastimage-2.3.0/lib/fastimage.rb:143:in `new'
/Users/fabian/.rbenv/versions/3.2.3/lib/ruby/gems/3.2.0/gems/fastimage-2.3.0/lib/fastimage.rb:143:in `size'

I found the following line

new_string = @read_fiber.resume if new_string.is_a? Net::ReadAdapter
which was introduced in 4373182 . This commit was added to fix an issue with webmock, but I'm not using webmock, this also happens when I start a plain irb session and require only fastimage.

Anyway, adding the same line in the skip-method (at line 512) here

fastimage/lib/fastimage.rb

Lines 509 to 519 in f832581

while n > fetched
discarded += @str[@strpos..-1].size
new_string = @read_fiber.resume
raise CannotParseImage if !new_string
new_string.force_encoding("ASCII-8BIT") if new_string.respond_to? :force_encoding
fetched += new_string.size
@str = new_string
@strpos = 0
end

fixes the problem - fastimage no longer raises.

I have no idea what this actually does, so I'm not sure it's a proper fix, but it prevents the error from raising. Maybe also

raise CannotParseImage if !new_string || new_string.is_a? Net::ReadAdapter

would be the right thing to do here.

@sdsykes
Copy link
Owner

sdsykes commented Mar 24, 2024

Surely this is a bug in Net::HTTP. I would love to be able to reproduce this, Net::ReadAdapter is an internal class that shouldn't leak outside of Net::HTTP as far as I can see.
Is there anything more about the circumstances of this bug you can share?

@fschwahn
Copy link
Author

Sorry, nothing else.

It reproduced consistently with the offending image, and the image file was damaged.

@sdsykes
Copy link
Owner

sdsykes commented Mar 27, 2024

Ah no worries, I managed to reproduce, it's because actually the Fiber terminates and just returns whatever, I will fix this correctly shortly.

@fschwahn
Copy link
Author

Great to hear that!

sdsykes added a commit that referenced this issue Mar 28, 2024
Ensure nil is returned from dead fibers, fixes #152
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 a pull request may close this issue.

2 participants