[PATCH v3 00/18] Close Two GWeb Request "Bookend" Failure "Holes"

[PATCH v3 00/18] Close Two GWeb Request "Bookend" Failure "Holes"

[Grant Erickson]

There existed, prior to this set of changes, 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.

For WISPr, 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" and network service failover
failure.

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 second 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.

With these changes, WISPr now leverages the newly-added
'GWebResultFunc' GError parameter to refactor
'wispr_portal_web_result' into 'wispr_portal_web_result_failure' and
'wispr_portal_web_result_success' with the former handling
non-successful cases and the latter handling successful HTTP status
code cases.

With this change, the second, failure EOF case will now return a
non-null GError instance from 'GWebResultFunc' and will be handled as
a failure by 'wispr_portal_web_result_failure', "bookending" the
original "online" HTTP-based Internet reachability check.

All of the prior HTTP status code cases are handled by
'wispr_portal_web_result_success'.

This approachs works within the boundaries of the following constraints:

  1. The gio/gio.h header cannot be included in either src/wispr.c
     or src/service.c since that throws a bunch of symbol conflicts
     with gdbus/gdbus.h which redefines a number of symbols otherwise
     defined by gio/gio.h.

  2. 'handle_online_check_failure' in src/service.c needs to determine
     when a check failed due to cancellation.

     a. Historically, it looked at 'err == -ECANCELED' from the WISPr
        closure callback.

     b. However, due to (1), using 'g_error_matches(error, G_IO_ERROR,
        G_IO_ERROR_CANCELLED)' instead of 'err == -ECANCELED' cannot be
        used. So, passing 'const GError *error' all the way through will
        not work.

  3. Some HTTP status codes are failures insofar as WISPr is
     concerned. The codes are not important, the error messages are.

  4. WISPr failure messages need to be logged in src/service.c by
     'online_check_log_failure'. A simple null-terminated C string
     will do.

Grant Erickson (18):
  build: Require gio >= 2.40.
  build: Add @GIO_LIBS@ to clients of GWeb.
  gweb: Add 'G_WEB_ERROR' and 'g_web_error_quark'.
  gweb: Add 'G_WEB_ERROR' domain status codes.
  gweb: Leverage 'g_autofree' for 'port' local in
    'handle_resolved_address'.
  gweb: Add documentation to 'call_result_func'.
  gweb: Add documentation to 'g_web_result_get_status'.
  gweb: Add documentation to 'received_data'.
  gweb: Adopt optional, immutable GError instance for GWeb result
    errors.
  gweb: Added and leveraged 'call_result_func_{failure,success}'
    wrappers.
  gweb: Close web request session finalization 'hole'.
  gweb: Add documentation to 'received_data_{finalize,continue}'.
  gweb: Document 'struct _GWebResult'.
  wispr: Refactor 'wispr_portal_web_result' to leverage 'GError'.
  wispr: Document 'wispr_portal_web_result_failure'.
  wispr: Document 'wispr_portal_web_result_success'.
  wispr: Document 'wispr_portal_web_result'.
  wispr: Add an optional 'message' parameter to '__connman_wispr_cb_t'.

 Makefile.am      |  10 +-
 configure.ac     |   5 +
 gweb/gweb.c      | 391 +++++++++++++++++++++++++++++++++++++++++++----
 gweb/gweb.h      |  22 ++-
 src/6to4.c       |   3 +-
 src/connman.h    |   3 +-
 src/service.c    |  61 +++++---
 src/wispr.c      | 185 +++++++++++++++++-----
 tools/web-test.c |   2 +-
 tools/wispr.c    |   2 +-
 10 files changed, 586 insertions(+), 98 deletions(-)

-- 
2.45.0

[PATCH v3 01/18] build: Require gio >= 2.40.

[Grant Erickson]

Required for 'G_IO_ERROR' and 'g_io_error_from_errno' used by GWeb.
---
 configure.ac | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/configure.ac b/configure.ac
index 7430e2ec9c9b..cae36c9eb182 100644
--- a/configure.ac
+++ b/configure.ac
@@ -284,6 +284,11 @@ fi
 
 AC_DEFINE_UNQUOTED([STATS_MAX_FILE_SIZE], (${stats_max_file_size}), [Maximal size of a statistics round robin file])
 
+PKG_CHECK_MODULES(GIO, gio-2.0 >= 2.40, dummy=yes,
+				AC_MSG_ERROR(GIo >= 2.40 is required))
+AC_SUBST(GIO_CFLAGS)
+AC_SUBST(GIO_LIBS)
+
 PKG_CHECK_MODULES(GLIB, glib-2.0 >= 2.40, dummy=yes,
 				AC_MSG_ERROR(GLib >= 2.40 is required))
 AC_SUBST(GLIB_CFLAGS)
-- 
2.45.0

[PATCH v3 02/18] build: Add @GIO_LIBS@ to clients of GWeb.

[Grant Erickson]

GWeb relies on 'G_IO_ERROR' and 'g_io_error_from_errno'; consequently,
so too do its clients, including: connmand, connman-vpnd, wispr,
dnsproxy-test, and web-test.
---
 Makefile.am | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index 4d6425ffa28d..89662395c027 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -140,7 +140,7 @@ src_connmand_SOURCES += src/dns-systemd-resolved.c
 endif
 
 src_connmand_LDADD = gdbus/libgdbus-internal.la $(builtin_libadd) \
-			@GLIB_LIBS@ @DBUS_LIBS@ @GNUTLS_LIBS@ \
+			@GIO_LIBS@ @GLIB_LIBS@ @DBUS_LIBS@ @GNUTLS_LIBS@ \
 			-lresolv -ldl -lrt
 
 src_connmand_LDFLAGS = -Wl,--export-dynamic \
@@ -184,7 +184,7 @@ vpn_connman_vpnd_SOURCES = $(builtin_vpn_sources) $(backtrace_sources) \
 			vpn/vpn-config.c vpn/vpn-settings.c vpn/vpn-util.c
 
 vpn_connman_vpnd_LDADD = gdbus/libgdbus-internal.la $(builtin_vpn_libadd) \
-				@GLIB_LIBS@ @DBUS_LIBS@ @GNUTLS_LIBS@ \
+				@GIO_LIBS@ @GLIB_LIBS@ @DBUS_LIBS@ @GNUTLS_LIBS@ \
 				-lresolv -ldl
 
 vpn_connman_vpnd_LDFLAGS = -Wl,--export-dynamic \
@@ -330,7 +330,7 @@ if WISPR
 noinst_PROGRAMS += tools/wispr
 
 tools_wispr_SOURCES = $(gweb_sources) tools/wispr.c
-tools_wispr_LDADD = @GLIB_LIBS@ @GNUTLS_LIBS@ -lresolv
+tools_wispr_LDADD = @GIO_LIBS@ @GLIB_LIBS@ @GNUTLS_LIBS@ -lresolv
 endif
 
 if TOOLS
@@ -349,7 +349,7 @@ tools_supplicant_test_LDADD = gdbus/libgdbus-internal.la \
 				@GLIB_LIBS@ @DBUS_LIBS@
 
 tools_web_test_SOURCES = $(gweb_sources) tools/web-test.c
-tools_web_test_LDADD = @GLIB_LIBS@ @GNUTLS_LIBS@ -lresolv
+tools_web_test_LDADD = @GIO_LIBS@ @GLIB_LIBS@ @GNUTLS_LIBS@ -lresolv
 
 tools_resolv_test_SOURCES = gweb/gresolv.h gweb/gresolv.c tools/resolv-test.c
 tools_resolv_test_LDADD = @GLIB_LIBS@ -lresolv
@@ -411,7 +411,7 @@ unit_test_iptables_LDADD = @GLIB_LIBS@ -ldl
 endif
 
 tools_dnsproxy_test_SOURCES = tools/dnsproxy-test.c
-tools_dnsproxy_test_LDADD = @GLIB_LIBS@
+tools_dnsproxy_test_LDADD = @GIO_LIBS@ @GLIB_LIBS@
 
 endif
 
-- 
2.45.0

[PATCH v3 03/18] gweb: Add 'G_WEB_ERROR' and 'g_web_error_quark'.

[Grant Erickson]

These allow allocating, instantiating, and setting GWeb-specific GLib
GError structures.
---
 gweb/gweb.c | 8 ++++++++
 gweb/gweb.h | 4 ++++
 2 files changed, 12 insertions(+)

diff --git a/gweb/gweb.c b/gweb/gweb.c
index d79d2d0808cd..94d304bd40d6 100644
--- a/gweb/gweb.c
+++ b/gweb/gweb.c
@@ -157,6 +157,14 @@ static void _debug(GWeb *web, const char *file, const char *caller,
 	va_end(ap);
 }
 
