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

Builder: avoid unsound buffer reuse #691

Merged
merged 1 commit into from
Sep 18, 2024

Conversation

adamgundry
Copy link
Member

@adamgundry adamgundry commented Aug 2, 2024

Fixes #690.

toLazyByteString :: Builder -> LazyByteString had a race condition that could generate wrong results if two threads concurrently evaluated the result. This bug was introduced in #581 (5c4d236) and first present in release 0.11.5.0 (as 0c030bb).

Due to the use of unsafeDupablePerformIO for performance, it is critical that any IO actions executed when running a Builder can be interrupted or executed multiple times. In principle, filling a buffer is safe provided the buffer is used only once and the same bytes are written each time. However, wrapChunk in buildStepToCIOS would re-use a buffer in the trimming case after copying its contents to produce a new trimmed chunk. This is safe when run in a single thread, but if two threads simultaneously execute the code, one of them may still be copying the contents while the other starts overwriting the buffer.

This patch fixes wrapChunk to unconditionally allocate a new buffer after trimming, rather than re-using the old buffer. This will presumably come at a slight performance cost for builders inserting many trimmed chunks.

`toLazyByteString :: Builder -> LazyByteString` had a race condition that could
generate wrong results if two threads concurrently evaluated the result.  This
bug was introduced in haskell#581 (5c4d236) and first
present in release 0.11.5.0 (as 0c030bb).

Due to the use of `unsafeDupablePerformIO` for performance, it is critical that
any IO actions executed when running a `Builder` can be interrupted or executed
multiple times.  In principle, filling a buffer is safe provided the buffer is
used only once and the same bytes are written each time. However, `wrapChunk` in
`buildStepToCIOS` would re-use a buffer in the trimming case after copying its
contents to produce a new trimmed chunk. This is safe when run in a single
thread, but if two threads simultaneously execute the code, one of them may
still be copying the contents while the other starts overwriting the buffer.

This patch fixes `wrapChunk` to unconditionally allocate a new buffer after
trimming, rather than re-using the old buffer. This will presumably come at a
slight performance cost for builders inserting many trimmed chunks.
Copy link
Member

@clyring clyring left a comment

Choose a reason for hiding this comment

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

This looks good, of course. Did you want to try adjusting the customStrategy documentation as well?

@clyring clyring merged commit 378d4c3 into haskell:master Sep 18, 2024
25 of 26 checks passed
@clyring clyring added this to the 0.11.5.4 milestone Sep 18, 2024
Bodigrim pushed a commit that referenced this pull request Oct 9, 2024
`toLazyByteString :: Builder -> LazyByteString` had a race condition that could
generate wrong results if two threads concurrently evaluated the result.  This
bug was introduced in #581 (5c4d236) and first
present in release 0.11.5.0 (as 0c030bb).

Due to the use of `unsafeDupablePerformIO` for performance, it is critical that
any IO actions executed when running a `Builder` can be interrupted or executed
multiple times.  In principle, filling a buffer is safe provided the buffer is
used only once and the same bytes are written each time. However, `wrapChunk` in
`buildStepToCIOS` would re-use a buffer in the trimming case after copying its
contents to produce a new trimmed chunk. This is safe when run in a single
thread, but if two threads simultaneously execute the code, one of them may
still be copying the contents while the other starts overwriting the buffer.

This patch fixes `wrapChunk` to unconditionally allocate a new buffer after
trimming, rather than re-using the old buffer. This will presumably come at a
slight performance cost for builders inserting many trimmed chunks.
Bodigrim pushed a commit that referenced this pull request Oct 15, 2024
`toLazyByteString :: Builder -> LazyByteString` had a race condition that could
generate wrong results if two threads concurrently evaluated the result.  This
bug was introduced in #581 (5c4d236) and first
present in release 0.11.5.0 (as 0c030bb).

Due to the use of `unsafeDupablePerformIO` for performance, it is critical that
any IO actions executed when running a `Builder` can be interrupted or executed
multiple times.  In principle, filling a buffer is safe provided the buffer is
used only once and the same bytes are written each time. However, `wrapChunk` in
`buildStepToCIOS` would re-use a buffer in the trimming case after copying its
contents to produce a new trimmed chunk. This is safe when run in a single
thread, but if two threads simultaneously execute the code, one of them may
still be copying the contents while the other starts overwriting the buffer.

This patch fixes `wrapChunk` to unconditionally allocate a new buffer after
trimming, rather than re-using the old buffer. This will presumably come at a
slight performance cost for builders inserting many trimmed chunks.
@Bodigrim
Copy link
Contributor

This is meant to be both 0.11.5.4 and 0.12.2.0, but GitHub allows only one milestone.

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.

Builder: unsound reuse of buffers
3 participants