Crystal: Missing check in HTTP::Content

[bararchy]

The .not_nil! in https://github.com/crystal-lang/crystal/blob/master/src/http/content.cr#L137 is not safe, apparently it can be nil:

Exception:Nil assertion failed Nil assertion failed (Exception)
  from /usr/lib/crystal/class.cr:66:0 in 'not_nil!'
  from /usr/lib/crystal/http/content.cr:0:26 in 'read_chunk_start'
  from /usr/lib/crystal/http/content.cr:65:9 in 'read'
  from /usr/lib/crystal/io/encoding.cr:74:45 in 'read'
  from /usr/lib/crystal/io.cr:554:11 in 'gets_to_end'
  from /usr/lib/crystal/http/client/response.cr:73:15 in 'consume_body_io'
  from /usr/lib/crystal/http/client/response.cr:97:9 in 'from_io?'
  from /usr/lib/crystal/http/client.cr:502:5 in 'exec_internal_single'
  from /usr/lib/crystal/http/client.cr:486:5 in 'exec_internal'
  from /usr/lib/crystal/http/client.cr:482:5 in 'exec'
  from /usr/lib/crystal/http/client.cr:585:5 in 'exec'
  from ???

[ysbaddaden]

Sounds like an invalid assumption (of parsed chunked) or an invalid chunk encoded input.

[RX14]

We need a reproducable test case to see what's going on here.

[bararchy]

I'll try and capture the response from server which causes this

[straight-shoota]

It seems to be quite clear that calling not_nil! on @io.gets can cause errors if there is no more data in the stream to read. Then this will inevitably fail but shouldn't raise a nil assertion. This needs to be handled appropriately.

Now, this will obviously need investigation whether it is cause by an invalid response or if there is a bug in the Crystal implementation.

[bararchy]

@straight-shoota I think there is a bug, if only because this should not have happened and yet it did.
When using .not_nil! you say to the compiler "this will never be nil", if that's not the case (as we can see above), there is most defiantly and issue.
nonetheless I'll try and capture the response that causes this issue

[straight-shoota]

Yeah, sure. Using not_nil! there is obviously wrong as IO#gets could always be nil.
The question is whether the IO being at the end (which seems not to be expected at this moment) is due to an invalid data format or because the Crystal implementation is incorrect.

[bararchy]

Oh, in that case I can't really tell, I'm working on recreating the issue, but i'm 95% it's data corruption due to the nature of what we do (fuzzing)
So, I'll try and share the pcap ASAP

[Timbus]

I ran into this. To reproduce you just start a chunked transfer from something, and then end the stream (TCP FIN) before sending the final empty chunk. This would be a common scenario for long running streams.

To reproduce: Setup socat or nc as a listener with a dummy return payload:

printf 'HTTP/1.1 200 OK\r
Content-Type: text/plain\r
Transfer-Encoding: chunked\r\n\r
3\r
Foo\r\n' | socat stdin tcp-l:4321,reuseaddr

Note the missing '0\r\n\r\n'
Run this crystal app:

require "http/client"
HTTP::Client.get("http://localhost:4321/") do |response|
  puts "Got a response"
  while line = response.body_io.gets
    puts line
  end
end