Created attachment 366598 [details] [review]
gnutls: Fix use of undefined variables on early cancellation

If a claim_op() operation is cancelled before entering its blocking
while loop, the value of `result` would be undefined, which is bad.
Initialise `result` to a value which imitates what would happen had the
while loop been iterated and *then* cancelled.

Coverity CID: 249792

Signed-off-by: Philip Withnall <withnall@endlessm.com>

Created attachment 366599 [details] [review]
tests: Fix addressing of buffers in tests

They were being referenced once too much, which was causing the tests to
write into arbitrary memory.

Coverity CID: 249795

Signed-off-by: Philip Withnall <withnall@endlessm.com>

Review of attachment 366598 [details] [review]:

This one should go on the glib-2-54 branch as well.

::: tls/gnutls/gtlsconnection-gnutls.c
@@ +741,3 @@
       int nfds;
       gint64 start_time;
+      gint result = 1;  /* if the loop is never entered, it’s as if we cancelled early */

Why 1 and not 0? Wouldn't it be 0 if the poll call timed out?

Review of attachment 366599 [details] [review]:

OK

(In reply to Michael Catanzaro from comment #3)
> Review of attachment 366598 [details] [review] [review]:
> 
> This one should go on the glib-2-54 branch as well.
> 
> ::: tls/gnutls/gtlsconnection-gnutls.c
> @@ +741,3 @@
>        int nfds;
>        gint64 start_time;
> +      gint result = 1;  /* if the loop is never entered, it’s as if we
> cancelled early */
> 
> Why 1 and not 0? Wouldn't it be 0 if the poll call timed out?

If we are to hit the path where `result` is referenced by the code while being undefined, then the `while` loop has to not be iterated at all. That means that either of the cancellables are already cancelled by the time its condition is first checked. A cancellable being cancelled is *not* equivalent to timeout in this code: it’s equivalent to one of the FDs in the g_poll() call becoming readable (because that’s what the poll() call is for). That would imply (result == 1) or (result == 2). I chose (result == 1) out of those options, since (result == 2) is only possible if both cancellables are present (which is not always the case; see higher up in the function).

Does that make sense?

t log -2 --oneline# Bug 792402 - glib-networking Coverity fixes - NEW
Pushed to master. Will push both to glib-2-54 shortly.

Attachment 366598 [details] pushed as eb24905 - gnutls: Fix use of undefined variables on early cancellation
Attachment 366599 [details] pushed as 2812952 - tests: Fix addressing of buffers in tests

Actually, neither of these apply on glib-2-54, and neither of them are relevant (those tests don’t exist, and the g_poll() result is not handled in gtlsconnection-gnutls.c because none of the new custom GSource code is there).