[PATCH 0/7] Improve PAC URL Mutation and PAC URL Logging

[PATCH 0/7] Improve PAC URL Mutation and PAC URL Logging

[Grant Erickson]

There appear to be a population of premises ISP equipment (some
possibly correlated with Spectrum Communications in Southern
California, United States) in which the ISP equipment sporadically and
incorrectly emits DHCP Option 252 ("Web Proxy Auto-Discovery (WPAD)")
with an invalid proxy auto-configuration (PAC) URL (such as
'http://wpad.lan/wpad.dat'), even when neither the customer nor the
ISP has such an option actively configured on the equipment.

This URL, per the WPAD specification, leads the Connection Manager to
believe that it must use a proxy to reach HTTP- or HTTPS-based
resources, including those specified for WISPr online reachability
checks. Because neither the PAC file ('wpad.dat' in the example above)
nor the host ('wpad.lan' in the example above) associated with the PAC
URL exist, those online reachability checks unconditionally fail
despite such resources being directly reachable, without an
intervening proxy.

Because:

  * PAC configuration (or lack thereof) has not been historically
    logged
  * POSIX error codes are themselves limited (for example, there are
    no -ENOPROXY, -EBADPROXY, or -EPROXYLOOKUP codes)
  * POSIX error code usage for WISPr failures is similarly limited
    (for example, -EINVAL)

insight into the actual nature of an online reachability check failure
is, by extension, limited, particularly when proxy failures are
involved. Consequently, log messages generated from these changes
provide crucial insight into triaging and resolving such failures.

Finally, there have been three (3) call sites in the network service
code where the network service proxy auto-configuration (PAC) URL was
set. However, it was never (but now, with these changes, is) set in
'__connman_service_set_proxy_autoconfig', where it is invoked by the
DHCP client code when DHCP option 252 (Web Proxy Auto-Discovery
(WPAD)) is encountered.

Grant Erickson (7):
  service: Leverage 'service_set_pac' function.
  service: Add 'service_set_proxy_method' function.
  service: Refactor 'connman_service_set_proxy_method'.
  service: Refactor '__connman_service_set_proxy_autoconfig'.
  service: Document 'service_set_proxy_method'.
  service: Document 'service_set_proxy_method_auto_handler'.
  service: Document '__connman_service_set_proxy_autoconfig'.

 src/service.c | 201 +++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 157 insertions(+), 44 deletions(-)

-- 
2.45.0

[PATCH 1/7] service: Leverage 'service_set_pac' function.

[Grant Erickson]

Leverage 'service_set_pac' at the three call sites the proxy
auto-configuration (PAC) URL is currently set or cleared for a network
service.

Frequently, a spurious, misconfigured, or otherwise incorrect PAC URL
is the source of connectivity problems, particularly for online
reachability checks. Consequently, being able to log all such
mutations can be helpful in triaging such problems.
---
 src/service.c | 22 +++++++---------------
 1 file changed, 7 insertions(+), 15 deletions(-)

diff --git a/src/service.c b/src/service.c
index b8943c03f459..b261f4ce0681 100644
--- a/src/service.c
+++ b/src/service.c
@@ -808,10 +808,10 @@ static int service_load(struct connman_service *service)
 
 	str = g_key_file_get_string(keyfile,
 				service->identifier, "Proxy.URL", NULL);
-	if (str) {
-		g_free(service->pac);
-		service->pac = str;
-	}
+	if (str)
+		service_set_pac(service, str, false);
+
+	g_free(str);
 
 	service->mdns_config = g_key_file_get_boolean(keyfile,
 				service->identifier, "mDNS", NULL);
