Critical Bug: net-http-persistent not resilient to thread interruptions, leading to potential (silent) response mismatches

[DDtKey]

Description

I've encountered a critical bug in the net-http-persistent - it is not resilient to thread interruptions. This issue can result in the response of one request being returned to another request, leading to potential data mismatches and unpredictable behavior - including leakage of sensitive data.

Details

When a thread performing a request is interrupted (e.g., using Thread.kill, Thread.terminate, Thread.raise, or common case is Timeout.timeout), the connection does not properly finish handling the sent request. The library currently relies only on rescue block for error handling, but this is insufficient for managing interruptions.

For example, this block won't be called:

net-http-persistent/lib/net/http/persistent.rb

Lines 916 to 919 in 12a9230

	rescue Exception # make sure to close the connection when it was interrupted
	finish connection
	raise

However, the connection will be returned to pool.

Root Cause

The core issue lies in the fact that net-http-persistent does not use Thread.handle_interrupt to manage thread interruptions safely. As a result, if a thread is interrupted, the response intended for one request may be silently delivered to another request, causing severe inconsistencies.

❗ This also means, net-http-persistent isn't thread-safe ❗

Minimal Reproducible Example

Below is a minimal reproducible example demonstrating the issue:

1. Simple Echo Server with 10s Delay

We need a simple echo server that introduces a 10-second delay to simulate a slow response, which makes it easier to interrupt the request in progress.

You can find simple node-js echo server in GitHub Gist: delayed-echo-server.js (it doesn't matter what to use, tested against different backends)

You can run it with Docker: docker run --rm -it -p 3000:3000 -v $(pwd):/usr/src/app -w /usr/src/app node:14-alpine node delayed-echo-server.js

2. Ruby Code to Reproduce the Issue

# webmock doesn't fit our needs, because mocking is happening in the same thread. But real applications are independent
# Probably there is better way to test it, but for MRE simple nodejs server is simple enough

def register_request(client, name)
  request_uri = "http://localhost:3000/register"
  post = Net::HTTP::Post.new(request_uri)
  post.body = name

  print "#{name} starts request, thread: #{Thread.current.object_id}\n"
  res = client.request request_uri, post
  print "#{name} receives: #{res.body} (thread: #{Thread.current.object_id})\n"
ensure
  print "#{name}'s request is done, thread: #{Thread.current.object_id}\n"
end

# can be any pool size, but using 1 to easily reproduce the issue with connection re-use
http_persistent = Net::HTTP::Persistent.new(name: "testing", pool_size: 1)

# start a request that will take some time
t1 = Thread.start do
  register_request(http_persistent, "Alice")
end

sleep 2 # wait for the request to start
# interrupt the thread, it can be anything, e.g. a timeout (the only difference is a signal)
t1.kill.join

register_request(http_persistent, "Bob")

The results of the execution:

Alice starts request, thread: 420640  
Alice's request is done, thread: 420640
Bob starts request, thread: 11380
Bob receives: Hello, Alice (thread: 11380)
Bob's request is done, thread: 11380

❗ Bob received a message intended for Alice: Bob receives: Hello, Alice (thread: 11380) ❗

Expected Behavior

Each request should receive its corresponding response, regardless of any thread interruptions.

Proposed Solution

To address this issue, net-http-persistent should incorporate Thread.handle_interrupt/Thread.pending_interrupt? to properly manage thread interruptions and ensure that connections are correctly handled and terminated when a thread is interrupted.