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

[Christophe de Dinechin]

> On 30 Jan 2018, at 12:22, Frediano Ziglio <fziglio at redhat.com> wrote:
> 
>>> 
>>> 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>
>> 
> 
> I think so.
> Checking all the paths (callers) is always used with a value >0.
> In red_sasl_handle_auth_steplen the condition ==0 is handled with an
> explicit test to avoid it.
> However if you consider the "normal" read(2) function reading 0 bytes
> is allowed, in my man page:
> 
> "If count is zero, read() may detect the errors described below. 
> In the absence of any errors, or if read() does not check for errors,
> a read() with a count of 0 returns zero  and has no other effects."
> 
> this is actually a tricky case that confuse some code where 0 bytes
> returned is detected as a socket closed while you asked just 0 bytes.
> 
> I think from the API prospective that reading 0 bytes should be
> allowed instead of having to manually check it.
> 
>> (otherwise clarify the rationale in the commit message)
>> 
>> Also, did you have case where the assert actually triggers, or is that
>> just code inspection?
>> 
> 
> Code inspection and the removal of the code in red_sasl_handle_auth_steplen.
> So we can say both.
> 
>>>    auth->data = g_realloc(auth->data, len);
>>>    red_stream_async_read(auth->stream, (uint8_t *)auth->data, len,
>>>                          red_sasl_handle_auth_step, auth);
> 
> I read again the commit message and make sense to me but maybe just
> because I wrote it. Suggestion for better phrasing are welcome.

No. That’s how I had read it too. So again,

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


> 
> Frediano
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel