From 5836a2b25ca6161eeb5cff0b1f145ab6bd862657 Mon Sep 17 00:00:00 2001 From: Adam Gundry Date: Fri, 2 Aug 2024 11:26:19 +0100 Subject: [PATCH] Builder: avoid unsound buffer reuse (#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 (5c4d23670e32967ad615699b5262e9cba4daccb7) and first present in release 0.11.5.0 (as 0c030bb63999117a6b1cb1275245a156313c0e49). 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. --- Data/ByteString/Builder/Internal.hs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Data/ByteString/Builder/Internal.hs b/Data/ByteString/Builder/Internal.hs index 69fb2ae0..b9053642 100644 --- a/Data/ByteString/Builder/Internal.hs +++ b/Data/ByteString/Builder/Internal.hs @@ -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' @@ -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