Clear the right number of bytes in StreamBuffer

[sudastelaro]

The wrong number of bytes was being cleared in StreamBuffer::grow. That could lead to memory access out of bounds. We identified that after a number of Segmentation Faults in our project.

By the way, is that clear in the final bytes really necessary?

[henriquesimoes]

Hi! I was helping with the debugging of IOC that motivated this PR. We reached this function while debugging a segfault, which would occur sometimes during that memset. I now see I misinterpreted the logic then, influenced by the fault we were seeing. Sorry about that.

We identified the segfault when using the following combination for input control (using asyn 4.42 and StreamDevice 2.8.22:

MaxInput = 6;
Separator = "";
Terminator = "";
inTerminator= "";
outTerminator= "";

We also identified it occurring in other locations, such as asynInterposeEos::readIt called by AsynDriverInterface::readHandler. We noticed both cap and offs started to get increase significantly when this happened. Any chance cap got wrong with this setup with the reset computation in StreamBuffer::replace or somewhere else?

[dirk-zimoch]

Are you able to reproduce the fault with a simulated device which you can send me?

---

By the way, is that clear in the final bytes really necessary?

I do that to ensure I always have 0 terminated strings without having to add a 0 each time I add a char to the buffer.

[dirk-zimoch]

I think that the actual bug was in StreamBuffer::replace(), not in StreamBuffer::grow(). See #97.

[henriquesimoes]

Are you able to reproduce the fault with a simulated device which you can send me?

Sorry for the very long delay. It turns out I had to work on other demands and couldn't put effort on it yet.

I think that the actual bug was in StreamBuffer::replace(), not in StreamBuffer::grow().

I've taken a quick look at the bugs you've found, and it seems that corrupts the data while keeping the metadata (cap, len and offs) correct.

In the first bug, the buffer will be full of data and no \0 will be guaranteed to be following the last byte. However, if the metadata is correct, it should not lead to an invalid memset address range in grow nor a segfault upon data deference in asynInterposeEos::readIt, right?

In the second one, offs bytes after the end of the moved buffer will not be zeroed out. Still, offs will be correctly reset to 0, and len to newlen, and should not lead to a segfault in grow (even though eventually a read will see more data than it should).

I'd like we could test the patched code in the IOC we saw the faults to check if that's really the case. However, device firmware and IOC have changed significantly since then. We're no longer using StreamDevice, but rather asyn directly due to performance.

BTW, we may turn this into a GitHub issue if you will.

[dirk-zimoch]

You are right. Those should not corrupt the meta data. Your problem seems to be something else. The meta data is after the local buffer. Thus, a buffer overrun may corrupt them.

[henriquesimoes]

The meta data is after the local buffer.

Indeed. I hadn't noticed that. However, in our case, it probably switches to dynamically allocated buffer away before reaching the fault due to the large amount of data read.

Thus, a buffer overrun may corrupt them.

Would that happen with the bugs you've found? I'm not familiar with the asyn interface StreamDevice uses, but it seams to me it reads a limited amount of bytes, such as in AsynDriverInterface::readHandler. Having bytesToRead corrupted would lead to a invalid read, though. Is there anywhere read is done until \0 is found?

[dirk-zimoch]

No, none of those two bugs should lead to a buffer overflow when writing. At least none of the append() and replace() methods should. Because of the lost \0 in the other bugs, reading the buffer for example with strcpy() may have overflown other buffers if they had been allocated using the length reported by StreamBuffer. The asyn interface calls reserve() and then writes into the returned char*. I hope that does not overflow anything.

[dirk-zimoch]

Maybe in AsynDriverInterface.cc I need to set bytesToRead = inputBuffer.capacity() -1 instead of inputBuffer.capacity()?

[dirk-zimoch]

No. capacity() already returns cap-1. I need to look somewhere else.