[PATCH v4 0/9] Close Two GWeb Request "Bookend" Failure "Holes"

[PATCH v4 0/9] 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 (9):
  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}'.
  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'.

 gweb/gweb.c      | 267 +++++++++++++++++++++++++++++++++++++++++------
 gweb/gweb.h      |   2 +-
 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 +-
 8 files changed, 430 insertions(+), 95 deletions(-)

-- 
2.45.0

[PATCH v4 1/9] 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      | 99 +++++++++++++++++++++++++++++++++---------------
 gweb/gweb.h      |  2 +-
 src/6to4.c       |  3 +-
 src/wispr.c      |  5 ++-
 tools/web-test.c |  2 +-
 tools/wispr.c    |  2 +-
 6 files changed, 76 insertions(+), 37 deletions(-)

diff --git a/gweb/gweb.c b/gweb/gweb.c
index 0e18e9d5d978..a736c32ef0a5 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"
@@ -210,28 +212,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)
@@ -265,9 +265,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;
+	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;
 
@@ -276,7 +281,11 @@ 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);
 
 	return G_SOURCE_REMOVE;
 }
@@ -1017,8 +1026,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;
@@ -1033,8 +1041,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;
@@ -1052,6 +1059,8 @@ static int handle_body(struct web_session *session,
 				const guint8 *buf, gsize len)
 {
 	int err;
+	g_autofree char *message = NULL;
+	g_autoptr(GError) local_error = NULL;
 
 	debug(session->web, "[body] length %zu", len);
 
@@ -1059,19 +1068,25 @@ 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);
 	}
 
 	return err;
@@ -1157,14 +1172,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,
@@ -1287,6 +1300,7 @@ static gboolean received_data(GIOChannel *channel, GIOCondition cond,
 	struct web_session *session = user_data;
 	gsize bytes_read;
 	GIOStatus status;
+	g_autoptr(GError) local_error = NULL;
 
 	/* We received some data or condition, cancel the connect timeout. */
 
@@ -1302,7 +1316,11 @@ 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);
 
 		return FALSE;
 	}
@@ -2320,6 +2338,7 @@ static void handle_resolved_address(struct web_session *session)
 	struct addrinfo hints;
 	g_autofree char *port;
 	int ret;
+	g_autoptr(GError) local_error = NULL;
 
 	debug(session->web, "address %s", session->address);
 
@@ -2335,15 +2354,26 @@ 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);
+		local_error = g_error_new(G_WEB_ERROR,
+			G_WEB_ERROR_HOST_NOT_FOUND,
+			"could not resolve %s: %s",
+			session->address,
+			gai_strerror(ret));
+
+		call_result_func(session, local_error);
+
 		return;
 	}
 
 	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);
 	}
 }
 
@@ -2361,9 +2391,16 @@ static void resolv_result(GResolvResultStatus status,
 					char **results, gpointer user_data)
 {
 	struct web_session *session = user_data;
+	g_autoptr(GError) local_error = NULL;
 
 	if (!results || !results[0]) {
-		call_result_func(session, GWEB_HTTP_STATUS_CODE_NOT_FOUND);
+		local_error = g_error_new(G_WEB_ERROR,
+				G_WEB_ERROR_HOST_NOT_FOUND,
+				"could not resolve %s",
+				session->host);
+
+		call_result_func(session, 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 v4 2/9] 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 a736c32ef0a5..07b50d07a93a 100644
--- a/gweb/gweb.c
+++ b/gweb/gweb.c
@@ -217,6 +217,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
@@ -234,6 +236,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)
@@ -285,7 +343,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);
 
 	return G_SOURCE_REMOVE;
 }
@@ -1026,7 +1084,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;
@@ -1041,7 +1099,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;
@@ -1068,7 +1126,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;
 	}
@@ -1086,7 +1144,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);
 	}
 
 	return err;
@@ -1320,7 +1378,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);
 
 		return FALSE;
 	}
