Drop assumption that passed bytes are pinned #178
Conversation
Apply a fix that in case of unpinned bytes passed in hPutBuffer we first create a pinned ByteArray and copy bytes there
There was a problem hiding this comment.
Pull request overview
This PR fixes an assumption in hPutBuffer that byte arrays passed to it are always pinned. The fix adds runtime checking and copying when unpinned bytes are encountered.
Key changes:
- Added
Data.Primitive.ByteArrayimport for pinning operations - Introduced
pinIfNeededhelper function to check and copy unpinned byte arrays - Replaced
pinnedByteArrayToForeignPtrwithbyteArrayAsForeignPtrfor the conversion
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| mempack-scls/mempack-2.0/Data/MemPack/Extra.hs | Adds pinning check/copy logic for byte arrays with offset support in hPutBuffer |
| mempack-scls/mempack-1.0/Data/MemPack/Extra.hs | Adds pinning check/copy logic for byte arrays without offset in hPutBuffer |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| (pinnedBytes, offset) <- pinIfNeeded (ByteArray bytes) (I# off) | ||
| withForeignPtr (byteArrayAsForeignPtr pinnedBytes) $ \ptr -> do | ||
| hPutBuf handle (ptr `plusPtr` offset) (len - offset) | ||
| ) | ||
| (\addr -> hPutBuf handle (Ptr addr) len) -- Write Ptr# | ||
| (\addr -> hPutBuf handle (Ptr addr) len) | ||
| where | ||
| len = bufferByteCount u | ||
| pinIfNeeded bytes off | ||
| | isByteArrayPinned bytes = return (bytes, off) | ||
| | otherwise = do | ||
| let l = len - off | ||
| dest <- newPinnedByteArray l | ||
| copyByteArray dest 0 bytes off l | ||
| (,0) <$> unsafeFreezeByteArray dest |
There was a problem hiding this comment.
In hPutBuffer, the unpinned path in pinIfNeeded allocates a pinned buffer of length len - off but hPutBuf is still called with len - offset, which equals the original len when offset is 0. For unpinned ByteArrays where off > 0, this causes hPutBuf to read past the end of the newly allocated buffer and send extra bytes from adjacent memory to the Handle, potentially leaking sensitive process data or causing memory corruption. Adjust the logic so that the length passed to hPutBuf always matches the actual size of pinnedBytes (i.e., the copied slice), rather than the original buffer length.
Apply a fix that in case of unpinned bytes passed in hPutBuffer we first create a pinned ByteArray and copy bytes there