This commit fixes a bug that causes incorrect results when pasting more
than 32 bytes, the size of the receive buffer b->buffer, into the virtio
console.
Example (note that the last 32 bytes are always correct, but something
goes wrong just before the last 32 bytes):
Pasting abcdefghijklmnopqrstuvwxyz1234567890ABCDEFGHIJKLMNOPQRSTUVWXYZ!@#$%^&*()
Received abcdefg)EFGHIJKLMNOPQRSTUVWXYZ!@#$%^&*()EFGHIJKLMNOPQRSTUVWXYZ!@#$%^&*()
The root cause of this bug is as follows:
The mux_chr_read function passes the data to the backend via
be->chr_read one byte at a time, either directly or via another
mux_chr_accept_input method. However, if the receive buffer is full,
there is a chance that the mux_chr_can_read method will return more than
one byte, because in this case the method directly returns whatever
be->chr_can_read returns.
This is problematic because if mux_chr_read passes a byte to the backend
by calling be->chr_read, it will consume the entire backend buffer, at
least in the case of virtio. Once all backend buffers are used,
mux_chr_read writes all remaining bytes to the receive buffer d->buffer,
My understanding of the code execution is:
- mux_chr_can_read() returns be->chr_can_read(), say N, because d->buffer is already MUX_BUFFER_SIZE.
- mux_chr_read() is called with N bytes
- mux_chr_accept_input() flushes d->buffer, writing MUX_BUFFER_SIZE
- be should still accept N-MUX_BUFFER_SIZE
- mux_proc_byte() loops for N bytes
- chr_read() should accept the N-MUX_BUFFER_SIZE
- d->buffer is then filled with the remaining MUX_BUFFER_SIZE
but the number of remaining bytes can be larger than the buffer size.
By the above description, I don't see how it happens.
This does not lead to security problems since it is a ring buffer, but
it does mess up the receive data.
This can be fixed by having mux_chr_can_read return either zero or one.
This fix is not very efficient, but it is quite reasonable since
mux_chr_read also passes the data to the backend one byte at a time.
Could you share your testing setup? Even better if you could write a test!
thanks
Signed-off-by: Ruihan Li <lrh2000@pku.edu.cn>
---
chardev/char-mux.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/chardev/char-mux.c b/chardev/char-mux.c
index ee2d47b..5c6eea2 100644
--- a/chardev/char-mux.c
+++ b/chardev/char-mux.c
@@ -210,8 +210,8 @@ static int mux_chr_can_read(void *opaque)
return 1;
}
- if (be && be->chr_can_read) {
- return be->chr_can_read(be->opaque);
+ if (be && be->chr_can_read && be->chr_can_read(be->opaque)) {
+ return 1;
}
return 0;
--
2.45.2