+/**
+ *  Return the GWeb error quark.
+ *
+ *  @returns
+ *    The GWeb error GQuark.
+ */
+G_DEFINE_QUARK(g-web-error-quark, g_web_error)
+
 static inline void call_result_func(struct web_session *session, guint16 status)
 {
 
diff --git a/gweb/gweb.h b/gweb/gweb.h
index 90cccadbe883..6768601d6c69 100644
--- a/gweb/gweb.h
+++ b/gweb/gweb.h
@@ -119,6 +119,8 @@ typedef bool (*GWebInputFunc)(const guint8 **data, gsize *length,
 
 typedef void (*GWebDebugFunc)(const char *str, gpointer user_data);
 
+#define G_WEB_ERROR g_web_error_quark()
+
 GWeb *g_web_new(int index);
 
 GWeb *g_web_ref(GWeb *web);
@@ -183,6 +185,8 @@ void g_web_parser_feed_data(GWebParser *parser,
 				const guint8 *data, gsize length);
 void g_web_parser_end_data(GWebParser *parser);
 
+GQuark g_web_error_quark(void);
+
 #ifdef __cplusplus
 }
 #endif
-- 
2.45.0

[PATCH v3 04/18] gweb: Add 'G_WEB_ERROR' domain status codes.

[Grant Erickson]

These allow instantiating and setting GWeb-specific GLib GError
structures for G_WEB_ERROR host not found and chunked response
decoding errors.
---
 gweb/gweb.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/gweb/gweb.h b/gweb/gweb.h
index 6768601d6c69..fb29498e90ad 100644
--- a/gweb/gweb.h
+++ b/gweb/gweb.h
@@ -101,6 +101,22 @@ enum GWebStatusCode {
 	GWEB_HTTP_STATUS_CODE_NETWORK_AUTHENTICATION_REQUIRED = 511
 };
 
+/**
+ *  GWeb-specific error enumerations potentially set when
+ *  #GWebResultFunc is invoked on a failure.
+ */
+enum GWebErrorEnum {
+	/**
+	 *  An error occorred when decoding a chunked HTTP response.
+	 */
+	G_WEB_ERROR_CHUNK_DECODE,
+
+	/**
+	 *  A host could not be resolved.
+	 */
+	G_WEB_ERROR_HOST_NOT_FOUND,
+};
+
 struct _GWeb;
 struct _GWebResult;
 struct _GWebParser;
-- 
2.45.0

[PATCH v3 05/18] gweb: Leverage 'g_autofree' for 'port' local in 'handle_resolved_address'.

[Grant Erickson]

---
 gweb/gweb.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/gweb/gweb.c b/gweb/gweb.c
index 94d304bd40d6..ba3b62165f5e 100644
--- a/gweb/gweb.c
+++ b/gweb/gweb.c
@@ -2240,7 +2240,7 @@ done:
 static void handle_resolved_address(struct web_session *session)
 {
 	struct addrinfo hints;
-	char *port;
+	g_autofree char *port;
 	int ret;
 
 	debug(session->web, "address %s", session->address);
@@ -2256,7 +2256,6 @@ static void handle_resolved_address(struct web_session *session)
 
 	port = g_strdup_printf("%u", session->port);
 	ret = getaddrinfo(session->address, port, &hints, &session->addr);
-	g_free(port);
 	if (ret != 0 || !session->addr) {
 		call_result_func(session, GWEB_HTTP_STATUS_CODE_BAD_REQUEST);
 		return;
-- 
2.45.0

[PATCH v3 06/18] gweb: Add documentation to 'call_result_func'.

[Grant Erickson]

Add documentation to the 'call_result_func' function.
---
 gweb/gweb.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/gweb/gweb.c b/gweb/gweb.c
index ba3b62165f5e..e4dad4f95e42 100644
--- a/gweb/gweb.c
+++ b/gweb/gweb.c
@@ -165,6 +165,30 @@ static void _debug(GWeb *web, const char *file, const char *caller,
  */
 G_DEFINE_QUARK(g-web-error-quark, g_web_error)
 
+/**
+ *  @brief
+ *    Invoke the closure callback associated with the web session
+ *    request.
+ *
+ *  This closes the specified web session request by invoking the @a
+ *  result_func originally assigned in #do_request when the session
+ *  was first initiated.
+ *
+ *  @param[in]  session
+ *    A pointer to the mutable web session request for which to invoke
+ *    the closure callback.
+ *
+ *  @param[in]  status
+ *    HTTP status code on success to set in the @a session result
+ *    structure. Note that #GWEB_HTTP_STATUS_CODE_UNKNOWN acts as a
+ *    null value such that the status is only set if the value is
+ *    not #GWEB_HTTP_STATUS_CODE_UNKNOWN.
+ *
+ *  @sa do_request
+ *
+ *  @private
+ *
+ */
 static inline void call_result_func(struct web_session *session, guint16 status)
 {
 
-- 
2.45.0

[PATCH v3 07/18] gweb: Add documentation to 'g_web_result_get_status'.

[Grant Erickson]

Add documentation to the 'g_web_result_get_status' function.
---
 gweb/gweb.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/gweb/gweb.c b/gweb/gweb.c
index e4dad4f95e42..c22ca8ef0aff 100644
--- a/gweb/gweb.c
+++ b/gweb/gweb.c
@@ -2493,6 +2493,19 @@ bool g_web_cancel_request(GWeb *web, guint id)
 	return true;
 }
 
+/**
+ *  @brief
+ *    Returns the HTTP status code, if any, associated with the
+ *    web session request result.
+ *
+ *  @param[in]  result  A pointer to the immutable web session
+ *                      request result for which to return the
+ *                      HTTP status code.
+ *
+ *  @returns
+ *    The HTTP status code.
+ *
+ */
 guint16 g_web_result_get_status(GWebResult *result)
 {
 	if (!result)
-- 
2.45.0

[PATCH v3 08/18] gweb: Add documentation to 'received_data'.

[Grant Erickson]

Add documentation to the 'received_data' function.
---
 gweb/gweb.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/gweb/gweb.c b/gweb/gweb.c
index c22ca8ef0aff..ad91d782e6e4 100644
--- a/gweb/gweb.c
+++ b/gweb/gweb.c
@@ -1227,6 +1227,28 @@ static bool received_data_continue(struct web_session *session,
 	return TRUE;
 }
 
+/**
+ *  @brief
+ *    Handle a glib I/O channel watch received data delegation for a
+ *    web session request.
+ *
+ *  This handles a glib I/O channel received data delegate for the web
+ *  session request associated with @a channel.
+ *
+ *  @param[in,out]  channel    A pointer to the glib channel that
+ *                             received data or a condition(s)/
+ *                             event(s).
+ *  @param[in]      cond       The conditions or events that
+ *                             generated this delegation.
+ *  @param[in,out]  user_data  A pointer to the mutable web session
+ *                             request associated with @a channel.
+ *
+ *  @sa received_data_finalize
+ *  @sa received_data_continue
+ *
+ *  @private
+ *
+ */
 static gboolean received_data(GIOChannel *channel, GIOCondition cond,
 							gpointer user_data)
 {
-- 
2.45.0

[PATCH v3 09/18] gweb: Adopt optional, immutable GError instance for GWeb result errors.

[Grant Erickson]

In the following instances, GWeb can fail before initiating or while
processing a web request:

  1. TCP connection timeout to the remote server peer.
  2. Decoding a chunked response.
  3. Non-successful events (that is, G_IO_NVAL, G_IO_ERR, or G_IO_HUP)
     from 'g_io_add_watch'.
  4. GResolve "host not found" DNS forward resolution failure.
  5. Transport creation failure from 'create_transport'.

Rather than trying to overload 'GWEB_HTTP_STATUS_CODE_' error
enumerations from actual web server responses for these non-response
or response-in-progress failures, the signature of 'GWebResultFunc' is
amended to take an optional pointer to an immutable GError instance
which describes one of the five errors above on a GWeb request
failure.
---
 gweb/gweb.c      | 125 +++++++++++++++++++++++++++++++++++------------
 gweb/gweb.h      |   2 +-
 src/6to4.c       |   3 +-
 src/wispr.c      |   5 +-
 tools/web-test.c |   2 +-
 tools/wispr.c    |   2 +-
 6 files changed, 101 insertions(+), 38 deletions(-)

diff --git a/gweb/gweb.c b/gweb/gweb.c
index ad91d782e6e4..1915d3737eb0 100644
--- a/gweb/gweb.c
+++ b/gweb/gweb.c
@@ -39,6 +39,8 @@
 #include <netinet/tcp.h>
 #include <ifaddrs.h>
 
+#include <gio/gio.h>
+
 #include "giognutls.h"
 #include "gresolv.h"
 #include "gweb.h"
@@ -178,28 +180,26 @@ G_DEFINE_QUARK(g-web-error-quark, g_web_error)
  *    A pointer to the mutable web session request for which to invoke
  *    the closure callback.
  *
- *  @param[in]  status
- *    HTTP status code on success to set in the @a session result
- *    structure. Note that #GWEB_HTTP_STATUS_CODE_UNKNOWN acts as a
- *    null value such that the status is only set if the value is
- *    not #GWEB_HTTP_STATUS_CODE_UNKNOWN.
+ *  @param[in]  error
+ *    An optional pointer to the immutable GError structure containing
+ *    information about an error that occurred during the GWeb request,
+ *    if any.
  *
  *  @sa do_request
  *
  *  @private
  *
  */
-static inline void call_result_func(struct web_session *session, guint16 status)
+static void call_result_func(struct web_session *session,
+					const GError *error)
 {
+	debug(session->web, "session %p error %p result_func %p",
+		session, error, session->result_func);
 
 	if (!session->result_func)
 		return;
 
-	if (status != GWEB_HTTP_STATUS_CODE_UNKNOWN)
-		session->result.status = status;
-
-	session->result_func(&session->result, session->user_data);
-
+	session->result_func(error, &session->result, session->user_data);
 }
 
 static inline void call_route_func(struct web_session *session)
@@ -233,9 +233,14 @@ static inline void call_route_func(struct web_session *session)
 static gboolean connect_timeout_cb(gpointer user_data)
 {
 	struct web_session *session = user_data;
+	g_autofree char *message = NULL;
+	GError *local_error = NULL;
+
+	message = g_strdup_printf("connect timeout to %s after %ums",
+		session->address, g_web_get_connect_timeout(session->web));
 
-	debug(session->web, "session %p connect timeout after %ums",
-			session, g_web_get_connect_timeout(session->web));
+	debug(session->web, "session %p %s",
+			session, message);
 
 	session->connect_timeout = 0;
 
@@ -244,7 +249,14 @@ static gboolean connect_timeout_cb(gpointer user_data)
 	session->result.buffer = NULL;
 	session->result.length = 0;
 
-	call_result_func(session, GWEB_HTTP_STATUS_CODE_REQUEST_TIMEOUT);
+	local_error = g_error_new_literal(G_IO_ERROR,
+					G_IO_ERROR_TIMED_OUT,
+					message);
+
+	call_result_func(session, local_error);
+
+	if (local_error)
+		g_error_free(local_error);
 
 	return G_SOURCE_REMOVE;
 }
@@ -985,8 +997,7 @@ static int decode_chunked(struct web_session *session,
 			if (session->chunk_left <= len) {
 				session->result.buffer = ptr;
 				session->result.length = session->chunk_left;
-				call_result_func(session,
-					GWEB_HTTP_STATUS_CODE_UNKNOWN);
+				call_result_func(session, NULL);
 
 				len -= session->chunk_left;
 				ptr += session->chunk_left;
@@ -1001,8 +1012,7 @@ static int decode_chunked(struct web_session *session,
 			/* more data */
 			session->result.buffer = ptr;
 			session->result.length = len;
-			call_result_func(session,
-				GWEB_HTTP_STATUS_CODE_UNKNOWN);
+			call_result_func(session, NULL);
 
 			session->chunk_left -= len;
 			session->total_len += len;
@@ -1020,6 +1030,8 @@ static int handle_body(struct web_session *session,
 				const guint8 *buf, gsize len)
 {
 	int err;
+	g_autofree char *message = NULL;
+	GError *local_error = NULL;
 
 	debug(session->web, "[body] length %zu", len);
 
@@ -1027,19 +1039,28 @@ static int handle_body(struct web_session *session,
 		if (len > 0) {
 			session->result.buffer = buf;
 			session->result.length = len;
-			call_result_func(session,
-				GWEB_HTTP_STATUS_CODE_UNKNOWN);
+			call_result_func(session, NULL);
 		}
 		return 0;
 	}
 
 	err = decode_chunked(session, buf, len);
 	if (err < 0) {
-		debug(session->web, "Error in chunk decode %d", err);
+		message = g_strdup_printf("Error in chunk decode %d", err);
+
+		debug(session->web, message);
 
 		session->result.buffer = NULL;
 		session->result.length = 0;
-		call_result_func(session, GWEB_HTTP_STATUS_CODE_BAD_REQUEST);
+
+		local_error = g_error_new_literal(G_WEB_ERROR,
+			G_WEB_ERROR_CHUNK_DECODE,
+			message);
+
+		call_result_func(session, local_error);
+
+		if (local_error)
+			g_error_free(local_error);
 	}
 
 	return err;
@@ -1125,14 +1146,12 @@ static void add_header_field(struct web_session *session)
 
 static void received_data_finalize(struct web_session *session)
 {
-	const guint16 code = GWEB_HTTP_STATUS_CODE_UNKNOWN;
-
 	session->transport_watch = 0;
 
 	session->result.buffer = NULL;
 	session->result.length = 0;
 
-	call_result_func(session, code);
+	call_result_func(session, NULL);
 }
 
 static bool received_data_continue(struct web_session *session,
@@ -1255,6 +1274,7 @@ static gboolean received_data(GIOChannel *channel, GIOCondition cond,
 	struct web_session *session = user_data;
 	gsize bytes_read;
 	GIOStatus status;
+	GError *local_error = NULL;
 
 	/* We received some data or condition, cancel the connect timeout. */
 
@@ -1270,7 +1290,14 @@ static gboolean received_data(GIOChannel *channel, GIOCondition cond,
 		session->result.buffer = NULL;
 		session->result.length = 0;
 
-		call_result_func(session, GWEB_HTTP_STATUS_CODE_BAD_REQUEST);
+		local_error = g_error_new_literal(G_IO_ERROR,
+			g_io_error_from_errno(EIO),
+			g_strerror(EIO));
+
+		call_result_func(session, local_error);
+
+		if (local_error)
+			g_error_free(local_error);
 
 		return FALSE;
 	}
@@ -2288,6 +2315,8 @@ static void handle_resolved_address(struct web_session *session)
 	struct addrinfo hints;
 	g_autofree char *port;
 	int ret;
+	g_autofree char *message = NULL;
+	GError *local_error = NULL;
 
 	debug(session->web, "address %s", session->address);
 
@@ -2303,16 +2332,35 @@ static void handle_resolved_address(struct web_session *session)
 	port = g_strdup_printf("%u", session->port);
 	ret = getaddrinfo(session->address, port, &hints, &session->addr);
 	if (ret != 0 || !session->addr) {
-		call_result_func(session, GWEB_HTTP_STATUS_CODE_BAD_REQUEST);
-		return;
+		message = g_strdup_printf("could not resolve %s: %s",
+			session->address,
+			gai_strerror(ret));
+
+		local_error = g_error_new_literal(G_WEB_ERROR,
+			G_WEB_ERROR_HOST_NOT_FOUND,
+			message);
+
+		call_result_func(session, local_error);
+
+		goto done;
 	}
 
 	call_route_func(session);
 
-	if (create_transport(session) < 0) {
-		call_result_func(session, GWEB_HTTP_STATUS_CODE_CONFLICT);
-		return;
+	ret = create_transport(session);
+	if (ret < 0) {
+		local_error = g_error_new_literal(G_IO_ERROR,
+						g_io_error_from_errno(ret),
+						g_strerror(ret));
+
+		call_result_func(session, local_error);
+
+		goto done;
 	}
+
+ done:
+	if (local_error)
+		g_error_free(local_error);
 }
 
 static gboolean already_resolved(gpointer data)
@@ -2329,9 +2377,22 @@ static void resolv_result(GResolvResultStatus status,
 					char **results, gpointer user_data)
 {
 	struct web_session *session = user_data;
+	g_autofree char *message = NULL;
+	GError *local_error = NULL;
 
 	if (!results || !results[0]) {
-		call_result_func(session, GWEB_HTTP_STATUS_CODE_NOT_FOUND);
+		message = g_strdup_printf("could not resolve %s",
+			session->host);
+
+		local_error = g_error_new_literal(G_WEB_ERROR,
+				G_WEB_ERROR_HOST_NOT_FOUND,
+				message);
+
+		call_result_func(session, local_error);
+
+		if (local_error)
+			g_error_free(local_error);
+
 		return;
 	}
 
diff --git a/gweb/gweb.h b/gweb/gweb.h
index fb29498e90ad..75629500ef9c 100644
--- a/gweb/gweb.h
+++ b/gweb/gweb.h
@@ -125,7 +125,7 @@ typedef struct _GWeb GWeb;
 typedef struct _GWebResult GWebResult;
 typedef struct _GWebParser GWebParser;
 
-typedef bool (*GWebResultFunc)(GWebResult *result, gpointer user_data);
+typedef bool (*GWebResultFunc)(const GError *error, GWebResult *result, gpointer user_data);
 
 typedef bool (*GWebRouteFunc)(const char *addr, int ai_family,
 		int if_index, gpointer user_data);
diff --git a/src/6to4.c b/src/6to4.c
index 5542b339691a..fe1fee0848db 100644
--- a/src/6to4.c
+++ b/src/6to4.c
@@ -233,7 +233,8 @@ static gboolean unref_web(gpointer user_data)
 	return FALSE;
 }
 
-static bool web_result(GWebResult *result, gpointer user_data)
+static bool web_result(const GError *error,
+		GWebResult *result, gpointer user_data)
 {
 	guint16 status;
 
diff --git a/src/wispr.c b/src/wispr.c
index 7f850bc5f5aa..c3d8b13a9610 100644
--- a/src/wispr.c
+++ b/src/wispr.c
@@ -133,7 +133,7 @@ struct connman_wispr_portal {
 	struct connman_wispr_portal_context *ipv6_context;
 };
 
-static bool wispr_portal_web_result(GWebResult *result, gpointer user_data);
+static bool wispr_portal_web_result(const GError *error, GWebResult *result, gpointer user_data);
 
 /**
  *  A dictionary / hash table of network interface indices to
@@ -1001,7 +1001,8 @@ static bool wispr_manage_message(GWebResult *result,
 	return false;
 }
 
-static bool wispr_portal_web_result(GWebResult *result, gpointer user_data)
+static bool wispr_portal_web_result(const GError *error, GWebResult *result,
+		gpointer user_data)
 {
 	struct connman_wispr_portal_context *wp_context = user_data;
 	const char *redirect = NULL;
diff --git a/tools/web-test.c b/tools/web-test.c
index a65e6c115c41..f3efa22c4749 100644
--- a/tools/web-test.c
+++ b/tools/web-test.c
@@ -43,7 +43,7 @@ static void sig_term(int sig)
 	g_main_loop_quit(main_loop);
 }
 
-static bool web_result(GWebResult *result, gpointer user_data)
+static bool web_result(const GError *error, GWebResult *result, gpointer user_data)
 {
 	const guint8 *chunk;
 	gsize length;
diff --git a/tools/wispr.c b/tools/wispr.c
index ef7b60d3f5ca..0c794ccbc4c2 100644
--- a/tools/wispr.c
+++ b/tools/wispr.c
@@ -492,7 +492,7 @@ static bool wispr_route(const char *addr, int ai_family, int if_index,
 	return true;
 }
 
-static bool wispr_result(GWebResult *result, gpointer user_data)
+static bool wispr_result(const GError *error, GWebResult *result, gpointer user_data)
 {
 	struct wispr_session *wispr = user_data;
 	const guint8 *chunk;
-- 
2.45.0

[PATCH v3 10/18] gweb: Added and leveraged 'call_result_func_{failure,success}' wrappers.

[Grant Erickson]

These wrappers both self-document the GWeb result callback closures
and, for the success wrapper, reduce the parameter list by defaulting
the error parameter to 'NULL'.
---
 gweb/gweb.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 67 insertions(+), 9 deletions(-)

diff --git a/gweb/gweb.c b/gweb/gweb.c
index 1915d3737eb0..7ad12c977ff8 100644
--- a/gweb/gweb.c
+++ b/gweb/gweb.c
@@ -185,6 +185,8 @@ G_DEFINE_QUARK(g-web-error-quark, g_web_error)
  *    information about an error that occurred during the GWeb request,
  *    if any.
  *
+ *  @sa call_result_func_failure
+ *  @sa call_result_func_success
  *  @sa do_request
  *
  *  @private
@@ -202,6 +204,62 @@ static void call_result_func(struct web_session *session,
 	session->result_func(error, &session->result, session->user_data);
 }
 
+/**
+ *  @brief
+ *    Invoke the closure callback on failure associated with the web
+ *    session request.
+ *
+ *  This closes the specified web session request on failure with @a
+ *  error by invoking the @a result_func originally assigned in
+ *  #do_request when the session was first initiated.
+ *
+ *  @param[in]  session
+ *    A pointer to the mutable web session request for which to invoke
+ *    the closure callback.
+ *
+ *  @param[in]  error
+ *    A required pointer to the immutable GError structure containing
+ *    informatino about an error that occurred during the GWeb
+ *    request.
+ *
+ *  @sa call_result_func
+ *  @sa call_result_func_success
+ *  @sa do_request
+ *
+ *  @private
+ *
+ */
+static inline void call_result_func_failure(struct web_session *session,
+					const GError *error)
+{
+	call_result_func(session, error);
+}
+
+/**
+ *  @brief
+ *    Invoke the closure callback on success associated with the web
+ *    session request.
+ *
+ *  This closes the specified web session request on success by
+ *  invoking the @a result_func originally assigned in #do_request
+ *  when the session was first initiated.
+ *
+ *  @param[in]  session
+ *    A pointer to the mutable web session request for which to invoke
+ *    the closure callback.
+ *
+ *  @sa call_result_func
+ *  @sa call_result_func_failure
+ *  @sa do_request
+ *
+ *  @private
+ *
+ */
+static void call_result_func_success(struct web_session *session)
+{
+	call_result_func(session, NULL);
+}
+
 static inline void call_route_func(struct web_session *session)
 {
 	if (session->route_func)
@@ -253,7 +311,7 @@ static gboolean connect_timeout_cb(gpointer user_data)
 					G_IO_ERROR_TIMED_OUT,
 					message);
 
-	call_result_func(session, local_error);
+	call_result_func_failure(session, local_error);
 
 	if (local_error)
 		g_error_free(local_error);
@@ -997,7 +1055,7 @@ static int decode_chunked(struct web_session *session,
 			if (session->chunk_left <= len) {
 				session->result.buffer = ptr;
 				session->result.length = session->chunk_left;
-				call_result_func(session, NULL);
+				call_result_func_success(session);
 
 				len -= session->chunk_left;
 				ptr += session->chunk_left;
@@ -1012,7 +1070,7 @@ static int decode_chunked(struct web_session *session,
 			/* more data */
 			session->result.buffer = ptr;
 			session->result.length = len;
-			call_result_func(session, NULL);
+			call_result_func_success(session);
 
 			session->chunk_left -= len;
 			session->total_len += len;
@@ -1039,7 +1097,7 @@ static int handle_body(struct web_session *session,
 		if (len > 0) {
 			session->result.buffer = buf;
 			session->result.length = len;
-			call_result_func(session, NULL);
+			call_result_func_success(session);
 		}
 		return 0;
 	}
@@ -1057,7 +1115,7 @@ static int handle_body(struct web_session *session,
 			G_WEB_ERROR_CHUNK_DECODE,
 			message);
 
-		call_result_func(session, local_error);
+		call_result_func_failure(session, local_error);
 
 		if (local_error)
 			g_error_free(local_error);
@@ -1294,7 +1352,7 @@ static gboolean received_data(GIOChannel *channel, GIOCondition cond,
 			g_io_error_from_errno(EIO),
 			g_strerror(EIO));
 
-		call_result_func(session, local_error);
+		call_result_func_failure(session, local_error);
 
 		if (local_error)
 			g_error_free(local_error);
@@ -2340,7 +2398,7 @@ static void handle_resolved_address(struct web_session *session)
 			G_WEB_ERROR_HOST_NOT_FOUND,
 			message);
 
-		call_result_func(session, local_error);
+		call_result_func_failure(session, local_error);
 
 		goto done;
 	}
@@ -2353,7 +2411,7 @@ static void handle_resolved_address(struct web_session *session)
 						g_io_error_from_errno(ret),
 						g_strerror(ret));
 
-		call_result_func(session, local_error);
+		call_result_func_failure(session, local_error);
 
 		goto done;
 	}
@@ -2388,7 +2446,7 @@ static void resolv_result(GResolvResultStatus status,
 				G_WEB_ERROR_HOST_NOT_FOUND,
 				message);
 
-		call_result_func(session, local_error);
+		call_result_func_failure(session, local_error);
 
 		if (local_error)
 			g_error_free(local_error);
-- 
2.45.0

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

[Grant Erickson]

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(-)

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;
+	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;
+		}
+	} 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);
 }
 
 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;
 
 	/* We received some data or condition, cancel the connect timeout. */
 
@@ -1364,16 +1417,20 @@ static gboolean received_data(GIOChannel *channel, GIOCondition cond,
 
 	status = g_io_channel_read_chars(channel,
 				(gchar *) session->receive_buffer,
-				session->receive_space - 1, &bytes_read, NULL);
+				bytes_available, &bytes_read, &error);
 
-	debug(session->web, "bytes read %zu status %d", bytes_read,
-		status);
+	debug(session->web, "bytes read %zu status %d error %p", bytes_read,
+		status, error);
 
 	/* Handle post-channel read errors, which could be either
 	 * G_IO_STATUS_ERROR or G_IO_STATUS_EOF.
 	 */
 	if (status != G_IO_STATUS_NORMAL && status != G_IO_STATUS_AGAIN) {
-		received_data_finalize(session);
+		received_data_finalize(session, status,
+			bytes_available, bytes_read, error);
+
+		if (error != NULL)
+			g_clear_error(&error);
 
 		return FALSE;
 	}
@@ -2512,6 +2569,7 @@ static guint do_request(GWeb *web, const char *url,
 	debug(web, "host %s", session->host);
 	debug(web, "flags %lu", session->flags);
 	debug(web, "request %s", session->request);
+	debug(web, "result_func %p", func);
 
 	if (type) {
 		session->content_type = g_strdup(type);
-- 
2.45.0

[PATCH v3 12/18] gweb: Add documentation to 'received_data_{finalize,continue}'.

[Grant Erickson]

Add documentation to the 'received_data_{finalize,continue}' functions.
---
 gweb/gweb.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/gweb/gweb.c b/gweb/gweb.c
index 6d517f9467bb..b0f2077a176a 100644
--- a/gweb/gweb.c
+++ b/gweb/gweb.c
@@ -1202,6 +1202,43 @@ static void add_header_field(struct web_session *session)
 	}
 }
 
+/**
+ *  @brief
+ *    Finalize a glib I/O channel watch received data delegation for a
+ *    web session request.
+ *
+ *  This finalizes a glib I/O channel received data failure or
+ *  success for @a session. This assumes that @a status is either
+ *  #G_IO_STATUS_ERROR or #G_IO_STATUS_EOF. #G_IO_STATUS_ERROR
+ *  represents an unconditional failure completion. #G_IO_STATUS_EOF
+ *  may represent a successful completion, if it follows one more
+ *  prior received data transfers for @a session. However, it
+ *  represents a failure completion if it occurs prior to the receipt
+ *  of any @a session data.
+ *
+ *  @param[in,out]  session          A pointer to the mutable web
+ *                                   session request to finalize.
+ *  @param[in]      status           The status from the prior call
+ *                                   to #g_io_channel_read_chars used
+ *                                   to determine how to finalize @a
+ *                                   session.
+ *  @param[in]      bytes_available  The number of bytes advertised
+ *                                   to the prior call to
+ *                                   #g_io_channel_read_chars.
+ *  @param[in]      bytes_read       The number of bytes read by the
+ *                                   prior call to
+ *                                   #g_io_channel_read_chars.
+ *  @param[in]      error            An optional pointer to the glib
+ *                                   error instance associated the
+ *                                   prior call to
+ *                                   #g_io_channel_read_chars.
+ *
+ *  @sa received_data_continue
+ *  @sa received_data
+ *
+ *  @private
+ *
+ */
 static void received_data_finalize(struct web_session *session,
 				GIOStatus status, gsize bytes_available,
 				gsize bytes_read, const GError *error)
@@ -1263,6 +1300,23 @@ static void received_data_finalize(struct web_session *session,
 		g_error_free(local_error);
 }
 
+/**
+ *  @brief
+ *    Continue a glib I/O channel watch received data delegation for a
+ *    web session request and process the data from it.
+ *
+ *  @param[in,out]  session     A pointer to the mutable web session
+ *                              request to continue and process
+ *                              received data for.
+ *  @param[in]      bytes_read  The number of bytes read by the prior
+ *                              call to #g_io_channel_read_chars.
+ *
+ *  @sa received_data_finalize
+ *  @sa received_data
+ *
+ *  @private
+ *
+ */
 static bool received_data_continue(struct web_session *session,
 				gsize bytes_read)
 {
-- 
2.45.0

[PATCH v3 13/18] gweb: Document 'struct _GWebResult'.

[Grant Erickson]

Add documentation to the remainder of the 'struct _GWebResult' data
members and to the overall structure.
---
 gweb/gweb.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/gweb/gweb.c b/gweb/gweb.c
index b0f2077a176a..ace3ac4dcb69 100644
--- a/gweb/gweb.c
+++ b/gweb/gweb.c
@@ -56,12 +56,44 @@ enum chunk_state {
 	CHUNK_DATA,
 };
 
+/**
+ *	The opaque structure presented to GWeb clients on received data or
+ *	request closures.
+ *
+ *  @private
+ */
 struct _GWebResult {
+	/**
+	 *	HTTP status code on success.
+	 */
 	guint16 status;
+
+	/**
+	 *	HTTP body content on success; otherwise, NULL.
+	 */
 	const guint8 *buffer;
+
+	/**
+	 *	HTTP body length on success; otherwise, 0.
+	 */
 	gsize length;
+
+	/**
+	 *	Boolean indicating whether the HTTP response uses HTTP/1.1
+	 *	chunked transfer encoding.
+	 */
 	bool use_chunk;
+
+	/**
+	 *	An optional pointer to a null-terminated C string containing
+	 *	the last HTTP header name added as part of a header key/value
+	 *	pair.
+	 */
 	gchar *last_key;
+
+	/**
+	 *	HTTP headers, on success, keyed by the header name.
+	 */
 	GHashTable *headers;
 };
 
-- 
2.45.0

[PATCH v3 14/18] wispr: Refactor 'wispr_portal_web_result' to leverage 'GError'.

[Grant Erickson]

Leverages the newly-added GError parameter to the 'GWebResultFunc' to
refactor 'wispr_portal_web_result' into
'wispr_portal_web_result_failure' and
'wispr_portal_web_result_success' with the former handling
non-successful cases and the latter handling successful HTTP status
code cases.

Prior to the introduction of an optional, immutable 'GError' parameter
to 'GWebResultFunc' and this refactor, there existed a "hole" in the
finalization of WISPr IPv4 and IPv6 "online" HTTP-based Internet
reachability checks web request sessions that both could and did leave
such checks "dangling" and non-renewing such that an expected active,
default network service failover does not occur when it should.

The "hole" 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 of the request response are fulfilled. When there
is no further data to send, the final receive completes a normal EOF
that leads to the successful closure of the web request, typically
with HTTP 200 "OK" in the nominal success case.

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. Prior to this change, all such failures hit the same
WISPr error-handling block which effectively maps such a failure to the
effective 'GWebResult' "null" GWEB_HTTP_STATUS_CODE_UNKNOWN status
value. Unfortunately, WISPr historically had 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".

With this change, the second, failure EOF case will now return a
GError instance pointer synthesized from 'ECONNRESET' and will be
handled as a failure by 'wispr_portal_web_result_failure',
"bookending" the original "online" HTTP-based Internet reachability
check.

All of the prior HTTP status code cases are handled by
'wispr_portal_web_result_success'.
---
 src/wispr.c | 105 +++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 71 insertions(+), 34 deletions(-)

diff --git a/src/wispr.c b/src/wispr.c
index c3d8b13a9610..3f42e131608f 100644
--- a/src/wispr.c
+++ b/src/wispr.c
@@ -652,6 +652,9 @@ static void wispr_portal_error(struct connman_wispr_portal_context *wp_context)
  *                              associated with the unsuccessful
  *                              "online" HTTP-based Internet
  *                              reachability check.
+ *  @param[in]      message     A pointer to an immutable null-
+ *                              terminated C string describing the
+ *                              reason for the online check failure.
  *
  *  @sa portal_manage_success_status
  *  @sa wispr_portal_web_result_no_err
@@ -663,7 +666,8 @@ static void wispr_portal_error(struct connman_wispr_portal_context *wp_context)
  */
 static void portal_manage_failure_status(
 			struct connman_wispr_portal_context *wp_context,
-			int err)
+			int err,
+			const char *message)
 {
 	struct connman_service *service = wp_context->service;
 	enum connman_ipconfig_type type = wp_context->type;
@@ -810,8 +814,10 @@ static void wispr_portal_request_portal(
 					wispr_route_request,
 					wp_context, &err);
 
+	DBG("wp_context->request_id %d err %d", wp_context->request_id, err);
+
 	if (wp_context->request_id == 0) {
-		portal_manage_failure_status(wp_context, err);
+		portal_manage_failure_status(wp_context, -err, strerror(-err));
 		wispr_portal_error(wp_context);
 		wispr_portal_context_unref(wp_context);
 	}
@@ -1001,35 +1007,22 @@ static bool wispr_manage_message(GWebResult *result,
 	return false;
 }
 
-static bool wispr_portal_web_result(const GError *error, GWebResult *result,
-		gpointer user_data)
+static void wispr_portal_web_result_failure(const GError *error,
+		GWebResult *result,
+		struct connman_wispr_portal_context *wp_context)
 {
-	struct connman_wispr_portal_context *wp_context = user_data;
-	const char *redirect = NULL;
-	const guint8 *chunk = NULL;
-	const char *str = NULL;
-	guint16 status;
-	gsize length;
-
-	DBG("");
-
-	if (wp_context->wispr_result != CONNMAN_WISPR_RESULT_ONLINE) {
-		g_web_result_get_chunk(result, &chunk, &length);
-
-		if (length > 0) {
-			g_web_parser_feed_data(wp_context->wispr_parser,
-								chunk, length);
-			/* read more data */
-			return true;
-		}
+	portal_manage_failure_status(wp_context, 0, error->message);
 
-		g_web_parser_end_data(wp_context->wispr_parser);
+	free_wispr_routes(wp_context);
+	wp_context->request_id = 0;
+}
 
-		if (wp_context->wispr_msg.message_type >= 0) {
-			if (wispr_manage_message(result, wp_context))
-				goto done;
-		}
-	}
+static void wispr_portal_web_result_success(GWebResult *result,
+		struct connman_wispr_portal_context *wp_context)
+{
+	guint16 status;
+	const char *str = NULL;
+	const char *redirect = NULL;
 
 	status = g_web_result_get_status(result);
 
@@ -1085,15 +1078,15 @@ static bool wispr_portal_web_result(const GError *error, GWebResult *result,
 
 		goto done;
 	case GWEB_HTTP_STATUS_CODE_BAD_REQUEST:
-		portal_manage_failure_status(wp_context, -EINVAL);
+		portal_manage_failure_status(wp_context, 0, "bad request");
 		break;
 
 	case GWEB_HTTP_STATUS_CODE_NOT_FOUND:
-		portal_manage_failure_status(wp_context, -ENOENT);
+		portal_manage_failure_status(wp_context, 0, "resource not found");
 		break;
 
 	case GWEB_HTTP_STATUS_CODE_REQUEST_TIMEOUT:
-		portal_manage_failure_status(wp_context, -ETIMEDOUT);
+		portal_manage_failure_status(wp_context, 0, "request timeout");
 		break;
 
 	case GWEB_HTTP_STATUS_CODE_HTTP_VERSION_NOT_SUPPORTED:
@@ -1102,12 +1095,55 @@ static bool wispr_portal_web_result(const GError *error, GWebResult *result,
 				wispr_portal_browser_reply_cb,
 				wp_context->status_url, wp_context);
 		break;
+
 	default:
 		break;
 	}
 
 	free_wispr_routes(wp_context);
 	wp_context->request_id = 0;
+
+done:
+	return;
+}
+
+static bool wispr_portal_web_result(const GError *error, GWebResult *result,
+		gpointer user_data)
+{
+	struct connman_wispr_portal_context *wp_context = user_data;
+	const guint8 *chunk = NULL;
+	gsize length;
+
+	DBG("error %p result %p user_data %p wispr_result %d",
+		error, result, user_data, wp_context->wispr_result);
+
+	if (wp_context->wispr_result != CONNMAN_WISPR_RESULT_ONLINE) {
+		g_web_result_get_chunk(result, &chunk, &length);
+
+		DBG("length %zu", length);
+
+		if (length > 0) {
+			g_web_parser_feed_data(wp_context->wispr_parser,
+								chunk, length);
+			/* read more data */
+			return true;
+		}
+
+		g_web_parser_end_data(wp_context->wispr_parser);
+
+		DBG("wp_context->wispr_msg.message_type %d", wp_context->wispr_msg.message_type);
+
+		if (wp_context->wispr_msg.message_type >= 0) {
+			if (wispr_manage_message(result, wp_context))
+				goto done;
+		}
+	}
+
+	if (error)
+		wispr_portal_web_result_failure(error, result, wp_context);
+	else
+		wispr_portal_web_result_success(result, wp_context);
+
 done:
 	wp_context->wispr_msg.message_type = -1;
 
@@ -1117,6 +1153,7 @@ done:
 	 * maintaining a weak reference to it.
 	 */
 	wispr_portal_context_unref(wp_context);
+
 	return false;
 }
 
@@ -1196,7 +1233,7 @@ static void proxy_callback(const char *proxy, void *user_data)
 	if (!proxy) {
 		wispr_log_proxy_failure(wp_context, "No valid proxy");
 
-		portal_manage_failure_status(wp_context, -EINVAL);
+		portal_manage_failure_status(wp_context, 0, "no valid proxy");
 
 		return;
 	}
@@ -1543,7 +1580,7 @@ int __connman_wispr_start(struct connman_service *service,
 free_wp:
 	DBG("err %d wp_context %p", err, wp_context);
 
-	portal_manage_failure_status(wp_context, err);
+	portal_manage_failure_status(wp_context, -err, strerror(-err));
 
 	g_hash_table_remove(wispr_portal_hash, GINT_TO_POINTER(index));
 	return err;
@@ -1629,7 +1666,7 @@ int __connman_wispr_cancel(struct connman_service *service,
 
 	cancel_connman_wispr_portal_context(wp_context);
 
-	portal_manage_failure_status(wp_context, -ECANCELED);
+	portal_manage_failure_status(wp_context, -ECANCELED, strerror(-ECANCELED));
 
 	wispr_portal_context_unref(wp_context);
 
-- 
2.45.0

[PATCH v3 15/18] wispr: Document 'wispr_portal_web_result_failure'.

[Grant Erickson]

Add documentation to the 'wispr_portal_web_result_failure' function.
---
 src/wispr.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/src/wispr.c b/src/wispr.c
index 3f42e131608f..76a7ba219921 100644
--- a/src/wispr.c
+++ b/src/wispr.c
@@ -657,7 +657,7 @@ static void wispr_portal_error(struct connman_wispr_portal_context *wp_context)
  *                              reason for the online check failure.
  *
  *  @sa portal_manage_success_status
- *  @sa wispr_portal_web_result_no_err
+ *  @sa wispr_portal_web_result_failure
  *  @sa wispr_portal_web_result_err
  *  @sa wispr_portal_web_result
  *
@@ -1007,6 +1007,28 @@ static bool wispr_manage_message(GWebResult *result,
 	return false;
 }
 
+/**
+ *  @brief
+ *    Handle closure and finalization of a web request associated with
+ *    an unsuccessful "online" HTTP-based Internet reachability check.
+ *
+ *  @param[in]      error       A pointer to the immutable GLib GError
+ *                              instance associated the web request
+ *                              failure.
+ *  @param[in]      result      A pointer to the mutable web request
+ *                              result being finalized.
+ *  @param[in,out]  wp_context  A pointer to the mutable WISPr portal
+ *                              detection context associated with the
+ *                              unsuccessful "online" HTTP-based
+ *                              Internet reachability check this is
+ *                              finalizing.
+ *
+ *  @sa wispr_portal_web_result
+ *  @sa wispr_portal_web_result_no_err
+ *
+ *  @private
+ *
+ */
 static void wispr_portal_web_result_failure(const GError *error,
 		GWebResult *result,
 		struct connman_wispr_portal_context *wp_context)
-- 
2.45.0

[PATCH v3 16/18] wispr: Document 'wispr_portal_web_result_success'.

[Grant Erickson]

Add documentation to the 'wispr_portal_web_result_success' function.
---
 src/wispr.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/src/wispr.c b/src/wispr.c
index 76a7ba219921..e69e90551979 100644
--- a/src/wispr.c
+++ b/src/wispr.c
@@ -658,7 +658,7 @@ static void wispr_portal_error(struct connman_wispr_portal_context *wp_context)
  *
  *  @sa portal_manage_success_status
  *  @sa wispr_portal_web_result_failure
- *  @sa wispr_portal_web_result_err
+ *  @sa wispr_portal_web_result_success
  *  @sa wispr_portal_web_result
  *
  *  @private
@@ -1024,7 +1024,7 @@ static bool wispr_manage_message(GWebResult *result,
  *                              finalizing.
  *
  *  @sa wispr_portal_web_result
- *  @sa wispr_portal_web_result_no_err
+ *  @sa wispr_portal_web_result_success
  *
  *  @private
  *
@@ -1039,6 +1039,25 @@ static void wispr_portal_web_result_failure(const GError *error,
 	wp_context->request_id = 0;
 }
 
+/**
+ *  @brief
+ *    Handle closure and finalization of a web request associated with
+ *    a successful "online" HTTP-based Internet reachability check.
+ *
+ *  @param[in]      result      A pointer to the mutable web request
+ *                              result being finalized.
+ *  @param[in,out]  wp_context  A pointer to the mutable WISPr portal
+ *                              detection context associated with the
+ *                              successful "online" HTTP-based
+ *                              Internet reachability check this is
+ *                              finalizing.
+ *
+ *  @sa wispr_portal_web_result
+ *  @sa wispr_portal_web_result_failure
+ *
+ *  @private
+ *
+ */
 static void wispr_portal_web_result_success(GWebResult *result,
 		struct connman_wispr_portal_context *wp_context)
 {
-- 
2.45.0

[PATCH v3 17/18] wispr: Document 'wispr_portal_web_result'.

[Grant Erickson]

Add documentation to the 'wispr_portal_web_result' function.
---
 src/wispr.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/src/wispr.c b/src/wispr.c
index e69e90551979..913c3135fffb 100644
--- a/src/wispr.c
+++ b/src/wispr.c
@@ -1148,6 +1148,36 @@ done:
 	return;
 }
 
+/**
+ *  @brief
+ *    Handle the receipt of new data and the potential closure and
+ *    finalization of a web request.
+ *
+ *  This handles the receipt of new data associated with @a result and
+ *  the potential closure and finalization of it, if appropriate.
+ *
+ *  @param[in]      error      An optional pointer to the immutable
+ *                             GLib GError instance associated the web
+ *                             request, if it failed.
+ *  @param[in]      result     A pointer to the mutable web request
+ *                             result with data to process or to be
+ *                             finalized.
+ *  @param[in,out]  user_data  A pointer to the mutable WISPr portal
+ *                             detection context associated with the
+ *                             "online" HTTP-based Internet
+ *                             reachability check this is finalizing.
+ *
+ *  @returns
+ *    True if web request should wait for and process further data;
+ *    otherwise, false.
+ *
+ *  @sa wispr_portal_web_result_failure
+ *  @sa wispr_portal_web_result_success
+ *  @sa wispr_manage_message
+ *
+ *  @private
+ *
+ */
 static bool wispr_portal_web_result(const GError *error, GWebResult *result,
 		gpointer user_data)
 {
-- 
2.45.0

[PATCH v3 18/18] wispr: Add an optional 'message' parameter to '__connman_wispr_cb_t'.

[Grant Erickson]

'message' is an optional pointer to an immutable null- terminated C
string describing the reason for the online check failure.

In the following instances, GWeb (which WISPr utilizes to make an
online reachability check request) can fail before initiating or while
processing a web request:

  1. TCP connection timeout to the remote server peer.
  2. Decoding a chunked response.
  3. Non-successful events (that is, G_IO_NVAL, G_IO_ERR, or G_IO_HUP)
     from 'g_io_add_watch'.
  4. GResolve "host not found" DNS forward resolution failure.
  5. Transport creation failure from 'create_transport'.
  6. Unexpected remote peer disconnect after connect but before data
     transfer.

When an online check request fails, 'online_check_log_failure' in
src/service.c logs the failure in the form:

  "Interface <interface> [ <technology> ] IPv[46] online check to
   <online check URL> failed: <message>"

where <message> is the contents of this newly-added callback
parameter. This avoids 'online_check_log_failure' having to invoke
'strerror' as the message has already been pre-created and -formatted
by the error source. So, for the failures above, the messages would
be:

  1. "connect timeout to <IP address> after <timeout in
     milliseconds>ms".
  2. "Error in chunk decode <error>".
  3. "I/O error".
  4. "could not resolve <host name>".
  5. Output from 'strerror'.
  6. "Connection reset by peer".
---
 src/connman.h |  3 ++-
 src/service.c | 61 +++++++++++++++++++++++++++++++++++----------------
 src/wispr.c   |  4 ++--
 3 files changed, 46 insertions(+), 22 deletions(-)

diff --git a/src/connman.h b/src/connman.h
index 32ba55912ad6..7ebda7af3a55 100644
--- a/src/connman.h
+++ b/src/connman.h
@@ -550,7 +550,8 @@ void __connman_wpad_stop(struct connman_service *service);
 typedef void (*__connman_wispr_cb_t) (struct connman_service *service,
 				enum connman_ipconfig_type type,
 				bool success,
-				int err);
+				int err,
+				const char *message);
 
 int __connman_wispr_init(void);
 void __connman_wispr_cleanup(void);
diff --git a/src/service.c b/src/service.c
index c44335d9c962..f73f42eba9b0 100644
--- a/src/service.c
+++ b/src/service.c
@@ -240,7 +240,8 @@ static void service_list_sort(const char *function);
 static void complete_online_check(struct connman_service *service,
 					enum connman_ipconfig_type type,
 					bool success,
-					int err);
+					int err,
+					const char *message);
 static bool service_downgrade_online_state(struct connman_service *service);
 static bool connman_service_is_default(const struct connman_service *service);
 static int start_online_check_if_connected(struct connman_service *service);
@@ -3451,27 +3452,27 @@ static bool handle_online_check_success(struct connman_service *service,
  *                       check failure.
  *  @param[in]  type     The IP configuration type for which
  *                       the online check failed.
- *  @param[in]  err      The error status, in the POSIX domain,
- *                       associated with the online check failure.
+ *  @param[in]  message  A pointer to an immutable null-terminated
+ *                       C string describing the reason for the online
+ *                       check failure.
  *
  */
 static void online_check_log_failure(const struct connman_service *service,
 			enum connman_ipconfig_type type,
-			int err)
+			const char *message)
 {
 	g_autofree char *interface = NULL;
 
 	interface = connman_service_get_interface(service);
 
-	connman_warn("Interface %s [ %s ] %s online check to %s failed: %d: %s",
+	connman_warn("Interface %s [ %s ] %s online check to %s failed: %s",
 		interface,
 		__connman_service_type2string(service->type),
 		__connman_ipconfig_type2string(type),
 		type == CONNMAN_IPCONFIG_TYPE_IPV4 ?
 			connman_setting_get_string("OnlineCheckIPv4URL") :
 			connman_setting_get_string("OnlineCheckIPv6URL"),
-		err,
-		strerror(-err));
+			message);
 }
 
 /**
@@ -3507,6 +3508,10 @@ static void online_check_log_failure(const struct connman_service *service,
  *                                      the failed previously-requested
  *                                      online check. This is expected
  *                                      to be less than zero ('< 0').
+ *  @param[in]      message             A pointer to an immutable null-
+ *                                      terminated C string describing
+ *                                      the reason for the online
+ *                                      check failure.
  *
  *  @returns
  *    True, unconditionally.
@@ -3520,7 +3525,8 @@ static bool handle_oneshot_online_check_failure(
 			enum connman_ipconfig_type type,
 			enum connman_service_state ipconfig_state,
 			struct online_check_state *online_check_state,
-			int err)
+			int err,
+			const char *message)
 {
 	const bool reschedule = true;
 
@@ -3562,6 +3568,10 @@ static bool handle_oneshot_online_check_failure(
  *                                      the failed previously-requested
  *                                      online check. This is expected
  *                                      to be less than zero ('< 0').
+ *  @param[in]      message             A pointer to an immutable null-
+ *                                      terminated C string describing
+ *                                      the reason for the online
+ *                                      check failure.
  *
  *  @returns
  *    True if another online check should be scheduled; otherwise,
@@ -3576,7 +3586,8 @@ static bool handle_continuous_online_check_failure(
 			enum connman_ipconfig_type type,
 			enum connman_service_state ipconfig_state,
 			struct online_check_state *online_check_state,
-			int err)
+			int err,
+			const char *message)
 {
 	bool reschedule = false;
 
@@ -3699,6 +3710,10 @@ static bool handle_continuous_online_check_failure(
  *                                      the failed previously-requested
  *                                      online check. This is expected
  *                                      to be less than zero ('< 0').
+ *  @param[in]      message             A pointer to an immutable null-
+ *                                      terminated C string describing
+ *                                      the reason for the online
+ *                                      check failure.
  *
  *  @returns
  *    True if another online check should be scheduled; otherwise,
@@ -3714,17 +3729,18 @@ static bool handle_online_check_failure(struct connman_service *service,
 				enum connman_service_state ipconfig_state,
 				struct online_check_state *online_check_state,
 				bool oneshot,
-				int err)
+				int err,
+				const char *message)
 {
 	bool reschedule = false;
 
 	DBG("service %p (%s) type %d (%s) state %d (%s) "
-		"one-shot %u err %d (%s)\n",
+		"one-shot %u err %d message %s\n",
 		service,
 		connman_service_get_identifier(service),
 		type, __connman_ipconfig_type2string(type),
 		ipconfig_state, state2string(ipconfig_state),
-		oneshot, err, strerror(-err));
+		oneshot, err, message ? message : "<null>");
 
 	/*
 	 * Regardless of online check mode, if this completion closure
@@ -3740,7 +3756,7 @@ static bool handle_online_check_failure(struct connman_service *service,
 
 	/* Unconditionally log the failure, regardless of online check mode. */
 
-	online_check_log_failure(service, type, err);
+	online_check_log_failure(service, type, message);
 
 	/* Handle the failure according to the online check mode. */
 
@@ -3750,14 +3766,16 @@ static bool handle_online_check_failure(struct connman_service *service,
 						type,
 						ipconfig_state,
 						online_check_state,
-						err);
+						err,
+						message);
 	else
 		reschedule = handle_continuous_online_check_failure(
 						service,
 						type,
 						ipconfig_state,
 						online_check_state,
-						err);
+						err,
+						message);
 
 done:
 	return reschedule;
@@ -3803,6 +3821,9 @@ done:
  *                           to be zero ('0') if @a success is @a true
  *                           and less than zero ('< 0') if @a success
  *                           is @a false.
+ *  @param[in]      message  An optional pointer to an immutable null-
+ *                           terminated C string describing the reason
+ *                           for the online check failure.
  *
  *  @sa cancel_online_check
  *  @sa start_online_check
@@ -3816,7 +3837,8 @@ done:
 static void complete_online_check(struct connman_service *service,
 					enum connman_ipconfig_type type,
 					bool success,
-					int err)
+					int err,
+					const char *message)
 {
 	const bool oneshot = __connman_service_is_online_check_mode(
 		CONNMAN_SERVICE_ONLINE_CHECK_MODE_ONE_SHOT);
@@ -3825,11 +3847,11 @@ static void complete_online_check(struct connman_service *service,
 	bool reschedule = false;
 
 	DBG("service %p (%s) type %d (%s) "
-		"success %u err %d (%s)\n",
+		"success %u err %d message %s\n",
 		service,
 		connman_service_get_identifier(service),
 		type, __connman_ipconfig_type2string(type),
-		success, err, strerror(-err));
+		success, err, message ? message : "<null>");
 
 	if (type == CONNMAN_IPCONFIG_TYPE_IPV4) {
 		online_check_state = &service->online_check_state_ipv4;
@@ -3851,7 +3873,8 @@ static void complete_online_check(struct connman_service *service,
 					 ipconfig_state,
 					 online_check_state,
 					 oneshot,
-					 err);
+					 err,
+					 message);
 
 	DBG("reschedule online check %u", reschedule);
 
diff --git a/src/wispr.c b/src/wispr.c
index 913c3135fffb..189961936d3b 100644
--- a/src/wispr.c
+++ b/src/wispr.c
@@ -672,7 +672,7 @@ static void portal_manage_failure_status(
 	struct connman_service *service = wp_context->service;
 	enum connman_ipconfig_type type = wp_context->type;
 
-	wp_context->cb(service, type, false, err);
+	wp_context->cb(service, type, false, err, message);
 }
 
 /**
@@ -724,7 +724,7 @@ static void portal_manage_success_status(GWebResult *result,
 				&str))
 		connman_info("Client-Timezone: %s", str);
 
-	wp_context->cb(service, type, true, 0);
+	wp_context->cb(service, type, true, 0, NULL);
 }
 
 static bool wispr_route_request(const char *address, int ai_family,
-- 
2.45.0

Re: [PATCH v3 01/18] build: Require gio >= 2.40.

[Denis Kenzior]

Hi Grant,

On 3/20/25 6:38 PM, Grant Erickson wrote:
> Required for 'G_IO_ERROR' and 'g_io_error_from_errno' used by GWeb.
> ---
>   configure.ac | 5 +++++
>   1 file changed, 5 insertions(+)

Note that g_autofree requires GLib 2.44.  I've added a commit for that prior to 
this commit and amended this commit to require GIO 2.44 for consistency.

Regards,
-Denis

Re: [PATCH v3 00/18] Close Two GWeb Request "Bookend" Failure "Holes"

[patchwork-bot+connman]

Hello:

This series was applied to connman.git (master)
by Denis Kenzior <denkenz@gmail.com>:

On Thu, 20 Mar 2025 16:38:51 -0700 you wrote:
> There existed, prior to this set of changes, 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.
> 
> [...]

Here is the summary with links:
  - [v3,01/18] build: Require gio >= 2.40.
    (no matching commit)
  - [v3,02/18] build: Add @GIO_LIBS@ to clients of GWeb.
    https://git.kernel.org/pub/scm/network/connman/connman.git/?id=102589024ed8
  - [v3,03/18] gweb: Add 'G_WEB_ERROR' and 'g_web_error_quark'.
    https://git.kernel.org/pub/scm/network/connman/connman.git/?id=cfa99db4f71e
  - [v3,04/18] gweb: Add 'G_WEB_ERROR' domain status codes.
    https://git.kernel.org/pub/scm/network/connman/connman.git/?id=3bd76c5b400c
  - [v3,05/18] gweb: Leverage 'g_autofree' for 'port' local in 'handle_resolved_address'.
    https://git.kernel.org/pub/scm/network/connman/connman.git/?id=31230cab01f1
  - [v3,06/18] gweb: Add documentation to 'call_result_func'.
    https://git.kernel.org/pub/scm/network/connman/connman.git/?id=4c342f33d22d
  - [v3,07/18] gweb: Add documentation to 'g_web_result_get_status'.
    https://git.kernel.org/pub/scm/network/connman/connman.git/?id=95db0a2c0adb
  - [v3,08/18] gweb: Add documentation to 'received_data'.
    https://git.kernel.org/pub/scm/network/connman/connman.git/?id=d104ac6f0f71
  - [v3,09/18] gweb: Adopt optional, immutable GError instance for GWeb result errors.
    (no matching commit)
  - [v3,10/18] gweb: Added and leveraged 'call_result_func_{failure,success}' wrappers.
    (no matching commit)
  - [v3,11/18] gweb: Close web request session finalization 'hole'.
    (no matching commit)
  - [v3,12/18] gweb: Add documentation to 'received_data_{finalize,continue}'.
    (no matching commit)
  - [v3,13/18] gweb: Document 'struct _GWebResult'.
    https://git.kernel.org/pub/scm/network/connman/connman.git/?id=acb4bb421530
  - [v3,14/18] wispr: Refactor 'wispr_portal_web_result' to leverage 'GError'.
    (no matching commit)
  - [v3,15/18] wispr: Document 'wispr_portal_web_result_failure'.
    (no matching commit)
  - [v3,16/18] wispr: Document 'wispr_portal_web_result_success'.
    (no matching commit)
  - [v3,17/18] wispr: Document 'wispr_portal_web_result'.
    (no matching commit)
  - [v3,18/18] wispr: Add an optional 'message' parameter to '__connman_wispr_cb_t'.
    (no matching commit)

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH v3 09/18] gweb: Adopt optional, immutable GError instance for GWeb result errors.

[Denis Kenzior]

Hi Grant,

On 3/20/25 6:39 PM, Grant Erickson wrote:
> In the following instances, GWeb can fail before initiating or while
> processing a web request:
> 
>    1. TCP connection timeout to the remote server peer.
>    2. Decoding a chunked response.
>    3. Non-successful events (that is, G_IO_NVAL, G_IO_ERR, or G_IO_HUP)
>       from 'g_io_add_watch'.
>    4. GResolve "host not found" DNS forward resolution failure.
>    5. Transport creation failure from 'create_transport'.
> 
> Rather than trying to overload 'GWEB_HTTP_STATUS_CODE_' error
> enumerations from actual web server responses for these non-response
> or response-in-progress failures, the signature of 'GWebResultFunc' is
> amended to take an optional pointer to an immutable GError instance
> which describes one of the five errors above on a GWeb request
> failure.

This looks so much better, a few nits:

> ---
>   gweb/gweb.c      | 125 +++++++++++++++++++++++++++++++++++------------
>   gweb/gweb.h      |   2 +-
>   src/6to4.c       |   3 +-
>   src/wispr.c      |   5 +-
>   tools/web-test.c |   2 +-
>   tools/wispr.c    |   2 +-
>   6 files changed, 101 insertions(+), 38 deletions(-)
> 

<snip>

> @@ -233,9 +233,14 @@ static inline void call_route_func(struct web_session *session)
>   static gboolean connect_timeout_cb(gpointer user_data)
>   {
>   	struct web_session *session = user_data;
> +	g_autofree char *message = NULL;
> +	GError *local_error = NULL;

Could you use:

g_autoptr(GError) local_error = NULL;

...

> +
> +	message = g_strdup_printf("connect timeout to %s after %ums",
> +		session->address, g_web_get_connect_timeout(session->web));
>   
> -	debug(session->web, "session %p connect timeout after %ums",
> -			session, g_web_get_connect_timeout(session->web));
> +	debug(session->web, "session %p %s",
> +			session, message);
>   
>   	session->connect_timeout = 0;
>   
> @@ -244,7 +249,14 @@ static gboolean connect_timeout_cb(gpointer user_data)
>   	session->result.buffer = NULL;
>   	session->result.length = 0;
>   
> -	call_result_func(session, GWEB_HTTP_STATUS_CODE_REQUEST_TIMEOUT);
> +	local_error = g_error_new_literal(G_IO_ERROR,
> +					G_IO_ERROR_TIMED_OUT,
> +					message);
> +
> +	call_result_func(session, local_error);
> +
> +	if (local_error)
> +		g_error_free(local_error);

And avoid this if statement completely.

>   
>   	return G_SOURCE_REMOVE;
>   }
> @@ -1020,6 +1030,8 @@ static int handle_body(struct web_session *session,
>   				const guint8 *buf, gsize len)
>   {
>   	int err;
> +	g_autofree char *message = NULL;
> +	GError *local_error = NULL;

Ditto here

>   
>   	debug(session->web, "[body] length %zu", len);
>   
> @@ -1027,19 +1039,28 @@ static int handle_body(struct web_session *session,
>   		if (len > 0) {
>   			session->result.buffer = buf;
>   			session->result.length = len;
> -			call_result_func(session,
> -				GWEB_HTTP_STATUS_CODE_UNKNOWN);
> +			call_result_func(session, NULL);
>   		}
>   		return 0;
>   	}
>   
>   	err = decode_chunked(session, buf, len);
>   	if (err < 0) {
> -		debug(session->web, "Error in chunk decode %d", err);
> +		message = g_strdup_printf("Error in chunk decode %d", err);
> +
> +		debug(session->web, message);
>   
>   		session->result.buffer = NULL;
>   		session->result.length = 0;
> -		call_result_func(session, GWEB_HTTP_STATUS_CODE_BAD_REQUEST);
> +
> +		local_error = g_error_new_literal(G_WEB_ERROR,
> +			G_WEB_ERROR_CHUNK_DECODE,
> +			message);
> +
> +		call_result_func(session, local_error);
> +
> +		if (local_error)
> +			g_error_free(local_error);

Ditto

>   	}
>   
>   	return err;

<snip>

> @@ -1255,6 +1274,7 @@ static gboolean received_data(GIOChannel *channel, GIOCondition cond,
>   	struct web_session *session = user_data;
>   	gsize bytes_read;
>   	GIOStatus status;
> +	GError *local_error = NULL;

And here

>   
>   	/* We received some data or condition, cancel the connect timeout. */
>   
> @@ -1270,7 +1290,14 @@ static gboolean received_data(GIOChannel *channel, GIOCondition cond,
>   		session->result.buffer = NULL;
>   		session->result.length = 0;
>   
> -		call_result_func(session, GWEB_HTTP_STATUS_CODE_BAD_REQUEST);
> +		local_error = g_error_new_literal(G_IO_ERROR,
> +			g_io_error_from_errno(EIO),
> +			g_strerror(EIO));
> +
> +		call_result_func(session, local_error);
> +
> +		if (local_error)
> +			g_error_free(local_error);

Ditto

>   
>   		return FALSE;
>   	}
> @@ -2288,6 +2315,8 @@ static void handle_resolved_address(struct web_session *session)
>   	struct addrinfo hints;
>   	g_autofree char *port;
>   	int ret;
> +	g_autofree char *message = NULL;
> +	GError *local_error = NULL;
>   
>   	debug(session->web, "address %s", session->address);
>   
> @@ -2303,16 +2332,35 @@ static void handle_resolved_address(struct web_session *session)
>   	port = g_strdup_printf("%u", session->port);
>   	ret = getaddrinfo(session->address, port, &hints, &session->addr);
>   	if (ret != 0 || !session->addr) {
> -		call_result_func(session, GWEB_HTTP_STATUS_CODE_BAD_REQUEST);
> -		return;
> +		message = g_strdup_printf("could not resolve %s: %s",
> +			session->address,
> +			gai_strerror(ret));

Maybe consider dropping 'message' variable entirely and using g_set_error or 
g_error_new?

> +
> +		local_error = g_error_new_literal(G_WEB_ERROR,
> +			G_WEB_ERROR_HOST_NOT_FOUND,
> +			message);
> +
> +		call_result_func(session, local_error);
> +
> +		goto done;
>   	}
>   
>   	call_route_func(session);
>   
> -	if (create_transport(session) < 0) {
> -		call_result_func(session, GWEB_HTTP_STATUS_CODE_CONFLICT);
> -		return;
> +	ret = create_transport(session);
> +	if (ret < 0) {
> +		local_error = g_error_new_literal(G_IO_ERROR,
> +						g_io_error_from_errno(ret),
> +						g_strerror(ret));
> +
> +		call_result_func(session, local_error);
> +
> +		goto done;

nit: The above line is not needed

>   	}
> +
> + done:
> +	if (local_error)
> +		g_error_free(local_error);
>   }
>   
>   static gboolean already_resolved(gpointer data)
> @@ -2329,9 +2377,22 @@ static void resolv_result(GResolvResultStatus status,
>   					char **results, gpointer user_data)
>   {
>   	struct web_session *session = user_data;
> +	g_autofree char *message = NULL;
> +	GError *local_error = NULL;
>   
>   	if (!results || !results[0]) {
> -		call_result_func(session, GWEB_HTTP_STATUS_CODE_NOT_FOUND);
> +		message = g_strdup_printf("could not resolve %s",
> +			session->host);
> +
> +		local_error = g_error_new_literal(G_WEB_ERROR,
> +				G_WEB_ERROR_HOST_NOT_FOUND,
> +				message);
> +

Same here, consider dropping 'message' entirely and using g_set_error or g_error_new

> +		call_result_func(session, local_error);
> +
> +		if (local_error)
> +			g_error_free(local_error);
> +
>   		return;
>   	}
>   

Regards,
-Denis

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

[Denis Kenzior]

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

Re: [PATCH v3 09/18] gweb: Adopt optional, immutable GError instance for GWeb result errors.

[Grant Erickson]

On Mar 24, 2025, at 3:28 PM, Denis Kenzior <denkenz@gmail.com> wrote:
> On 3/20/25 6:39 PM, Grant Erickson wrote:
>> In the following instances, GWeb can fail before initiating or while
>> processing a web request:
>>   1. TCP connection timeout to the remote server peer.
>>   2. Decoding a chunked response.
>>   3. Non-successful events (that is, G_IO_NVAL, G_IO_ERR, or G_IO_HUP)
>>      from 'g_io_add_watch'.
>>   4. GResolve "host not found" DNS forward resolution failure.
>>   5. Transport creation failure from 'create_transport'.
>> Rather than trying to overload 'GWEB_HTTP_STATUS_CODE_' error
>> enumerations from actual web server responses for these non-response
>> or response-in-progress failures, the signature of 'GWebResultFunc' is
>> amended to take an optional pointer to an immutable GError instance
>> which describes one of the five errors above on a GWeb request
>> failure.
> 
> This looks so much better, a few nits:

Thanks.

> Could you use:
> 
> g_autoptr(GError) local_error = NULL;

Great feedback and optimization. I was familiar with g_autofree but not g_autoptr.

I’ve adopted this across the board, as well as g_error_new, where appropriate.

Best,

Grant

-- 
Principal
Nuovations

gerickson@nuovations.com
https://www.nuovations.com/

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

[Grant Erickson]

On Mar 24, 2025, at 3:45 PM, Denis Kenzior <denkenz@gmail.com> 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

gerickson@nuovations.com
https://www.nuovations.com/