@@ -2360,7 +2418,7 @@ static void handle_resolved_address(struct web_session *session)
 			session->address,
 			gai_strerror(ret));
 
-		call_result_func(session, local_error);
+		call_result_func_failure(session, local_error);
 
 		return;
 	}
@@ -2373,7 +2431,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);
 	}
 }
 
@@ -2399,7 +2457,7 @@ static void resolv_result(GResolvResultStatus status,
 				"could not resolve %s",
 				session->host);
 
-		call_result_func(session, local_error);
+		call_result_func_failure(session, local_error);
 
 		return;
 	}
-- 
2.45.0

[PATCH v4 3/9] 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 | 64 ++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 58 insertions(+), 6 deletions(-)

diff --git a/gweb/gweb.c b/gweb/gweb.c
index 07b50d07a93a..10f1cf1df0b2 100644
--- a/gweb/gweb.c
+++ b/gweb/gweb.c
@@ -1228,14 +1228,62 @@ 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)
 {
+	g_autoptr(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);
 }
 
 static bool received_data_continue(struct web_session *session,
@@ -1356,9 +1404,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;
 	g_autoptr(GError) local_error = NULL;
+	g_autoptr(GError) error = NULL;
 
 	/* We received some data or condition, cancel the connect timeout. */
 
@@ -1387,16 +1437,17 @@ 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);
 
 		return FALSE;
 	}
@@ -2520,6 +2571,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 v4 4/9] 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 10f1cf1df0b2..c17475c5b8c1 100644
--- a/gweb/gweb.c
+++ b/gweb/gweb.c
@@ -1228,6 +1228,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)
@@ -1286,6 +1323,23 @@ static void received_data_finalize(struct web_session *session,
 	call_result_func_failure(session, passed_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 v4 5/9] 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 v4 6/9] 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 v4 7/9] 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 v4 8/9] 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 v4 9/9] 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 v4 5/9] wispr: Refactor 'wispr_portal_web_result' to leverage 'GError'.

[Denis Kenzior]

Hi Grant,

On 3/24/25 11:59 PM, Grant Erickson wrote:
> 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(-)
> 

<snip>

> @@ -1085,15 +1078,15 @@ static bool wispr_portal_web_result(const GError *error, GWebResult *result,
>   
>   		goto done;

This part seems fishy.  Previous version of this function had 
wispr_portal_context_unref() call after the 'done' label...

>   	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;

But now you have a simple return at the end of the function.  Which by the way 
is discouraged and should have been picked up by the compiler or static analysis.

> +}
> +
> +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);

We still use 80 character lines, so please break up accordingly.

> +
> +		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);
> +

Please don't introduce spurious whitespace changes and keep the patches minimal.

>   	return false;
>   }
>   

<snip>

> @@ -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));

 > 80 chars again

>   
>   	wispr_portal_context_unref(wp_context);
>   

Regards,
-Denis

Re: [PATCH v4 0/9] 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 Mon, 24 Mar 2025 21:59:18 -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:
  - [v4,1/9] gweb: Adopt optional, immutable GError instance for GWeb result errors.
    https://git.kernel.org/pub/scm/network/connman/connman.git/?id=8d0117f5ffed
  - [v4,2/9] gweb: Added and leveraged 'call_result_func_{failure,success}' wrappers.
    https://git.kernel.org/pub/scm/network/connman/connman.git/?id=32d7fcb1f1e4
  - [v4,3/9] gweb: Close web request session finalization 'hole'.
    https://git.kernel.org/pub/scm/network/connman/connman.git/?id=6975c4b8a3bb
  - [v4,4/9] gweb: Add documentation to 'received_data_{finalize,continue}'.
    https://git.kernel.org/pub/scm/network/connman/connman.git/?id=4c8e985e296b
  - [v4,5/9] wispr: Refactor 'wispr_portal_web_result' to leverage 'GError'.
    (no matching commit)
  - [v4,6/9] wispr: Document 'wispr_portal_web_result_failure'.
    (no matching commit)
  - [v4,7/9] wispr: Document 'wispr_portal_web_result_success'.
    (no matching commit)
  - [v4,8/9] wispr: Document 'wispr_portal_web_result'.
    (no matching commit)
  - [v4,9/9] 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 v4 5/9] wispr: Refactor 'wispr_portal_web_result' to leverage 'GError'.

