[Spice-devel] [PATCH spice-server] red-stream: Handle reading of 0 bytes in red_stream_async_read

[Christophe de Dinechin]

> On 17 Jan 2018, at 11:19, Frediano Ziglio <fziglio at redhat.com> wrote:
> 
> Currently red_stream_async_read cannot handle read of 0 bytes.
> This would cause a wrong assert in async_read_handler.
> Fixing the assert would just make the code wrongly detect a
> disconnection (usually a return of 0 from read is handled that
> way but happens also if you try to read 0 bytes).
> Current callers of these function does not pass 0 as size however
> handling data protocols having data_length+data this can happen
> and is handled manually in red_sasl_handle_auth_steplen.
> Avoid needing manually to check for this condition.
> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
> server/red-stream.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/server/red-stream.c b/server/red-stream.c
> index 8f2c9d32..bdc8bc1f 100644
> --- a/server/red-stream.c
> +++ b/server/red-stream.c
> @@ -550,6 +550,10 @@ void red_stream_async_read(RedStream *stream,
>     AsyncRead *async = &stream->priv->async_read;
> 
>     g_return_if_fail(async->now == NULL && async->end == NULL);
> +    if (size == 0) {
> +        read_done_cb(opaque);
> +        return;
> +    }
>     async->now = data;
>     async->end = async->now + size;
>     async->done = read_done_cb;
> @@ -904,10 +908,6 @@ static void red_sasl_handle_auth_steplen(void *opaque)
>         return red_sasl_async_result(opaque, auth->mechname ? RED_SASL_ERROR_INVALID_DATA : RED_SASL_ERROR_GENERIC);
>     }
> 
> -    if (len == 0) {
> -        return red_sasl_handle_auth_step(auth);
> -    }
> -

I think from the commit message that you are saying that this condition
is handled in  red_sasl_handle_auth_steplen anyway, which is the reason
it’s OK to remove it here.

Assuming that’s what you mean, then

Acked-by: Christophe de Dinechin <dinechin at redhat.com>

(otherwise clarify the rationale in the commit message)

Also, did you have case where the assert actually triggers, or is that
just code inspection?

>     auth->data = g_realloc(auth->data, len);
>     red_stream_async_read(auth->stream, (uint8_t *)auth->data, len,
>                           red_sasl_handle_auth_step, auth);
> -- 
> 2.14.3
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel