Re: [PATCH v3 11/18] gweb: Close web request session finalization 'hole'.

On Mar 24, 2025, at 3:45 PM, Denis Kenzior <[email protected]> wrote:
> On 3/20/25 6:39 PM, Grant Erickson wrote:
>> There exist two "holes" in the finalization of web request sessions
>> that can, for example in a client such as WISPr, leave one or both of
>> IPv4 and IPv6 "online" HTTP-based Internet reachability checks
>> "dangling" and non-renewing such that an expected active, default
>> network service failover does not occur when it should.
>> The first of those two failure "holes" involves an end-of-file (EOF)
>> condition. In the normal case, there are a series of one or more data
>> receipts for the web request as headers and, if present, the body of
>> the request response are fulfilled. When there is no further data to
>> send, the final receive completes with 'g_io_channel_read_chars'
>> returning 'G_IO_STATUS_EOF' status and zero (0) bytes read. This should
>> and does lead to a successful EOF closure of the web request.
>> However, there is a second EOF case that appears to happen with a
>> random distribution in the nginx server backing the default "online"
>> HTTP-based Internet reachability check URLs,
>> 'ipv[46].connman.net/online/status.html'. In that case, the nginx
>> server appears to periodically do an unexpected and spontaneous remote
>> connection close after the initial connection but before any data has
>> been received. Presently, all such failures hit the same
>> error-handling block which effectively maps such a failure to the
>> effective "null" 'GWEB_HTTP_STATUS_CODE_UNKNOWN' status
>> value. Unfortunately, clients such as WISPr have no way to distinguish
>> this as an actual failure or success and so it lands on the case:
>> case GWEB_HTTP_STATUS_CODE_UNKNOWN:
>> wispr_portal_context_ref(wp_context);
>> __connman_agent_request_browser(wp_context->service,
>> wispr_portal_browser_reply_cb,
>> wp_context->status_url, wp_context);
>> which does not "bookend" the original, initiating WISPr web request
>> and leaves the original request "dangling" and non-renewed, eventually
>> leading to the aforementioned "hole".
>> To handle this failure EOF case, if GWeb asked for a non-zero amount
>> of data from 'g_io_channel_read_chars' but received none and has
>> accumulated no headers thus far upon receiving the status
>> 'G_IO_STATUS_EOF', then GWeb assumes that the remote peer server
>> unexpectedly closed the connection, and synthesizes a new GError with
>> the error 'ECONNRESET'. With the addition of the optional, immutable
>> GError parameter to the GWebResult callback function, clients such as
>> WISPr can now distinguish between a HTTP non-response in which no HTTP
>> data was received and one in which HTTP data was received and the
>> status can be disguished by the HTTP status code.
>> The second of the two failure "holes" involves the case where
>> 'g_io_channel_read_chars' returns 'G_IO_STATUS_ERROR' status. Prior to
>> this change, this funneled to the same "hole" as the 'G_IO_STATUS_ERROR'
>> failure with the same consequence to clients such as WISPr.
>> To handle this error case, GWeb now passes a glib GError pointer to
>> 'g_io_channel_read_chars'. If it is set, GWeb passes the GError
>> through to the GWebResult callback function. Otherwise, it synthesizes
>> a GError anew from 'EIO'. As with the failure EOF case, this allows
>> clients to distinguish and handle such failures and to successfully
>> "bookend" their initial web request.
>> ---
>>  gweb/gweb.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++-----
>>  1 file changed, 64 insertions(+), 6 deletions(-)
> 
> <snip>
> 
>> + if (status == G_IO_STATUS_ERROR) {
>> +     if (error != NULL)
>> +         passed_error = error;
>> +     else {
>> +         local_error = g_error_new_literal(G_IO_ERROR,
>> +                g_io_error_from_errno(EIO),
>> +                g_strerror(EIO));
>> +         passed_error = local_error;
>> +     }
> 
> Why is this needed?  Isn't g_io_read_chars always creating a GError object for you in case of G_IO_STATUS_ERROR return?

Without having looked at the actual implementation of ‘g_io_read_chars' in any particular version of glib, from the documentation alone, it is not clear that ‘error' is always set when the status is G_IO_STATUS_ERROR.

As implemented, this seems more defensive (which is what’s desired for this overall patch set).

Best,

Grant

-- 
Principal
Nuovations

[email protected]
https://www.nuovations.com/