Do not modify --bind and --client arguments inplace#1960
Do not modify --bind and --client arguments inplace#1960bmah888 merged 1 commit intoesnet:masterfrom
Conversation
|
Thanks for the pull request! I see and understand the problem you're trying to fix, iperf3 shouldn't go mucking around with argv[] like that. We'll take a look. |
For archives, this has since been applied to OpenBSD's |
bmah888
left a comment
There was a problem hiding this comment.
Thanks again for the PR! I've verified that it seems to fix the problem you originally reported.
The only feedback I have deals with freeing the duplicated string under certain error handling paths (I put two comments on the PR indicate the approximate places). What do you think?
Do as iperf_parse_hostname()'s comment says already: pass a copy of getopt(3)'s `optarg` aka. to avoid strtok(3) scribbling over `argv[]`. Otherwise arguments like "fe80::1%vport0" appear as "fe80::1" in the process list and cause exact matching of process name and arguments (against what was used) to fail. OpenBSD's net/iperf3 package ships a rc.subr(8) script and the service framework uses pgrep(1) to check for running processes, where this bug causes a mismatch due to the scope identifier being stripped: ``` $ 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 -B fe80::1 ``` Pass a copy to avoid modification, thus fixing rcctl(8) reporting: ``` $ rcctl check iperf3 iperf3(ok) $ pgrep -fl iperf3 98863 /usr/local/bin/iperf3 -s -D -6 -B fe80::1%vport0 ```
bmah888
left a comment
There was a problem hiding this comment.
Thanks for the revision! Approved, and will merge this shortly.
Do as iperf_parse_hostname()'s comment says already: pass a copy of
getopt(3)'s
optargaka. to avoid strtok(3) scribbling overargv[].Otherwise arguments like "fe80::1%vport0" appear as "fe80::1" in the
process list and cause exact matching of process name and arguments
(against what was used) to fail.
OpenBSD's net/iperf3 package ships a rc.subr(8) script and the service
framework uses pgrep(1) to check for running processes, where this
bug causes a mismatch due to the scope identifier being stripped:
Pass a copy to avoid modification, thus fixing rcctl(8) reporting: