Skip to content

Conversation

rpeng
Copy link
Contributor

@rpeng rpeng commented Sep 22, 2023

When timeouts are set on the client, there is potential for a resource leak when timeouts occur during connection initialization.

When timeout errors are thrown during connection initialization, the connection object itself is not created (nil) and the client won't be able to properly close the connection.

This causes the created socket to stay in either an ESTABLISHED or CLOSE_WAIT state, while the client no longer has a reference to it, effectively causing a leak.

Steps to repro:

In the client:

# irb or rails c
client = HTTP.timeout(5)

client.get("https://www.example.org").body.to_s # or use 240.0.0.0
client.get("https://www.example.org").body.to_s
client.get("https://www.example.org").body.to_s

Simultaneously:

  1. Set up a mitm to intercept example.org, and add network conditioning to make the request very slow. Or you can use a black hole IP such as 240.0.0.0
  2. Check the sockets opened by the ruby process:
watch -n 1 "lsof -i -P | grep PROCESS_PID" # replace PROCESS_PID with the rails process pid

You'll notice that sockets will stay in ESTABLISHED or CLOSE_WAIT as you make more and more requests that time out.

ruby    28497 rpeng   21u  IPv4 43444771      0t0  TCP f6eb5410737c:57558->93.184.216.34:443 (CLOSE_WAIT)
ruby    28497 rpeng   23u  IPv4 43449275      0t0  TCP f6eb5410737c:56876->93.184.216.34:443 (CLOSE_WAIT)
ruby    28497 rpeng   24u  IPv4 43452649      0t0  TCP f6eb5410737c:34242->93.184.216.34:443 (ESTABLISHED)
ruby    28497 rpeng   25u  IPv4 43458906      0t0  TCP f6eb5410737c:55366->93.184.216.34:443 (ESTABLISHED)
ruby    28497 rpeng   26u  IPv4 43461133      0t0  TCP f6eb5410737c:37510->93.184.216.34:443 (ESTABLISHED)

The fix ensures that a socket is closed if any timeouts happen during initialization, though I suspect we could make this even more stringent (e.g. on any StandardError, we ensure that we send @socket.close if a socket exists and is open)

@rpeng rpeng force-pushed the rpeng/close-socket-on-timeout-in-initialize branch 2 times, most recently from 6272bdb to c1ddb6f Compare September 22, 2023 05:07
require "io/wait"

module HTTP
module Timeout
class Null
extend Forwardable

def_delegators :@socket, :close, :closed?
Copy link
Contributor Author

@rpeng rpeng Sep 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forwardable and def_delegators don't really work with safe nav operators

@tarcieri tarcieri requested a review from ixti September 22, 2023 12:56
@tarcieri tarcieri merged commit 3b7133c into httprb:main Sep 26, 2023
@rpeng rpeng deleted the rpeng/close-socket-on-timeout-in-initialize branch September 26, 2023 17:13
Copy link
Member

@ixti ixti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Aug 11, 2025
Changelog:
## [5.3.1] - 2025-06-09

### Changed

- Revert switch to the native llhttp on MRI, as it's not compatible with
  standalone bundles
  ([#802](httprb/http#802))


## [5.3.0] - 2025-06-09

### Added

- (backported) Add .retriable feature to Http
- (backported) Add more specific ConnectionError classes
- (backported) New feature: RaiseError

### Changed

- (backported) Drop depenency on base64
- (backported) Cache header normalization to reduce object allocation
- (backported) Use native llhttp on MRI


## [5.2.0] - 2024-02-05

### Added

- Add `Connection#finished_request?`
  ([#743](httprb/http#743))
- Add `Instrumentation#on_error`
  ([#746](httprb/http#746))
- Add `base64` dependency (suppresses warnings on Ruby 3.0)
  ([#759](httprb/http#759))
- Add `PURGE` HTTP verb
  ([#757](httprb/http#757))
- Add Ruby-3.3 support

### Changed

- **BREAKING** Process features in reverse order
  ([#766](httprb/http#766))
- **BREAKING** Downcase Content-Type charset name
  ([#753](httprb/http#753))
- **BREAKING** Make URI normalization more conservative
  ([#758](httprb/http#758))

### Fixed

- Close sockets on initialize failure
  ([#762](httprb/http#762))
- Prevent CRLF injection due to broken URL normalizer
  ([#765](httprb/http#765))

[unreleased]: httprb/http@v5.3.0...5-x-stable
[5.3.0]: httprb/http@v5.2.0...v5.3.0
[5.2.0]: httprb/http@v5.1.1...v5.2.0

## 5.1.1 (2022-12-17)

* [#731](httprb/http#731)
  Strip brackets from IPv6 addresses in `HTTP::URI`.
  ([@jeraki])

* [#722](httprb/http#722)
  Add `on_redirect` callback.
  ([@benubois])

## 5.1.0 (2022-06-17)

* Drop ruby-2.5 support.

* [#715](httprb/http#715)
  Set default encoding to UTF-8 for `application/json`.
  ([@drwl])

* [#712](httprb/http#712)
  Recognize cookies set by redirect.
  ([@tkellogg])

* [#707](httprb/http#707)
  Distinguish connection timeouts.
  ([@YuLeven])

## 5.0.4 (2021-10-07)

* [#698](httprb/http#698)
  Fix `HTTP::Timeout::Global#connect_ssl`.
  ([@tarcieri])

## 5.0.3 (2021-10-06)

* [#695](httprb/http#695)
  Revert DNS resolving feature.
  ([@PhilCoggins])

* [#694](httprb/http#694)
  Fix cookies extraction.
  ([@flosacca])

## 5.0.2 (2021-09-10)

* [#686](httprb/http#686)
  Correctly reset the parser.
  ([@bryanp])

* [#684](httprb/http#684)
  Don't set Content-Length for GET, HEAD, DELETE, or CONNECT requests without a BODY.
  ([@jyn514])

* [#679](httprb/http#679)
  Use features on redirected requests.
  ([@nomis])

* [#678](httprb/http#678)
  Restore `HTTP::Response` `:uri` option for backwards compatibility.
  ([@schwern])

* [#676](httprb/http#676)
  Update addressable because of CVE-2021-32740.
  ([@matheussilvasantos])

* [#653](httprb/http#653)
  Avoid force encodings on frozen strings.
  ([@bvicenzo])

* [#638](httprb/http#638)
  DNS failover handling.
  ([@midnight-wonderer])


## 5.0.1 (2021-06-26)

* [#670](httprb/http#670)
  Revert `Response#parse` behavior introduced in [#540].
  ([@DannyBen])

* [#669](httprb/http#669)
  Prevent bodies from being resubmitted when following unsafe redirects.
  ([@odinhb])

* [#664](httprb/http#664)
  Bump llhttp-ffi to 0.3.0.
  ([@bryanp])


## 5.0.0 (2021-05-12)

* [#656](httprb/http#656)
  Handle connection timeouts in `Features`
  ([@semenyukdmitry])

* [#651](httprb/http#651)
  Replace `http-parser` with `llhttp`
  ([@bryanp])

* [#647](httprb/http#647)
  Add support for `MKCALENDAR` HTTP verb
  ([@meanphil])

* [#632](httprb/http#632)
  Respect the SSL context's `verify_hostname` value
  ([@colemannugent])

* [#625](httprb/http#625)
  Fix inflator with empty responses
  ([@LukaszMaslej])

* [#599](httprb/http#599)
  Allow passing `HTTP::FormData::{Multipart,UrlEncoded}` object directly.
  ([@ixti])

* [#593](httprb/http#593)
  [#592](httprb/http#592)
  Support informational (1XX) responses.
  ([@ixti])

* [#590](httprb/http#590)
  [#589](httprb/http#589)
  Fix response headers paring.
  ([@Bonias])

* [#587](httprb/http#587)
  [#585](httprb/http#585)
  Fix redirections when server responds with multiple Location headers.
  ([@ixti])

* [#581](httprb/http#581)
  [#582](httprb/http#582)
  Add Ruby 2.7.x support.
  ([@janko])

* [#577](httprb/http#577)
  Fix `Chainable#timeout` with frozen Hash.
  ([@antonvolkoff])

* [#576](httprb/http#576)
  [#524](httprb/http#524)
  **BREAKING CHANGE**
  Preserve header names casing.
  ([@joshuaflanagan])

* [#540](httprb/http#540)
  [#538](httprb/http#538)
  **BREAKING CHANGE**
  Require explicit MIME type for Response#parse
  ([@ixti])

* [#532](httprb/http#532)
  Fix pipes support in request bodies.
  ([@ixti])

* [#530](httprb/http#530)
  Improve header fields name/value validation.
  ([@Bonias])

* [#506](httprb/http#506)
  [#521](httprb/http#521)
  Skip auto-deflate when there is no body.
  ([@Bonias])

* [#489](httprb/http#489)
  Fix HTTP parser.
  ([@ixti], [@fxposter])

* [#546](httprb/http#546)
  **BREAKING CHANGE**
  Provide initiating `HTTP::Request` object on `HTTP::Response`.
  ([@joshuaflanagan])

* [#571](httprb/http#571)
  Drop Ruby 2.3.x support.
  ([@ixti])

* [3ed0c31](httprb/http@3ed0c31)
  Drop Ruby 2.4.x support.
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 this pull request may close these issues.

3 participants