pipe
pipe copied to clipboard
cgaebel
Consumers segfaults if processing data too fast
It seems if producers/consumers are writing/reading (i.e., changing content of buffer) very quickly, we get a situation where the pipe (data) is "clobbered" somehow resulting in crash.
This doesn't seem to occur in the regular code because of #6, the pipe buffer continually grows. However, if you apply the patch from the issue, you may run into this problem.
Pipe maintains a circular buffer, where s.begin + elem_size points to the 1st elem to pop. There is a sentinel that is elem_size. s.bufend is the end of the buffer. When the pipe is "wrapping around", it looks like this. From the source code comments:
* buffer end begin bufend
* [============> >=====================]
In the pop function, there is a potential underflow if the number of bytes between begin and bufend < elem_size. In other words, there is no data between begin/bufend, only the sentinel.
// Copy either as many bytes as requested, or the available bytes in the RHS
// of a wrapped buffer - whichever is smaller.
{
size_t first_bytes_to_copy = min(bytes_to_copy, (size_t)(s.bufend - s.begin - elem_size));
target = offset_memcpy(target, s.begin + elem_size, first_bytes_to_copy);
When this happens, this leads to large number, so that min() always evaluate to bytes_to_copy. If this is elem_size or bigger (if we are fetching 1 or more elements), we are actually reading past the end of the buffer. This can lead to garbage data or a segmentation fault. Here is a stack trace of a segfault:
(gdb) p elem_size
$30 = 4096
(gdb) p s.bufend - s.begin
$31 = 2048
(gdb) p s.bufend - s.begin - elem_size
$32 = 18446744073709549568
(gdb) p bytes_to_copy
$33 = 4096
(gdb) p first_bytes_to_copy
$34 = 4096
The fix is simply making sure s.bufend - s.begin > elem_size before copying.
--- a/pipe.c
+++ b/pipe.c
@@ -960,11 +960,14 @@ static inline snapshot_t pop_without_locking(snapshot_t s,
assertume(s.begin != s.bufend);
size_t elem_size = s.elem_size;
+ // from begin to bufend could just be the sentinel (or part of one)
+ size_t remaining = s.bufend - s.begin;
// Copy either as many bytes as requested, or the available bytes in the RHS
// of a wrapped buffer - whichever is smaller.
+ if(likely(remaining > elem_size))
{
- size_t first_bytes_to_copy = min(bytes_to_copy, (size_t)(s.bufend - s.begin - elem_size));
+ size_t first_bytes_to_copy = min(bytes_to_copy, (size_t)(remaining - elem_size));
target = offset_memcpy(target, s.begin + elem_size, first_bytes_to_copy);
