realtek-amb: fix double free and truncated copy of Arduino WiFi event data

[Bl00d-B0b]

Summary

Three defects in the Arduino WiFi event path shared by all realtek-amb families (AmebaZ, AmebaZ2, and the AmebaD work in progress). Found while bringing up RTL8720DN: the device hard-faulted inside memcpy called from wifi_indication(), with 0xdeadbeef in R0-R3 (heap poison).

1. Double free of the event buffer

WiFiSTA.cpp:157 queues a real EventInfo * with flags == -2:

wifi_indication(WIFI_EVENT_CONNECT, (char *)eventInfo, ARDUINO_EVENT_WIFI_STA_GOT_IP, -2);

wifi_indication() copies it into ev->buf. Then handleRtwEvent() frees that copy in the flags == -2 branch, and wifiEventTask() frees the same pointer again immediately afterwards. wifiEventTask() now owns and frees the queued copy for every flags value; handleRtwEvent() frees nothing — one deletion retires the double free on both the queued and the non-queued path.

2. Copy length taken from an event id

On the flags == -2 path buf_len carries the Arduino event id, not a byte count, so the EventInfo was copied at whatever length that id happened to be. It now copies sizeof(EventInfo).

3. Null event info dereferenced

Events raised with no payload (ARDUINO_EVENT_WIFI_STA_START and friends in WiFiGeneric.cpp/WiFiAP.cpp) pass buf == NULL, which reached *eventInfo. Now handled explicitly.

The copy is also guarded against a buf that is not a pointer: the AmebaD driver calls wifi_indication() with buf = 0x1, buf_len = 2 while switching modes, which is what produced the original fault.

Testing

• Built for bw15 (AmebaZ2) with this change: SUCCESS, RAM 4.8%, flash 38.5%.
• On RTL8720DN hardware the fault is gone and the device completes association and DHCP; the same code path now runs to GOT_IP without heap corruption.

[bdraco]

@esphbot review

[esphbot]

@bdraco: @esphbot review

Reviewed. Three fixes verified against source.

GOT_IP enum equals 7. Old copy truncated EventInfo to 7 bytes. STA_START enum equals 2, with buf=NULL — that memcpy caused the reported fault. Double free confirmed at former lines 97 and 127.

Two blockers remain in the new guard. (size_t)buf_len admits negative lengths the old signed check rejected. And ev->buf_len stays non-zero after the pointer check nulls ev->buf, so WIFI_EVENT_STA_DISASSOC derefs NULL.

Both are one-line fixes. Details inline.

[esphbot]

Previous review — superseded by a newer review below.

[esphbot]

Warning

Important issues found.

• Signed guard weakened: negative buf_len now passes the length check
• When the copy is skipped, buf_len is still forwarded — NULL data with a non-zero length

[bdraco]

Sorry, wrong window, didn't mean to drop a review from the bot on this one.

[Bl00d-B0b]

Both blockers plus the three non-blocking notes, in e3326f7:

1. Signed guard — the sign check now runs before the cast: (flags == -2 || buf_len > 0) gates entry, and (size_t)buf_len only executes on the positive path. A negative length can no longer widen to SIZE_MAX.
2. Forwarded length — when no copy survives, the length is zeroed with the pointer (if (!bufCopy) buf_len = 0;), so handleRtwEvent() and SDK handlers never see NULL data with a non-zero length. The WIFI_EVENT_STA_DISASSOC deref path is closed.
3. Ownership split — took the simpler model you suggested: the queue owns its copy for every flags value and frees it in wifiEventTask(); handleRtwEvent() never frees what it is passed. This also removes the pre-existing double free on the non-queued fallback path (WiFiSTA.cpp frees its own buffer after the call, as it always did).
4. 0x1000 is now WIFI_EVENT_BUF_MIN_ADDR with a comment.
5. Both malloc results are checked; a failed event allocation drops the event instead of faulting.

Hardware state: an RTL8720DN (BW16) is running this exact code — boots, associates, DHCP, and serves the ESPHome native API with no heap corruption across repeated reconnects. clang-format 19 clean.

[esphbot]

@Bl00d-B0b: Both blockers plus the three non-blocking notes, in e3326f7:

Important

Items 1, 3, 4, 5 confirmed fixed. Ownership model now correct both directions. Item 2 overshoots.

