WiFi: fix dangling pointer in setHostname / softAPsetHostname - #412
Open
tomasgvivo wants to merge 1 commit into
Open
tomasgvivo wants to merge 1 commit into
tomasgvivo wants to merge 1 commit into
Conversation
netif_set_hostname() assigns the pointer it is given -- it does not copy:
#define netif_set_hostname(netif, name) \
do { if((netif) != NULL) { (netif)->hostname = name; }} while(0)
Every WiFiClass hostname setter passed the caller's pointer straight through,
so netif->hostname was left aliasing memory the caller owns. Callers cannot
reasonably satisfy that contract, and the convenience overload actively
violates it:
inline bool hostname(const String &aHostname) {
return setHostname(aHostname.c_str());
}
That takes .c_str() of a String temporary which is destroyed on return, so
WiFi.hostname("my-device") leaves netif->hostname dangling. The name is read
later, when DHCP builds option 12, by which point the memory has been reused --
so the device advertises whatever now occupies it. Observed in the field on
realtek-amb (RTL8720CF): a device calling WiFi.hostname() advertised the WiFi
SSID as its DHCP hostname, because the SSID string had landed in the freed
block. It is a use-after-free, so the symptom varies with heap state; a stack
buffer or a String member that is later reassigned fails the same way.
Store the hostname in fixed storage owned by WiFiClass instead, and hand lwIP
a pointer to that. Separate STA and AP buffers, since both netifs can be up at
once. Affects all three cores with a WiFi implementation:
realtek-amb, beken-72xx, lightning-ln882h
Compile-tested on realtek-amb (RTL8720CF) via both setHostname(const char *)
and hostname(const String &).
Signed-off-by: Tomas Gonzalez Vivo <1284744+tomasgvivo@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
netif_set_hostname()assigns the pointer it is given — it does not copy:Every
WiFiClasshostname setter passes the caller's pointer straight through,so
netif->hostnameends up aliasing memory the caller owns. Callers can'treasonably satisfy that contract, and the convenience overload actively
violates it:
That takes
.c_str()of aStringtemporary, destroyed on return. SoWiFi.hostname("my-device")leavesnetif->hostnamedangling. The name is readmuch later, when DHCP builds option 12 — by which point the memory has been
reused, and the device advertises whatever now occupies it.
Observed
On an RTL8720CF (
realtek-amb), a device callingWiFi.hostname()advertisedits own WiFi SSID as the DHCP hostname, visible in the router's lease table.
The SSID string had landed in the freed block. Being a use-after-free the
symptom varies with heap state — a stack buffer, or a
Stringmember laterreassigned, fails the same way but may look fine for a while.
setHostname()returns
trueregardless, so nothing signals a problem.Fix
Store the hostname in fixed storage owned by
WiFiClassand hand lwIP a pointerto that. Separate STA and AP buffers, since both netifs can be up at once.
Affects all three cores with a WiFi implementation —
realtek-amb,beken-72xx,lightning-ln882h— STA and AP in each, six call sites.Testing
Verified on hardware for
realtek-ambonly (RTL8720CF, Broadlink RM4 mini),which is the only device I have. Before the fix the DHCP lease showed the SSID;
after it, with the firmware deliberately calling the previously-dangling
hostname(const String &)overload and nothing else changed, the lease showsthe intended name:
Compile-tested through both
setHostname(const char *)andhostname(const String &).beken-72xxandlightning-ln882hare unverified — I have no hardware foreither. Those changes are mechanically identical to the Realtek one, but I'd
appreciate a second pair of eyes, or a test from someone with the hardware,
before they're trusted.
Note
LT_HOSTNAME_SIZEis 64 (63-octet practical DNS label limit plus NUL). Longernames are truncated rather than rejected; happy to change that if you'd prefer
an explicit failure. I also kept the fix inside the existing per-core
setHostname()implementations rather than restructuring — let me know ifyou'd rather it lived somewhere else.