dakejahl opened a new pull request, #19436:
URL: https://github.com/apache/nuttx/pull/19436
## Summary
`igmp_input()` never handles IGMP General Queries. The test that
distinguishes a General Query from a Group-Specific Query compares the
*address* of a struct member against zero, so it is always false and the
General Query branch is unreachable. GCC discards the branch entirely.
The group address is declared in `include/nuttx/net/igmp.h` as
```c
uint16_t grpaddr[2]; /* 32-bit Group address */
```
so `igmp->grpaddr` decays to `FAR uint16_t *`. Comparing it to `INADDR_ANY`
compares a pointer, not the address the packet carries.
This was reported once before, as a compiler warning rather than as a
defect. Commit 09bb292fa2 (#7273, "net/igmp: fix build warning on GCC 12.2.0")
changed
```c
if (igmp->grpaddr == 0)
```
to
```c
if (net_ipv4addr_cmp(igmp->grpaddr, INADDR_ANY) != 0)
```
`net_ipv4addr_cmp(a, b)` expands to `(a == b)` and `INADDR_ANY` expands to
`((in_addr_t)0)`, so the emitted comparison is unchanged. The `-Waddress`
diagnostic disappeared for a different reason: the comparison now originates
inside a macro expanded from a header reached via `-isystem`, and GCC
suppresses warnings from system-header macros. Demonstration with the same
expression, macro in a header:
```
$ gcc -c -O2 -Wall -I sysinc lauder.c
sysinc/nxmacros.h:5:48: warning: comparison between pointer and zero
character constant [-Wpointer-compare]
$ gcc -c -O2 -Wall -isystem sysinc lauder.c # what NuttX's build
actually does
$ # silent
```
The defect was hidden rather than fixed, and the report was closed. That
commit also rewrote the unicast query test from `group->grpaddr != 0`, which
was well-formed (`group->grpaddr` is an `in_addr_t`), into the same pointer
comparison, making it unconditionally true.
The fix converts the header field once with `net_ip4addr_conv32()` and
compares the resulting `in_addr_t`. The conversion was already performed in the
group-specific branch, so this only hoists it and reuses it at the other two
sites. `net_ipv4addr_hdrcmp()` is not usable here because it applies
`net_ip4addr_conv32()` to *both* operands, and the second operand is the scalar
`INADDR_ANY`.
## Impact
Affects all users with `CONFIG_NET_IGMP=y`. No API, configuration, or
build-system change; no new symbols. Code size grows by the branch that is
currently being discarded (+149 bytes of `.text` on sim, see below).
A General Query — destination `224.0.0.1`, group address `0`, per RFC 2236
§2.4 — is the periodic query every IGMP querier emits, typically every 125 s.
Today it falls through to the group-specific branch, where
`igmp_grpallocfind(dev, &grpaddr)` is called with `grpaddr == 0.0.0.0`. That
allocates a group entry for `0.0.0.0` and schedules a membership report for it,
while the groups the device actually joined never get their report timers
restarted. The querier then ages out those memberships and multicast delivery
to the device stops.
## Testing
Host: Linux x86_64, GCC 13.
Board/config: `sim:dynconns`, which sets `CONFIG_NET_IGMP=y`. Also
cross-checked the affected expressions with `arm-none-eabi-gcc 13.2.1`.
`tools/checkpatch.sh -c -u -m -g master..HEAD` (the invocation CI uses):
```
Used config files:
1: .codespellrc
✔️ All checks pass.
```
`sim:dynconns` builds cleanly before and after; the file produces no
diagnostics either way, since the warning is suppressed as described above. The
behavioural change is visible in the generated code instead. `igmp->maxresp =
10;` occurs only inside the General Query branch, so it is a reliable marker
for whether that branch survives:
```
$ make && cd net && size igmp_input.o && objdump -d igmp_input.o | grep -c
'movb.*\$0xa'
```
| | `.text` | `igmp->maxresp = 10` present |
|---|---|---|
| master (`073570a103`) | 510 | 0 — branch eliminated as unreachable |
| this PR | 659 | 1 |
Before this change the compiler proves the General Query handler is dead and
removes it; after it, the handler is emitted.
The defect was originally found by a `-Werror` build of PX4's NuttX fork,
which predates 09bb292fa2 and so still carries the original `igmp->grpaddr ==
0` and still fails to build. That fork is functionally affected in exactly the
same way.
Signed-off-by: Jacob Dahl <[email protected]>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