On flags == -2, buf_len carries EventId, not a length. Zeroing it maps STA_START(2), STA_STOP(3), AP_START(10), AP_STOP(11) to event 0 — ARDUINO_EVENT_WIFI_READY. Those four callers pass buf = NULL unconditionally (WiFiGeneric.cpp:149/151/154, WiFiAP.cpp:85), so every mode change hits it.

Gate on flags: if (!bufCopy && flags != -2) buf_len = 0;. handleRtwEvent() returns early for -2, before SDK handlers and before the DISASSOC deref — nothing downstream needs that zero.

[esphbot]

Previous review — superseded by a newer review below.

[esphbot]

Warning

Important issues found.

• Zeroing buf_len destroys the Arduino event id on the flags == -2 path

[Bl00d-B0b]

All three, in 18eb65d:

1. The zeroing is gated on flags != -2, with a comment stating why the id must survive. STA_START/STA_STOP/AP_START/AP_STOP deliver their real ids again — verified by reading the queue consumer: handleRtwEvent() takes the -2 early-return before anything reads len as a length.
2. A failed payload allocation now frees the event and returns, so !bufCopy means exactly "no payload was supplied".
3. WIFI_EVENT_STA_DISASSOC validates data && len >= 6 before the MAC copy.

Branch is 0 behind master. Compiled for the AmebaD family (shared file); the previous head runs on BW16 hardware — this delta changes only which id the four no-payload events carry.

[esphbot]

@Bl00d-B0b: All three, in 18eb65d:

Verified all three against the tree.

1. Gate on flags != -2 correct. handleRtwEvent() returns at line 141 before anything reads len as a length. Four no-payload events carry real ids again.
2. free(ev); return; narrows !bufCopy to one meaning.
3. DISASSOC guard also covers the direct-call path, which skips sanitization entirely — startWifiTask() runs first in modePriv(), so that path stays unreachable anyway.

One nit: PR body bullet #1 still describes the reverted design — "task skips the free for Arduino events, leaving ownership with handleRtwEvent()". Code does the opposite. Reword before merge; body becomes the commit message.

[esphbot]

Previous review — superseded by a newer review below.

[esphbot]

Tip

No blocking issues found — ready to merge.

[esphbot]

Note

No code changes since the last review (18eb65d) — skipping. Push new commits or re-request a review to run again.

[Bl00d-B0b]

Hardware confirmation of the final revision on RTL8720DN: with event-flow instrumentation in the queue path, a no-payload Arduino event now arrives as

ind ev=0 len=2 fl=-2  ->  arduino id=2

— STA_START (2) delivered with its id intact through the NULL-buf path, the exact case the last review round fixed. Association, DHCP and the ESPHome native API all run on this code.

[Bl00d-B0b]

Both notes addressed: the description's ownership bullet now states the final design (task owns and frees the queued copy for every flags value; handleRtwEvent frees nothing), and 0a14b62 adds a log line on both heap-pressure drop paths in wifi_indication() — the event-record malloc and the payload malloc — so a dropped disconnect under memory pressure is visible in the field. The third path (address below WIFI_EVENT_BUF_MIN_ADDR) stays silent deliberately: that is the documented value-in-buf convention (AmebaD passes buf=0x1 on every no-payload Arduino event), so a log there would fire on routine mode changes, not on losses.

[esphbot]

@Bl00d-B0b: Both notes addressed: the description's ownership bullet now states the final design (task owns and ...

Both notes confirmed closed. Description bullet now matches implemented ownership model. Log lines cover both heap-pressure drops.

Low-address silence accepted per your rationale: buf=0x1 fires on routine mode changes, so a log line there would report normal traffic as loss. Finding dropped, not re-raised.

One leftover note only: flags == -2 copy length now fixed at sizeof(EventInfo), enforced nowhere. Safe for all four current call sites. Worth one comment line.

[esphbot]

PR Review — realtek-amb: fix double free and truncated copy of Arduino WiFi event data

Both outstanding notes are closed and nothing new blocks merge — approving.

