apple / swift-nio

Event-driven network application framework for high performance protocol servers & clients, non-blocking.
https://swiftpackageindex.com/apple/swift-nio/documentation
Apache License 2.0
7.98k stars 652 forks source link

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

Open weissi opened 1 month ago

weissi commented 1 month ago
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 commented 1 month ago

Agree. This intersects a bit weirdly with https://github.com/apple/swift-nio/issues/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 commented 1 month ago

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 commented 1 month ago

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

weissi commented 1 month ago

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

Why? ByteCount.unlimited sounds alright, no?