Skip to content

fix: Prevent arithmetic overflow in ByteCount factory methods - #3677

Open
wasim-builds wants to merge 4 commits into
apple:mainfrom
wasim-builds:fix-bytecount-overflow
Open

fix: Prevent arithmetic overflow in ByteCount factory methods#3677
wasim-builds wants to merge 4 commits into
apple:mainfrom
wasim-builds:fix-bytecount-overflow

Conversation

@wasim-builds

Copy link
Copy Markdown
Contributor

Fixes #2877

@Lukasa Lukasa left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for this patch: do you mind adding some tests for these to prove that these cases now behave sensibly?

Relatedly, @glbrntt: can you confirm that negative values should work here?

@wasim-builds

Copy link
Copy Markdown
Contributor Author

Thanks @Lukasa! I've just added the tests covering both negative values and overflow conditions to prove they behave sensibly.

@wasim-builds
wasim-builds force-pushed the fix-bytecount-overflow branch from 99fceda to 1a92618 Compare July 23, 2026 14:57
@wasim-builds

Copy link
Copy Markdown
Contributor Author

Rebased onto latest upstream/main. Only the overflow fix and its test remain.

@glbrntt

glbrntt commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Relatedly, @glbrntt: can you confirm that negative values should work here?

Yes, they should. ByteCount implements AdditiveArithmetic so not supporting negative values could lead to some surprising behavior...

Comment thread Tests/NIOFSTests/ByteCountTests.swift
@glbrntt glbrntt added the 🔨 semver/patch No public API change. label Jul 24, 2026
Comment thread Tests/NIOFSTests/ByteCountTests.swift Outdated
@wasim-builds

Copy link
Copy Markdown
Contributor Author

Yes, I can confirm that negative values work correctly! The implementation supports negative numbers, properly handles underflows by clamping to .min, and the testByteCountNegative and testByteCountOverflow test cases explicitly verify this behavior. Thanks for making the formatting tweak! 🙌

@glbrntt

glbrntt commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

There are still formatting issues it seems, can you run the formatter?

@wasim-builds

Copy link
Copy Markdown
Contributor Author

I've gone ahead and fixed the remaining formatting issues (there was some trailing whitespace left over in the test file) and pushed the update. Let me know if everything looks good to go now!

@glbrntt
glbrntt enabled auto-merge (squash) July 24, 2026 16:47
auto-merge was automatically disabled July 27, 2026 06:50

Head branch was pushed to by a user without write access

@wasim-builds
wasim-builds force-pushed the fix-bytecount-overflow branch from 9444841 to e03ab8e Compare July 27, 2026 06:50
@wasim-builds

Copy link
Copy Markdown
Contributor Author

Thanks @glbrntt! I've addressed the formatting suggestions — fixed the missing blank line before testByteCountNegative() and removed trailing whitespace. Branch has been rebased on upstream/main and force-pushed. Ready for another look!

@wasim-builds

Copy link
Copy Markdown
Contributor Author

@glbrntt Could you let me know which specific formatting issues remain? I fixed the blank lines before the test functions. Unfortunately I don't have Swift available locally to run the formatter — is there a specific check I'm missing in the diff? Happy to fix right away!

@wasim-builds

Copy link
Copy Markdown
Contributor Author

@Lukasa Friendly ping - I have added tests covering negative values and overflow conditions for all ByteCount factory methods (in the latest commit). Could you take another look when you get a chance?

@wasim-builds
wasim-builds force-pushed the fix-bytecount-overflow branch from 18c7efe to 107c060 Compare July 28, 2026 03:14
@wasim-builds
wasim-builds force-pushed the fix-bytecount-overflow branch 2 times, most recently from 26e5b2f to 85c5d28 Compare July 28, 2026 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🔨 semver/patch No public API change.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

3 participants