[Grant Erickson]

On Mar 25, 2025, at 7:28 AM, Denis Kenzior <denkenz@gmail.com> wrote:
> On 3/24/25 11:59 PM, Grant Erickson wrote:
>> 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.
>> 
>> <snip>
>> 
>> ---
>>  src/wispr.c | 105 +++++++++++++++++++++++++++++++++++-----------------
>>  1 file changed, 71 insertions(+), 34 deletions(-)
> 
> <snip>
> 
>> @@ -1085,15 +1078,15 @@ static bool wispr_portal_web_result(const GError *error, GWebResult *result,
>>     goto done;
> 
> This part seems fishy.  Previous version of this function had wispr_portal_context_unref() call after the 'done' label...

As far as I can see, both success and failure termination paths with or without ‘goto done’ calls, lead to the wispr_portal_context_unref() block:

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;

	/*
	 * Release a reference to the WISPr/portal context to balance the
	 * earlier reference retained to account for ‘g_web_request_get'
	 * maintaining a weak reference to it.
	 */
	wispr_portal_context_unref(wp_context);

	return false;
}

>>   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;
> 
> But now you have a simple return at the end of the function.  Which by the way is discouraged and should have been picked up by the compiler or static analysis.

I tried to keep the original block of logic as-is. 

Personally, I’m a fan of single points of exit—so much easier to audit. But, happy to keep whatever style is preferred. It sounds like the preference is to replace the prior ‘goto done’ with ‘return’, correct?

>> + g_web_parser_end_data(wp_context->wispr_parser);
>> +
>> + DBG("wp_context->wispr_msg.message_type %d", wp_context->wispr_msg.message_type);
> 
> We still use 80 character lines, so please break up accordingly.

Surprisingly, checkpatch.pl did not squawk about this. Fixed, regardless.

>> @@ -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));
> 
> > 80 chars again

Surprisingly, checkpatch.pl did not squawk about this either. Fixed, regardless.

Best,

Grant

-- 
Principal
Nuovations

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

Re: [PATCH v4 5/9] wispr: Refactor 'wispr_portal_web_result' to leverage 'GError'.

[Denis Kenzior]

Hi Grant,

 >>
>> But now you have a simple return at the end of the function.  Which by the way is discouraged and should have been picked up by the compiler or static analysis.
> 
> I tried to keep the original block of logic as-is.
> 
> Personally, I’m a fan of single points of exit—so much easier to audit. But, happy to keep 

Kernel often prefers the 'exit-early' approach, but this is situational.

whatever style is preferred. It sounds like the preference is to replace the 
prior ‘goto done’ with ‘return’, correct?

Rule of thumb is not to have return at the end of a void function.  Static 
analysis will for sure flag that and I'll kick off a coverity run once I get the 
rest of your patches merged.

The question really is: Is the now-empty-done label hiding a bug?

> 
>>> + g_web_parser_end_data(wp_context->wispr_parser);
>>> +
>>> + DBG("wp_context->wispr_msg.message_type %d", wp_context->wispr_msg.message_type);
>>
>> We still use 80 character lines, so please break up accordingly.
> 
> Surprisingly, checkpatch.pl did not squawk about this. Fixed, regardless.
> 

The default is now 100 for the kernel.  We're sticking to 80 for now.  Maybe:

   --max-line-length=n        set the maximum line length, (default 100)
                              if exceeded, warn on patches
                              requires --strict for use with --file

works?

Regards,
-Denis