@@ -5800,12 +5800,9 @@ void __connman_service_timeserver_changed(struct connman_service *service,
 void __connman_service_set_pac(struct connman_service *service,
 					const char *pac)
 {
-	if (service->hidden)
-		return;
-	g_free(service->pac);
-	service->pac = g_strdup(pac);
+	const bool dochanged = true;
 
-	proxy_changed(service);
+	service_set_pac(service, pac, dochanged);
 }
 
 void __connman_service_set_agent_identity(struct connman_service *service,
@@ -6092,12 +6089,7 @@ static int update_proxy_configuration(struct connman_service *service,
 
 		break;
 	case CONNMAN_SERVICE_PROXY_METHOD_AUTO:
-		g_free(service->pac);
-
-		if (url && strlen(url) > 0)
-			service->pac = g_strstrip(g_strdup(url));
-		else
-			service->pac = NULL;
+		service_set_pac(service, url, false);
 
 		/* if we are connected:
 		   - if service->pac == NULL
-- 
2.45.0

[PATCH 2/7] service: Add 'service_set_proxy_method' function.

[Grant Erickson]

Allows for logging, setting, and dispatching property changed
notifications for the network service proxy method and associated URL,
if any, from a single place.
---
 src/service.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/src/service.c b/src/service.c
index b261f4ce0681..db6ec26240fd 100644
--- a/src/service.c
+++ b/src/service.c
@@ -5569,6 +5569,37 @@ const char * const *connman_service_get_timeservers(const struct connman_service
 	return (const char * const *)service->timeservers;
 }
 
+static void service_set_proxy_method(struct connman_service *service,
+			enum connman_service_proxy_method method,
+			bool donotifier,
+			bool (*handler)(struct connman_service *service,
+				enum connman_service_proxy_method method,
+				const void *context),
+			const void *context)
+{
+	DBG("service %p (%s) method %d (%s) donotifier %u "
+		"handler %p, context %p",
+		service, connman_service_get_identifier(service),
+		method, proxymethod2string(method),
+		donotifier,
+		handler,
+		context);
+
+	if (!service || service->hidden)
+		return;
+
+	service->proxy = method;
+
+	if (handler != NULL)
+		if (handler(service, method, context) != true)
+			return;
+
+	proxy_changed(service);
+
+	if (donotifier)
+		__connman_notifier_proxy_changed(service);
+}
+
 /**
  *  @brief
  *    Set the web proxy method of the specified service.
-- 
2.45.0

[PATCH 3/7] service: Refactor 'connman_service_set_proxy_method'.

[Grant Erickson]

There are currently two places where the network service proxy method
is set:

  1. connman_service_set_proxy_method
  2. __connman_service_set_proxy_autoconfig

Prior to this change, each did a similar, but bespoke set of
operations to set the network service proxy method.

Leverage 'service_set_proxy_method', which embodies the
previously-similar, bespoke operations, for
'connman_service_set_proxy_method', the first of the above two call
sites.
---
 src/service.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/src/service.c b/src/service.c
index db6ec26240fd..6fff20d4de96 100644
--- a/src/service.c
+++ b/src/service.c
@@ -5618,19 +5618,13 @@ static void service_set_proxy_method(struct connman_service *service,
 void connman_service_set_proxy_method(struct connman_service *service,
 					enum connman_service_proxy_method method)
 {
-	DBG("service %p (%s) method %d (%s)",
-		service, connman_service_get_identifier(service),
-		method, proxymethod2string(method));
-
-	if (!service || service->hidden)
-		return;
-
-	service->proxy = method;
-
-	proxy_changed(service);
+	const bool donotifier = method != CONNMAN_SERVICE_PROXY_METHOD_AUTO;
 
-	if (method != CONNMAN_SERVICE_PROXY_METHOD_AUTO)
-		__connman_notifier_proxy_changed(service);
+	service_set_proxy_method(service,
+		method,
+		donotifier,
+		NULL,
+		NULL);
 }
 
 enum connman_service_proxy_method connman_service_get_proxy_method(
-- 
2.45.0

[PATCH 4/7] service: Refactor '__connman_service_set_proxy_autoconfig'.

[Grant Erickson]

There are currently two places where the network service proxy method
is set:

  1. connman_service_set_proxy_method
  2. __connman_service_set_proxy_autoconfig

Prior to this change, each did a similar, but bespoke set of
operations to set the network service proxy method.

Leverage 'service_set_proxy_method', which embodies the
previously-similar, bespoke operations, for
'__connman_service_set_proxy_autoconfig', the second of the above two
call sites.
---
 src/service.c | 48 +++++++++++++++++++++++++++++++-----------------
 1 file changed, 31 insertions(+), 17 deletions(-)

diff --git a/src/service.c b/src/service.c
index 6fff20d4de96..cb168b03c790 100644
--- a/src/service.c
+++ b/src/service.c
@@ -5662,32 +5662,46 @@ const char *connman_service_get_proxy_url(const struct connman_service *service)
 	return service->pac;
 }
 
-void __connman_service_set_proxy_autoconfig(struct connman_service *service,
-							const char *url)
+static bool service_set_proxy_method_auto_handler(
+				struct connman_service *service,
+				enum connman_service_proxy_method method,
+				const void *context)
 {
-	const bool dochanged = true;
-
-	if (!service || service->hidden)
-		return;
-
-	service_set_pac(service, url, !dochanged);
-
-	service->proxy = CONNMAN_SERVICE_PROXY_METHOD_AUTO;
+	const char * const url = context;
 
 	if (service->ipconfig_ipv4) {
 		if (__connman_ipconfig_set_proxy_autoconfig(
-			    service->ipconfig_ipv4, url) < 0)
-			return;
+				service->ipconfig_ipv4, url) < 0)
+			return false;
 	} else if (service->ipconfig_ipv6) {
 		if (__connman_ipconfig_set_proxy_autoconfig(
-			    service->ipconfig_ipv6, url) < 0)
-			return;
+				service->ipconfig_ipv6, url) < 0)
+			return false;
 	} else
-		return;
+		return false;
 
-	proxy_changed(service);
+	return true;
+}
+
+void __connman_service_set_proxy_autoconfig(struct connman_service *service,
+							const char *url)
+{
+	const bool dochanged = true;
+	const bool donotifier = true;
 
-	__connman_notifier_proxy_changed(service);
+	DBG("service %p (%s) url %p (%s)",
+		service,
+		connman_service_get_identifier(service),
+		url,
+		url ? url : "<null>");
+
+	service_set_pac(service, url, !dochanged);
+
+	service_set_proxy_method(service,
+		CONNMAN_SERVICE_PROXY_METHOD_AUTO,
+		donotifier,
+		service_set_proxy_method_auto_handler,
+		url);
 }
 
 const char *connman_service_get_proxy_autoconfig(struct connman_service *service)
-- 
2.45.0

[PATCH 5/7] service: Document 'service_set_proxy_method'.

[Grant Erickson]

Add documentation to the 'service_set_proxy_method' function.
---
 src/service.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/src/service.c b/src/service.c
index cb168b03c790..17193706d9d4 100644
--- a/src/service.c
+++ b/src/service.c
@@ -5569,6 +5569,41 @@ const char * const *connman_service_get_timeservers(const struct connman_service
 	return (const char * const *)service->timeservers;
 }
 
+/**
+ *  @brief
+ *    Set the proxy method for the specified service.
+ *
+ *  This attempts to set the proxy method for the specified
+ *  service. If the service is null or if the service is hidden, no
+ *  action is taken. If specified, a handler will be invoked, with the
+ *  specified context, after setting the proxy method.
+ *
+ *  If the handler is specified and it returns true, following, a
+ *  D-Bus change notification should be sent for the service "Proxy"
+ *  property and, if requested by @a donotifier, the proxy changed
+ *  notifier chain will be run.
+ *
+ *  @param[in,out]  service     A pointer to the mutable network
+ *                              service for which to set the proxy
+ *                              method.
+ *  @param[in]      method      The network service proxy method to set
+ *                              on @a service.
+ *  @param[in]      donotifier  A Boolean indicating whether the proxy
+ *                              changed notifier chain should be run
+ *                              after @a method is set on @a service.
+ *  @param[in]      handler     An optional pointer to a handler that,
+ *                              if specified, will be invoked after @a
+ *                              method is set on @a service.
+ *  @param[in]      context     An optional pointer to immutable context
+ *                              that will be passed to @a handler,
+ *                              along with @a service and @a method.
+ *
+ *  @sa __connman_notifier_proxy_changed
+ *  @sa proxy_changed
+ *
+ *  @private
+ *
+ */
 static void service_set_proxy_method(struct connman_service *service,
 			enum connman_service_proxy_method method,
 			bool donotifier,
-- 
2.45.0

[PATCH 6/7] service: Document 'service_set_proxy_method_auto_handler'.

[Grant Erickson]

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

diff --git a/src/service.c b/src/service.c
index 17193706d9d4..4b31db3c6358 100644
--- a/src/service.c
+++ b/src/service.c
@@ -5697,6 +5697,36 @@ const char *connman_service_get_proxy_url(const struct connman_service *service)
 	return service->pac;
 }
 
+/**
+ *  @brief
+ *    A post-mutation handler for the service proxy method when the
+ *    method is #CONNMAN_SERVICE_PROXY_METHOD_AUTO.
+ *
+ *  @param[in,out]  service     A pointer to the mutable network
+ *                              service for which the proxy method was
+ *                              set.
+ *  @param[in]      method      The network service proxy method @a
+ *                              service was set to.
+ *  @param[in]      context     An pointer to immutable context
+ *                              that was passed, along with @a
+ *                              service, @a method, and this handler,
+ *                              to #service_set_proxy_method. For this
+ *                              handler, @a context is an optional
+ *                              null-terminated C string containing
+ *                              the service proxy auto-configuration
+ *                              (PAC) URL.
+ *
+ *  @returns
+ *    True if #service_set_proxy_method should continue with state
+ *    change notifications and processing proxy notifier changes after
+ *    the handler returns; otherwise, false.
+ *
+ *  @sa __connman_ipconfig_set_proxy_autoconfig
+ *  @sa service_set_proxy_method
+ *
+ *  @private
+ *
+ */
 static bool service_set_proxy_method_auto_handler(
 				struct connman_service *service,
 				enum connman_service_proxy_method method,
-- 
2.45.0

[PATCH 7/7] service: Document '__connman_service_set_proxy_autoconfig'.

[Grant Erickson]

Add documentation to the '__connman_service_set_proxy_autoconfig' function.
---
 src/service.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/src/service.c b/src/service.c
index 4b31db3c6358..ed0dea1dc1e5 100644
--- a/src/service.c
+++ b/src/service.c
@@ -5748,6 +5748,23 @@ static bool service_set_proxy_method_auto_handler(
 	return true;
 }
 
+/**
+ *  @brief
+ *    Set the network service proxy method to # and proxy
+ *    auto-configuation (PAC) URL.
+ *
+ *  @param[in,out]  service  A pointer to the mutable network
+ *                           service for which to set the proxy
+ *                           method.
+ *  @param[in]  url          An optional pointer to the immutable
+ *                           null-terminated C string containing the
+ *                           proxy auto-configuration (PAC) URL.
+ *
+ *  @sa service_set_pac
+ *  @sa service_set_proxy_method
+ *  @sa service_set_proxy_method_auto_handler
+ *
+ */
 void __connman_service_set_proxy_autoconfig(struct connman_service *service,
 							const char *url)
 {
-- 
2.45.0

Re: [PATCH 0/7] Improve PAC URL Mutation and PAC URL Logging

[patchwork-bot+connman]

Hello:

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

On Tue,  4 Mar 2025 13:02:58 -0800 you wrote:
> There appear to be a population of premises ISP equipment (some
> possibly correlated with Spectrum Communications in Southern
> California, United States) in which the ISP equipment sporadically and
> incorrectly emits DHCP Option 252 ("Web Proxy Auto-Discovery (WPAD)")
> with an invalid proxy auto-configuration (PAC) URL (such as
> 'http://wpad.lan/wpad.dat'), even when neither the customer nor the
> ISP has such an option actively configured on the equipment.
> 
> [...]

Here is the summary with links:
  - [1/7] service: Leverage 'service_set_pac' function.
    https://git.kernel.org/pub/scm/network/connman/connman.git/?id=eb65e1e3fb88
  - [2/7] service: Add 'service_set_proxy_method' function.
    https://git.kernel.org/pub/scm/network/connman/connman.git/?id=d8fcf6cc0c54
  - [3/7] service: Refactor 'connman_service_set_proxy_method'.
    https://git.kernel.org/pub/scm/network/connman/connman.git/?id=bf532b3cceb9
  - [4/7] service: Refactor '__connman_service_set_proxy_autoconfig'.
    https://git.kernel.org/pub/scm/network/connman/connman.git/?id=5488cd6dcaa8
  - [5/7] service: Document 'service_set_proxy_method'.
    https://git.kernel.org/pub/scm/network/connman/connman.git/?id=25e6914c6e46
  - [6/7] service: Document 'service_set_proxy_method_auto_handler'.
    https://git.kernel.org/pub/scm/network/connman/connman.git/?id=1123f398de9f
  - [7/7] service: Document '__connman_service_set_proxy_autoconfig'.
    https://git.kernel.org/pub/scm/network/connman/connman.git/?id=288b28bd2c57

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