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

Hi Grant,

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>

> diff --git a/gweb/gweb.c b/gweb/gweb.c
> index 7ad12c977ff8..6d517f9467bb 100644
> --- a/gweb/gweb.c
> +++ b/gweb/gweb.c
> @@ -1202,14 +1202,65 @@ static void add_header_field(struct web_session *session)
>   	}
>   }
>   
> -static void received_data_finalize(struct web_session *session)
> +static void received_data_finalize(struct web_session *session,
> +				GIOStatus status, gsize bytes_available,
> +				gsize bytes_read, const GError *error)
>   {
> +	GError *local_error = NULL;

g_autoptr(GError)?

> +	const GError *passed_error = NULL;
> +
>   	session->transport_watch = 0;
>   
>   	session->result.buffer = NULL;
>   	session->result.length = 0;
>   
> -	call_result_func(session, NULL);
> +	/* Handle post-channel read errors, which could be either
> +	 * G_IO_STATUS_ERROR or G_IO_STATUS_EOF.
> +	 *
> +	 * For G_IO_STATUS_ERROR, simply pass through the GError, if
> +	 * non-null. If there is no GError, create a GError anew based on
> +	 * EIO.
> +	 *
> +	 * For G_IO_STATUS_EOF, this could occur at the end of a nominal,
> +	 * successful get. That is, some number of headers, with or
> +	 * without a body, termiated by an expected end-of-file (EOF)
> +	 * condition. However, G_IO_STATUS_EOF can also happen as a result
> +	 * of the remote peer server unexpectedly terminating the
> +	 * connection without transferring any data at all. The only
> +	 * reasonable recovery for this case it to fail the request,
> +	 * synthesizing ECONNRESET as the error, and to let the client
> +	 * request again.
> +	 *
> +	 * If we asked for a non-zero amount of data but received none and
> +	 * have accumulated no headers thus far, then we assume that the
> +	 * remote peer server unexpectedly closed the connection;
> +	 * otherwise, we assume it is a normal EOF closure.
> +	 */
> +
> +	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?

> +	} else if (status == G_IO_STATUS_EOF) {
> +		if (bytes_available > 0 &&
> +				bytes_read == 0 &&
> +				!g_web_result_has_headers(&session->result, NULL)) {
> +			local_error = g_error_new_literal(G_IO_ERROR,
> +				g_io_error_from_errno(ECONNRESET),
> +				g_strerror(ECONNRESET));
> +			passed_error = local_error;
> +		}
> +	}
> +
> +	call_result_func_failure(session, passed_error);
> +
> +	if (local_error)
> +		g_error_free(local_error);

This could be omitted if g_autoptr is used

>   }
>   
>   static bool received_data_continue(struct web_session *session,
> @@ -1330,9 +1381,11 @@ static gboolean received_data(GIOChannel *channel, GIOCondition cond,
>   							gpointer user_data)
>   {
>   	struct web_session *session = user_data;
> +	const gsize bytes_available = session->receive_space - 1;
>   	gsize bytes_read;
>   	GIOStatus status;
>   	GError *local_error = NULL;
> +	GError *error = NULL;

g_autoptr?

>   
>   	/* We received some data or condition, cancel the connect timeout. */
>   

Regards,
-Denis