The ownership model is now stated identically in three places that used to disagree: the PR body, the inline comment at WiFiEvents.cpp:120, and the code itself — wifiEventTask() frees the queued copy for every flags value, handleRtwEvent() frees nothing, WiFiSTA.cpp:159 frees its own calloc. That single deletion retires the double free on the queued and the direct-call path. I re-verified the supporting facts independently: EventInfo is a union (WiFiPrivate.h:73), so EventInfo empty = {} zero-initializes fully and postEvent(EventId, EventInfo) (WiFi.h:90) takes it by value; handleRtwEvent() has exactly one caller in the tree, so no other path depended on the removed free(); and the !data || len < 6 guard on WIFI_EVENT_STA_DISASSOC also covers the unsanitized fallback path, where the sibling STA_ASSOC case is already protected by its len != 124 check.

The sanitization logic holds across all four combinations I traced: valid pointer + flags == -2 copies sizeof(EventInfo); NULL + flags == -2 delivers an empty payload with the event id intact (matching your ind ev=0 len=2 fl=-2 -> arduino id=2 capture); rejected/NULL pointer with flags != -2 zeroes buf_len so no case ever sees a length without data; and the free(ev); return; on copy failure keeps !bufCopy meaning exactly one thing.

• Prior suggestion ＃1 (description/code mismatch on ownership) — resolved; the body now describes the implemented design
• Prior suggestion ＃2 (silent drops) — resolved for both heap-pressure paths via 0a14b62. The third path staying silent is dismissed per @Bl00d-B0b: the sub-0x1000 address is the documented value-in-buf convention and would log on routine AmebaD mode changes, not on losses. Not re-raised
• One non-blocking note: the flags == -2 copy length is now fixed at sizeof(EventInfo) with no caller-side contract enforcing it — verified safe for all four current call sites, worth one comment line for the next one

---

✅ Resolved since last review (1)

Previously-flagged issues verified fixed

• cores/realtek-amb/arduino/libraries/WiFi/WiFiEvents.cpp:88 Rejected pointers and failed allocations drop events with no diagnostics

---

🟢 Suggestions

1. `flags == -2` now carries an implicit "buffer is at least sizeof(EventInfo)" contract with nothing enforcing it
cores/realtek-amb/arduino/libraries/WiFi/WiFiEvents.cpp:90-91

Correct as written — I verified every flags == -2 caller in the tree:

• WiFiGeneric.cpp:149,151,154 and WiFiAP.cpp:85 pass buf = NULL (handled by the address guard)
• WiFiSTA.cpp:157 passes calloc(1, sizeof(EventInfo)) — exactly the length copied

So the fixed-size copy is safe today, and it is a strict improvement over the old code, which copied buf_len bytes where buf_len was the event id (ARDUINO_EVENT_WIFI_STA_GOT_IP = 7) out of a sizeof(EventInfo)-sized allocation and then had postEvent() read the whole union — the over-read this PR retires.

Why it's worth a note: the length is no longer derived from anything the caller supplies. A future flags == -2 call site that passes a pointer to a narrower struct (say just a wifi_event_sta_disconnected_t, or a stack local) compiles cleanly and silently over-reads by tens of bytes — the same class of implicit-length contract that produced the bug being fixed here. The comment above the branch explains why buf_len is not a length, but not that the buffer must be a full EventInfo.

Fix: one line in the existing comment block is enough, e.g. "callers on this path must pass a buffer of at least sizeof(EventInfo) — see WiFiSTA.cpp:149". No code change needed.

		if ((uintptr_t)buf >= WIFI_EVENT_BUF_MIN_ADDR && (flags == -2 || buf_len > 0)) {
			size_t copyLen = flags == -2 ? sizeof(EventInfo) : (size_t)buf_len;

---

Checklist

• Memory ownership is unambiguous across all call paths
• Length arithmetic safe against negative/overflow values
• Event identity preserved on all delivery paths
• Allocation failures handled and observable
• Null / non-pointer payloads handled on all event paths
• Copy length derived from a real size, not an event id — suggestion ＃1
• Magic values named and documented
• PR description matches the implemented design
• Diff matches PR scope, no scope creep

Important

The branch moved during review. This review was performed against HEAD=0a14b62, but the PR branch now points at d46c3d4. Commits pushed after the review started are not reflected below — re-run /review to cover them.

---

Automated review by Kōan (Claude) HEAD=0a14b62 3 min 32s

[esphbot]

Tip

No blocking issues found — ready to merge.