Fix flaky test test_horrible_queries (#3857)

tenderlove · web-flow · commit 98ff11d220b5 · 2026-01-12T21:08:17.000-06:00
`test_horrible_queries` can be flaky, but it's very hard to detect.
This commit fixes the flakiness.  To understand the flakiness, let me
put the original test code below:

```ruby
 # then that large mangled field values are caught
10.times do |c|
  get = "GET /#{rand_data(10,120)} HTTP/1.1\r\nX-Test: #{rand_data(1024, 1024+(c*1024), false)}\r\n\r\n"
  assert_raises Puma::HttpParserError do
    parser.execute({}, get, 0)
    parser.reset
  end
end
```

The way I understand `rand_data(xxx, false)` is that the function should
_always_ return invalid bytes (IOW cause the parser to raise an
exception).

However, it is possible for that function to _sometimes_ return valid
bytes.  The above test asserts that an error is raised when
`parser.execute` is called, but if an exception gets raised, then
`parser.reset` isn't called.  If `parser.reset` isn't called, then
subsequent calls to `parser.execute` will raise an exception
_regardless_ of whether the input is parse-able or not.  In other words,
if the first iteration caused an exception, subsequent iterations are
guaranteed to raise an exception.

This commit does two things:

1. Pulls `parser.reset` outside of the assert_raises so that the flake
   is more obvious / reproducible
2. Changes `rand_data` to always add an invalid byte so that if the
   random data happens to contain parsable data, we'll at least have one
   byte guaranteed to fail.
diff --git a/test/test_http11.rb b/test/test_http11.rb
@@ -136,7 +136,10 @@ def rand_data(min, max, readable=true)
     if readable
       res << Digest(:SHA1).hexdigest(rand(count * 100).to_s) * (count / 40)
     else
-      res << Digest(:SHA1).digest(rand(count * 100).to_s) * (count / 20)
+      data = Digest(:SHA1).digest(rand(count * 100).to_s) * (count / 20)
+      # Guarantee there is an invalid byte at the end of the string
+      data.setbyte(data.bytesize - 1, 0x1)
+      res << data
     end
 
     res
@@ -208,8 +211,8 @@ def test_max_uri_path_length
     http = "GET #{path} HTTP/1.1\r\n\r\n"
     assert_raises Puma::HttpParserError do
       parser.execute(req, http, 0)
-      parser.reset
     end
+    parser.reset
   end
 
   def test_horrible_queries
@@ -220,34 +223,34 @@ def test_horrible_queries
       get = "GET /#{rand_data(10,120)} HTTP/1.1\r\nX-#{rand_data(1024, 1024+(c*1024))}: Test\r\n\r\n"
       assert_raises Puma::HttpParserError do
         parser.execute({}, get, 0)
-        parser.reset
       end
+      parser.reset
     end
 
     # then that large mangled field values are caught
     10.times do |c|
       get = "GET /#{rand_data(10,120)} HTTP/1.1\r\nX-Test: #{rand_data(1024, 1024+(c*1024), false)}\r\n\r\n"
       assert_raises Puma::HttpParserError do
         parser.execute({}, get, 0)
-        parser.reset
       end
+      parser.reset
     end
 
     # then large headers are rejected too
     get  = "GET /#{rand_data(10,120)} HTTP/1.1\r\n"
     get += "X-Test: test\r\n" * (80 * 1024)
     assert_raises Puma::HttpParserError do
       parser.execute({}, get, 0)
-      parser.reset
     end
+    parser.reset
 
     # finally just that random garbage gets blocked all the time
     10.times do |c|
       get = "GET #{rand_data(1024, 1024+(c*1024), false)} #{rand_data(1024, 1024+(c*1024), false)}\r\n\r\n"
       assert_raises Puma::HttpParserError do
         parser.execute({}, get, 0)
-        parser.reset
       end
+      parser.reset
     end
   end
 
