From: Klemens Nanni <kn@openbsd.org>
Subject: Re: iperf3: unbreak rcctl with IPv6 link-local bind/source addresses
To: ports <ports@openbsd.org>
Date: Sun, 16 Nov 2025 13:35:40 +0000

08.11.2025 06:18, Klemens Nanni пишет:
> 30.10.2025 22:42, Jeremie Courreges-Anglas пишет:
>> On Thu, Oct 30, 2025 at 05:49:15PM +0000, Klemens Nanni wrote:
>>> If the argument contains a '%', the code will modify argv[] in place,
>>> which causes rc.subr(8)'s pexp aka. pgrep(1) to mismatch and thus
>>> rcctl(8) to report failure despite the service running fine:
>>>
>>> 	$ rcctl get iperf3 flags
>>> 	-6 --bind fe80::1%vport0
>>> 	$ rcctl check iperf3
>>> 	iperf3(failed)
>>> 	$ pgrep -fl iperf3
>>> 	33091 /usr/local/bin/iperf3 -s -D -6 --bind fe80::1
>>>
>>>
>>> Funnily, the code knows that:
>>>
>>> 	 * Modifies the string pointed to by spec in-place due to the use of
>>> 		  * strtok(3). The caller should strdup(3) or otherwise copy the string
>>> 	 * if an unmodified copy is needed.
>>> 	 */
>>> 	int
>>> 	iperf_parse_hostname(struct iperf_test *test, char *spec, char **p, char **p1) {
>>>
>>>
>>> Simply pass a copy to that function to keep scope identifies in argv[]:
>>>
>>> 	$ rcctl check iperf3
>>> 	iperf3(ok)
>>> 	$ pgrep -fl iperf3
>>> 	98863 /usr/local/bin/iperf3 -s -D -6 --bind fe80::1%vport0
>>>
>>> --client works the same as --bind, so fix it as well.
>>>
>>> Feedback? OK?
>>
>> Nit that isn't just a nit: I guess this is only used at startup, but
>> I'd suggest to check the return value of strdup(3), just for
>> correctness.
> 
> iperf3 strdup()s stuff all over the place without checking for NULL.
> In most places it seems to store away the return value and use it later
> through some getter function.
> 
> Here's an extra check (provoked with `arg = NULL, errno = ENOMEM;'),
> but it does not match the rest of the code;  the -1 is needed to hit
> iperf_strerror()'s default case:
> 
> 	$ iperf3 -6 -s --bind fe80::1%lo0                                                 <	
> 	iperf3: parameter error - int_errno=-1: Cannot allocate memory
> 	
> 	Usage: iperf3 [-s|-c host] [options]
> 	Try `iperf3 --help' for more information.
> 
> 
> I've left the upstream PR as-is, no response so far.
> 
> Feedback? OK for either of the two versions?
Ping, same diff again.

Index: Makefile
===================================================================
RCS file: /cvs/ports/net/iperf3/Makefile,v
diff -u -p -r1.21 Makefile
--- Makefile	26 Dec 2024 18:12:38 -0000	1.21
+++ Makefile	30 Oct 2025 17:08:08 -0000
@@ -3,6 +3,7 @@ COMMENT=	tool to measure maximum achieva
 V=		3.18
 DISTNAME=	iperf-${V}
 PKGNAME=	iperf3-${V}
+REVISION=	0
 
 SHARED_LIBS +=	iperf                5.0      # 0.0
 
Index: patches/patch-src_iperf_api_c
===================================================================
RCS file: /cvs/ports/net/iperf3/patches/patch-src_iperf_api_c,v
diff -u -p -r1.12 patch-src_iperf_api_c
--- patches/patch-src_iperf_api_c	26 Dec 2024 18:12:38 -0000	1.12
+++ patches/patch-src_iperf_api_c	7 Nov 2025 23:17:32 -0000
@@ -1,9 +1,74 @@
-Default to IPv4.
+- Pass a copy of argv[] to prevent changes by strtok(3),
+  otherwise rc.subr(8) pexp might not match and cause rcctl failure.
+  Pending https://github.com/esnet/iperf/pull/1960
+
+- Default to IPv4.
 
 Index: src/iperf_api.c
 --- src/iperf_api.c.orig
 +++ src/iperf_api.c
-@@ -2998,7 +2998,7 @@ iperf_defaults(struct iperf_test *testp)
+@@ -1243,7 +1243,7 @@ iperf_parse_arguments(struct iperf_test *test, int arg
+                 }
+ 		iperf_set_test_role(test, 's');
+                 break;
+-            case 'c':
++            case 'c': {
+                 if (test->role == 's') {
+                     i_errno = IESERVCLIENT;
+                     return -1;
+@@ -1251,7 +1251,12 @@ iperf_parse_arguments(struct iperf_test *test, int arg
+ 		iperf_set_test_role(test, 'c');
+ 		iperf_set_test_server_hostname(test, optarg);
+ 
+-                if (iperf_parse_hostname(test, optarg, &p, &p1)) {
++                char *arg = strdup(optarg);
++                if (arg == NULL) {
++                    i_errno = -1;
++                    return -1;
++                }
++                if (iperf_parse_hostname(test, arg, &p, &p1)) {
+ #if defined(HAVE_SO_BINDTODEVICE)
+                     /* Get rid of the hostname we saved earlier. */
+                     free(iperf_get_test_server_hostname(test));
+@@ -1262,7 +1267,9 @@ iperf_parse_arguments(struct iperf_test *test, int arg
+                     return -1;
+ #endif /* HAVE_SO_BINDTODEVICE */
+                 }
++                free(arg);
+                 break;
++            }
+             case 'u':
+                 set_protocol(test, Pudp);
+ 		client_flag = 1;
+@@ -1374,10 +1381,15 @@ iperf_parse_arguments(struct iperf_test *test, int arg
+ 		client_flag = 1;
+                 break;
+ 
+-            case 'B':
++            case 'B': {
+                 iperf_set_test_bind_address(test, optarg);
+ 
+-                if (iperf_parse_hostname(test, optarg, &p, &p1)) {
++                char *arg = strdup(optarg);
++                if (arg == NULL) {
++                    i_errno = -1;
++                    return -1;
++                }
++                if (iperf_parse_hostname(test, arg, &p, &p1)) {
+ #if defined(HAVE_SO_BINDTODEVICE)
+                     /* Get rid of the hostname we saved earlier. */
+                     free(iperf_get_test_bind_address(test));
+@@ -1388,7 +1400,9 @@ iperf_parse_arguments(struct iperf_test *test, int arg
+                     return -1;
+ #endif /* HAVE_SO_BINDTODEVICE */
+                 }
++                free(arg);
+                 break;
++            }
+ #if defined (HAVE_SO_BINDTODEVICE)
+             case OPT_BIND_DEV:
+                 iperf_set_test_bind_dev(test, optarg);
+@@ -2998,7 +3012,7 @@ iperf_defaults(struct iperf_test *testp)
      testp->stats_interval = testp->reporter_interval = 1;
      testp->num_streams = 1;