On Wed, 2018-06-27 at 14:57 +0100, Frediano Ziglio wrote:
> Although source of these data should be safe improve data checks
> to avoid some overflows.
>
> Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> ---
> vdagent/vdagent.cpp | 13 ++++++++++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/vdagent/vdagent.cpp b/vdagent/vdagent.cpp
> index e22687c..8294ea7 100644
> --- a/vdagent/vdagent.cpp
> +++ b/vdagent/vdagent.cpp
> @@ -1314,7 +1314,7 @@ VOID VDAgent::read_completion(DWORD err, DWORD
> bytes, LPOVERLAPPED overlapped)
> count = sizeof(VDIChunk) - a->_read_pos;
> } else if (a->_read_pos == sizeof(VDIChunk)) {
> count = chunk->hdr.size;
> - if (a->_read_pos + count > sizeof(a->_read_buf)) {
> + if (count > sizeof(a->_read_buf) - a->_read_pos) {
> vd_printf("chunk is too large, size %u port %u", chunk-
> >hdr.size, chunk->hdr.port);
> a->_running = false;
> return;
> @@ -1351,12 +1351,19 @@ void VDAgent::handle_chunk(VDIChunk* chunk)
> uint32_t msg_size = sizeof(VDAgentMessage) + msg->size;
> if (chunk->hdr.size == msg_size) {
> dispatch_message(msg, chunk->hdr.port);
> - } else {
> - ASSERT(chunk->hdr.size < msg_size);
> + } else if (chunk->hdr.size < msg_size) {
> _in_msg = (VDAgentMessage*)new uint8_t[msg_size];
> memcpy(_in_msg, chunk->data, chunk->hdr.size);
> _in_msg_pos = chunk->hdr.size;
> + } else {
> + vd_printf("Invalid VDAgentMessage message");
> + _running = false;
> + return;
> }
This chunk of the patch isn't really about preventing an overflow. It's
changing an assert to a warning message. So I kind of want to say that
it should be a separate commit. But it is pretty closely related to the
below change, which *does* avoid an overflow...
> + } else if (chunk->hdr.size > sizeof(VDAgentMessage) + _in_msg-
> >size - _in_msg_pos) {
> + vd_printf("Invalid VDAgentMessage message");
> + _running = false;
> + return;
> } else {
> memcpy((uint8_t*)_in_msg + _in_msg_pos, chunk->data, chunk-
> >hdr.size);
> _in_msg_pos += chunk->hdr.size;
It's a little bit hard to keep track of what is potentially
uninitialized among these member variables. I thought that _in_msg.size
could potentially be uninitialized here, but it looks like we're
protected by the following line above:
if (chunk->hdr.size < sizeof(VDAgentMessage)) {
return;
}
So the code looks correct, albeit a bit hard to read.
Acked-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel