Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

NIOFileSytem: crash when passing maximumSizeAllowed: .gibibytes(.max) to disable limits #2877

Open
weissi opened this issue Sep 10, 2024 · 4 comments
Open

NIOFileSytem: crash when passing maximumSizeAllowed: .gibibytes(.max) to disable limits #2877

weissi opened this issue Sep 10, 2024 · 4 comments

Comments

@weissi
Copy link
Member

weissi commented Sep 10, 2024 •
edited
Loading 

request.body = try await Data(buffer: ByteBuffer(
    contentsOf: filePath,
    maximumSizeAllowed: .gibibytes(.max)
))

crashes with

Thread 5 Crashed:: NIO-SGLTN-0-#4
0   0x104f0e510 Swift runtime failure: arithmetic overflow + 0 [inlined]
1   0x104f0e510 static ByteCount.gibibytes(_:) + 76 (ByteCount.swift:79)

but it shouldn't.
@glbrntt
Copy link
Contributor

glbrntt commented Sep 10, 2024

Agree. This intersects a bit weirdly with #2878 – if .gigabytes(.max) is okay, is .megabytes(.max) okay? Maybe we should reshape the API and allow maximumSizeAllowed to be nil or add a sentinel value to the ByteCount which makes the intent clearer (.max, maybe?).

@weissi
Copy link
Member Author

weissi commented Sep 10, 2024

Agree. This intersects a bit weirdly with #2878 – if .gigabytes(.max) is okay, is .megabytes(.max) okay? Maybe we should reshape the API and allow maximumSizeAllowed to be nil or add a sentinel value to the ByteCount which makes the intent clearer (.max, maybe?).

Yes, I'd suggest just .unlimitedDangerousForSecurityReasons or something straight on ByteCount.

@glbrntt
Copy link
Contributor

glbrntt commented Sep 10, 2024

Feels weird having it on ByteCount but it might just be the pragmatic choice.

@weissi
Copy link
Member Author

weissi commented Sep 12, 2024

Feels weird having it on ByteCount but it might just be the pragmatic choice.

Why? ByteCount.unlimited sounds alright, no?

clintonpi added a commit to clintonpi/swift-nio that referenced this issue Oct 10, 2024
@clintonpi
[NIOFileSystem] Provide an API to specify allowing unlimited sized reads
bdaecb6 
Motivation:

As described in issue [apple#2877](apple#2877), there is no API available to specify allowing a read of unlimited size.

Modifications:

- Add a new `public static` property, `unlimited`, to `ByteCount` representing an unlimited amount of bytes.
- Adapt `ReadableFileHandleProtocol.readToEnd(fromAbsoluteOffset:maximumSizeAllowed:)` to work with `maximumSizeAllowed` being `ByteCount.unlimited`.

Result:

An API to specify allowing a read of an unlimited size will be available.
@clintonpi clintonpi mentioned this issue Oct 10, 2024
Merged
glbrntt pushed a commit that referenced this issue Oct 11, 2024
@clintonpi
[NIOFileSystem] Provide an API to specify allowing unlimited sized re…
17ebfcd 
…ads (#2914)

Motivation:

As described in issue
[#2877](#2877), there is no API
available to specify allowing a read of unlimited size.

Modifications:

- Add a new `public static` property, `unlimited`, to `ByteCount`
representing an unlimited amount of bytes.
- Adapt
`ReadableFileHandleProtocol.readToEnd(fromAbsoluteOffset:maximumSizeAllowed:)`
to work with `maximumSizeAllowed` being `ByteCount.unlimited`.

Result:

An API to specify allowing a read of an unlimited size will be
available.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@weissi @glbrntt