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.

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

Fixed off by one for writeBlocksRaw #590

Merged
merged 7 commits into from
May 29, 2024

Conversation

BebeSparkelSparkel
Copy link
Contributor

closes #587

@Bodigrim
Copy link
Contributor

Is it possible to add a regression test?

@BebeSparkelSparkel
Copy link
Contributor Author

It may be possible, but I am unsure how to. This is a case of the last character in the buffer not being used because the bounds check was copy pasted from the above write* functions that have the possibility of writing two characters CRLF.

@Bodigrim
Copy link
Contributor

Would anything in the current test suite break if we put n - 10 instead of n / n + 1? Do we have any tests in this area at all?

@BebeSparkelSparkel
Copy link
Contributor Author

All tests pass with n - 10

@BebeSparkelSparkel
Copy link
Contributor Author

I don't know how to test for buffer overflow unless we can add a test buffer type that checks for that.

@Lysxia
Copy link
Contributor

Lysxia commented Apr 30, 2024

We might not catch this in regular builds but we can test this by inserting a debug assertion (enabled by -DASSERTS under the -fdeveloper cabal flag) to ensure that writeCharBuf is within bounds.

@BebeSparkelSparkel
Copy link
Contributor Author

@Lysxia Good idea. I have added the bounds assertion to a wrapped writeCharBuf

@BebeSparkelSparkel BebeSparkelSparkel marked this pull request as ready for review April 30, 2024 17:23
@BebeSparkelSparkel BebeSparkelSparkel force-pushed the writeBlocksRaw-off-one branch 2 times, most recently from f0337f2 to cd295ec Compare May 5, 2024 16:42
@BebeSparkelSparkel
Copy link
Contributor Author

@Bodigrim Anything else that needs to be done?

src/Data/Text/IO.hs Outdated Show resolved Hide resolved
@Bodigrim Bodigrim requested a review from Lysxia May 23, 2024 19:42
src/Data/Text/IO.hs Outdated Show resolved Hide resolved
src/Data/Text/IO.hs Outdated Show resolved Hide resolved
src/Data/Text/IO.hs Outdated Show resolved Hide resolved
Copy link
Contributor

@Lysxia Lysxia left a comment

Choose a reason for hiding this comment

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

LGTM!

@Bodigrim Bodigrim merged commit 4fba353 into haskell:master May 29, 2024
28 checks passed
@Bodigrim
Copy link
Contributor

Thanks!

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.

Off by one in writeBlocksRaw
3 participants