Skip to content

Commit

Permalink
Builder: avoid unsound buffer reuse (#690) (#691)
Browse files Browse the repository at this point in the history
`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.
  • Loading branch information
adamgundry authored Sep 18, 2024
1 parent 594966b commit 378d4c3
Showing 1 changed file with 4 additions and 4 deletions.
8 changes: 4 additions & 4 deletions Data/ByteString/Builder/Internal.hs
Original file line number Diff line number Diff line change
Expand Up @@ -1196,7 +1196,7 @@ buildStepToCIOS (AllocationStrategy nextBuffer bufSize trim) =
-- Checking for empty case avoids allocating 'n-1' empty
-- buffers for 'n' insertChunkH right after each other.
if isEmpty
then fill nextStep (Buffer fpbuf (BufferRange pbuf pe))
then fill nextStep buf
else do buf' <- nextBuffer (Just (buf, bufSize))
fill nextStep buf'

Expand All @@ -1208,9 +1208,9 @@ buildStepToCIOS (AllocationStrategy nextBuffer bufSize trim) =
| trim chunkSize size = do
bs <- S.createFp chunkSize $ \fpbuf' ->
S.memcpyFp fpbuf' fpbuf chunkSize
-- Instead of allocating a new buffer after trimming,
-- we re-use the old buffer and consider it empty.
return $ Yield1 bs (mkCIOS True)
-- It is not safe to re-use the old buffer (see #690),
-- so we allocate a new buffer after trimming.
return $ Yield1 bs (mkCIOS False)
| otherwise =
return $ Yield1 (S.BS fpbuf chunkSize) (mkCIOS False)
where
Expand Down

0 comments on commit 378d4c3

Please sign in to comment.