>
> > On 17 Jan 2018, at 11:19, Frediano Ziglio <fziglio@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx>
>
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.
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel