Skip to content

Conversation

aryan-25
Copy link
Contributor

Motivation:

  • The properties that store the request body length and the cumulative number of bytes sent as part of a request are of type Int.
  • On 32-bit devices, when sending requests larger than Int32.max, these properties overflow and cause a crash.
  • To solve this problem, the properties should use the explicit Int64 type.

Modifications:

  • Changed the type of the known field of the RequestBodyLength enum to Int64.
  • Changed the type of expectedBodyLength and sentBodyBytes in HTTPRequestStateMachine to Int64? and Int64 respectively.
  • Deprecated the public var length: Int? property of HTTPClient.Body and backed it with a new property: contentLength: Int64?
    • Added a new initializer and "overloaded" the stream function in HTTPClient.Body to work with the new contentLength property.
    • Note: The newly added stream function has different parameter names (length -> contentLength and stream -> bodyStream) to avoid ambiguity problems.
  • Added a test case that streams a 3GB request -- verified this fails with the types of the properties set explicitly to Int32.

Result:

  • 32-bit devices can send requests larger than 2GB without integer overflow issues.

@fabianfett
Copy link
Member

@swift-server-bot test this please

Copy link
Member

@fabianfett fabianfett left a comment

Choose a reason for hiding this comment

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

This already looks very god! Great patch! @Lukasa can you please check the API changes?

@fabianfett fabianfett requested a review from Lukasa June 24, 2024 14:06
@fabianfett fabianfett added the 🆕 semver/minor Adds new public API. label Jun 24, 2024
@Lukasa
Copy link
Collaborator

Lukasa commented Jun 27, 2024

@swift-server-bot add to allowlist

Copy link
Member

@fabianfett fabianfett left a comment

Choose a reason for hiding this comment

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

Great change!

@fabianfett fabianfett merged commit 4316eca into swift-server:main Jun 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🆕 semver/minor Adds new public API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants