Bug 263358 - hostapd rc script defines function hostapd_poststart() before $ifn
Summary: hostapd rc script defines function hostapd_poststart() before $ifn
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: misc (show other bugs)
Version: 13.1-RELEASE
Hardware: Any Any
: --- Affects Only Me
Assignee: Cy Schubert
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-04-17 04:08 UTC by Joshua Kinard
Modified: 2022-04-25 13:56 UTC (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Joshua Kinard 2022-04-17 04:08:49 UTC
In commit 0da2c91e6452, the hostapd rc script was modified to down and then up the wireless interface by way of a new hostapd_poststart function:

hostapd_poststart() {
	ifconfig ${ifn} down
	sleep 2
	ifconfig ${ifn} up
}

But this function, which references the variable $ifn, is defined before $ifn is defined.  Thus, when hostapd is started or restarted, the poststart function simply executes 'ifconfig down', sleeps, then 'ifconfig up'.  This results in errors from the ifconfig calls that 'down' and 'up' are not valid interfaces, and the wireless interface is not properly restarted.

I don't follow the logic because I don't know what is supposed to be passed in $2 to $ifn.  Would it be better to define a new var for rc.conf called something like $hostapd_iface that the user can use to identify which interface is the wireless one that hostapd should control?

Note, this first appeared in 13.1-RC2, so a fix should probably be pushed out so it makes it into the final RC before release.
Comment 1 Andriy Gapon freebsd_committer freebsd_triage 2022-04-18 06:16:43 UTC
(In reply to Joshua Kinard from comment #0)
First, that's not how shell functions work.
Second, the hostapd script requires an additional argument, the interface name.

So, it's your usage error.  There is no bug.
Comment 2 Joshua Kinard 2022-04-18 06:45:19 UTC
(In reply to Andriy Gapon from comment #1)

Prior to this //recent// change, I did not have to specify an interface to the rc script.  'service hostapd restart' worked as expected, as it has been for the past two years, because the interface was specified in hostapd's config file.  The interface being required in this instance is so it can be passed to ifconfig, NOT to hostapd.  This isn't a usage error, this is an improper overloading of the rc script's functionality, if anything.

As far as bourne shell functions go, unless a variable is explicitly passed to it, it will pull it from global scope.  But because the function is defined before $ifn, it gets a blank value.  This means when substitution occurs, you get "ifconfig  down" followed by "ifconfig  up" (note the two spaces).

The commit that added this change references a similar commit to wpa_supplicant, 5fcdc19a8111, and if you look at that rc script, the equivalent poststart function is defined AFTER $ifn is defined, so it will pickup an interface passed to the rc script.  wpa_supplicant's rc script also includes an explicit check for $ifn being defined, and if not, returns 1 for an error.  The hostapd rc script does not have such a check, thus it is logical to assume that the interface argument is optional, NOT required.  If this assumption is wrong, then that is itself a second bug, because the author of the change forgot to enforce the passing of an interface in a similar manner to what wpa_supplicant's rc script is doing.

This is a bug :)
Comment 3 Joshua Kinard 2022-04-18 07:04:08 UTC
I suspect that the need for $ifn was for systems with more than one wireless radio, so the user would have to supply an interface so the the correct radio was restarted by ifconfig.  In my case, I only have a single radio, so I don't need to always pass an interface.  The preexisting logic allows for this and it's been that way for some time.  So I am pretty confident that it is not usage error.  If it is, then that is still a bug because the rc script should explicitly check and enforce that usage.

When the poststart function was added, I guess the committer didn't realize that hostapd made passing $ifn entirely optional, thus the poststart function would break if $ifn wasn't defined (after the function gets moved to be below $ifn's definition).

I think the rc script could be made more robust by including some rc variables in rc.conf to specify the defined wireless interfaces, then the rc script can restart each defined interface appropriately.  Most systems won't (as far as I know) exceed two radios (2.4GHz & 5GHz), so this won't add much cruft to rc.conf.
Comment 4 Mark Johnston freebsd_committer freebsd_triage 2022-04-22 15:36:37 UTC
It looks like rc.d/hostapd indeed supports two execution models.  You can set
hostapd_enable=YES or ifconfig_wlan0="HOSTAP".  Both are documented.  In the second usage, netconfig passes an interface name, in the first one it doesn't.  So it does seem like a regression, and hostapd_poststart should do nothing if [ -z "$ifn" ] is true.
Comment 5 commit-hook freebsd_committer freebsd_triage 2022-04-22 16:22:05 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=1452bfcd9bbcb2f5bbb89fa38d01ce51dd9b6d44

commit 1452bfcd9bbcb2f5bbb89fa38d01ce51dd9b6d44
Author:     Cy Schubert <cy@FreeBSD.org>
AuthorDate: 2022-04-22 16:03:08 +0000
Commit:     Cy Schubert <cy@FreeBSD.org>
CommitDate: 2022-04-22 16:15:49 +0000

    libexec/rc.d/hostapd: Down/up interface when interface is specified

    When no interface is specified results in a syntax error in the rc
    script. Only execute poststart when an interface has been specified.

    PR:             263358
    Submitted by:   markj
    Reported by:    Joshua Kinard <freebsd@kumba.dev>
    Fixes:          0da2c91e64528d896f69d36670e25b4b4a140579
    MFC after:      3 days

 libexec/rc/rc.d/hostapd | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
Comment 6 commit-hook freebsd_committer freebsd_triage 2022-04-25 13:49:41 UTC
A commit in branch stable/13 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=c93cddd19baf396806a982f7659dcc77cdd5867e

commit c93cddd19baf396806a982f7659dcc77cdd5867e
Author:     Cy Schubert <cy@FreeBSD.org>
AuthorDate: 2022-04-22 16:03:08 +0000
Commit:     Cy Schubert <cy@FreeBSD.org>
CommitDate: 2022-04-25 13:49:04 +0000

    libexec/rc.d/hostapd: Down/up interface when interface is specified

    When no interface is specified results in a syntax error in the rc
    script. Only execute poststart when an interface has been specified.

    PR:             263358
    Submitted by:   markj
    Reported by:    Joshua Kinard <freebsd@kumba.dev>
    Fixes:          0da2c91e64528d896f69d36670e25b4b4a140579

    (cherry picked from commit 1452bfcd9bbcb2f5bbb89fa38d01ce51dd9b6d44)

 libexec/rc/rc.d/hostapd | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
Comment 7 commit-hook freebsd_committer freebsd_triage 2022-04-25 13:51:42 UTC
A commit in branch stable/12 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=d818ef7df7330e8b3aac43f5160e102d6a702862

commit d818ef7df7330e8b3aac43f5160e102d6a702862
Author:     Cy Schubert <cy@FreeBSD.org>
AuthorDate: 2022-04-22 16:03:08 +0000
Commit:     Cy Schubert <cy@FreeBSD.org>
CommitDate: 2022-04-25 13:51:12 +0000

    libexec/rc.d/hostapd: Down/up interface when interface is specified

    When no interface is specified results in a syntax error in the rc
    script. Only execute poststart when an interface has been specified.

    PR:             263358
    Submitted by:   markj
    Reported by:    Joshua Kinard <freebsd@kumba.dev>
    Fixes:          0da2c91e64528d896f69d36670e25b4b4a140579

    (cherry picked from commit 1452bfcd9bbcb2f5bbb89fa38d01ce51dd9b6d44)

 libexec/rc/rc.d/hostapd | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
Comment 8 Cy Schubert freebsd_committer freebsd_triage 2022-04-25 13:56:12 UTC
Fixed.