Fixed nullptr crash in ETH IP comparing code - #5796
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughThe Ethernet static IP configuration check now requires a ChangesEthernet static IP configuration
Estimated code review effort: 1 (Trivial) | ~5 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@Tarik2142 can you explain a bit more on which build this crash is happening?
The cast to (uint32_t) was added to enforce that the uint32_t overload of "==" gets used, this overload should also handle nullptr properly - at least in the new framework. |
I am working on a fork for ESP32-S3 + W5500 ethernet https://github.com/smlight-tech/A1-SLWF-09-WLED I can say for sure that |
|
@coderabbitai please scan the WLED source code for other locations where an |
This comment was marked as duplicate.
This comment was marked as duplicate.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Candidate comparisons with zero or four-octet literals ==='
rg -n -P -i -C 3 \
'(?:==|!=)\s*(?:\(\s*(?:u?int(?:8|16|32|64)_t|unsigned\s+(?:long\s+)?int)\s*\)\s*)?(?:0[xX]0+\b|0U?\b|\{\s*0\s*,\s*0\s*,\s*0\s*,\s*0\s*\}|\[\s*0\s*,\s*0\s*,\s*0\s*,\s*0\s*\})|(?:0[xX]0+\b|0U?\b|\{\s*0\s*,\s*0\s*,\s*0\s*,\s*0\s*\}|\[\s*0\s*,\s*0\s*,\s*0\s*,\s*0\s*\})\s*(?:==|!=)' \
. \
-g '!**/.git/**' \
-g '!**/node_modules/**' \
-g '!**/build/**' \
-g '!**/dist/**' || true
echo
echo '=== IPAddress declarations, aliases, and assignments ==='
rg -n -P -i -C 2 \
'\bIPAddress\b|\b(?:staticIP|staticGW|staticSN|dnsAddress|gateway|subnet|localIP)\b' \
. \
-g '!**/.git/**' \
-g '!**/node_modules/**' \
-g '!**/build/**' \
-g '!**/dist/**' || true
echo
echo '=== Outline of network source candidates ==='
fd -t f '^network\.(cpp|h)$' . -x ast-grep outline {} --items all 2>/dev/null || trueLength of output: 180298 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Direct comparisons: known IPAddress members, globals, and expressions ==='
rg -n -P -C 2 --glob '*.{c,cc,cpp,cxx,h,hpp,ino}' \
--glob '!wled00/src/dependencies/**' \
'(?:\b(?:staticIP|staticGW|staticSN|dnsAddress|hueIP|realtimeIP|ntpServerIP|mqttIP|localIP|ipAddress|ip|client|clnt|knownIp|lastKnownIp|debugPrintHostIP|_client|lightsIP)\b\s*(?:==|!=)\s*(?:0(?:[uUlL]*)\b|0[xX]0+\b|\{\s*0\s*,\s*0\s*,\s*0\s*,\s*0\s*\}|\[\s*0\s*,\s*0\s*,\s*0\s*,\s*0\s*\})|(?:0(?:[uUlL]*)\b|0[xX]0+\b|\{\s*0\s*,\s*0\s*,\s*0\s*,\s*0\s*\}|\[\s*0\s*,\s*0\s*,\s*0\s*,\s*0\s*\})\s*(?:==|!=)\s*\b(?:staticIP|staticGW|staticSN|dnsAddress|hueIP|realtimeIP|ntpServerIP|mqttIP|localIP|ipAddress|ip|client|clnt|knownIp|lastKnownIp|debugPrintHostIP|_client|lightsIP)\b)' \
. || true
echo
echo '=== All direct zero comparisons in files that declare IPAddress (review candidates) ==='
python3 - <<'PY'
from pathlib import Path
import re
root = Path(".")
exts = {".c", ".cc", ".cpp", ".cxx", ".h", ".hpp", ".ino"}
skip_parts = {".git", "node_modules", "build", "dist"}
zero = re.compile(r'(?:==|!=)\s*(?:0(?:[uUlL]*)\b|0[xX]0+\b|\{\s*0\s*,\s*0\s*,\s*0\s*,\s*0\s*\}|\[\s*0\s*,\s*0\s*,\s*0\s*,\s*0\s*\})|(?:0(?:[uUlL]*)\b|0[xX]0+\b|\{\s*0\s*,\s*0\s*,\s*0\s*,\s*0\s*\}|\[\s*0\s*,\s*0\s*,\s*0\s*,\s*0\s*\})\s*(?:==|!=)')
for path in root.rglob("*"):
if path.suffix not in exts or any(part in skip_parts for part in path.parts):
continue
if "wled00/src/dependencies" in path.as_posix():
continue
try:
lines = path.read_text(errors="replace").splitlines()
except OSError:
continue
if not any(re.search(r'\bIPAddress\b', line) for line in lines):
continue
matches = [(i + 1, line) for i, line in enumerate(lines) if zero.search(line)]
if matches:
print(f"\n{path}:")
for number, line in matches:
print(f"{number}: {line}")
PY
echo
echo '=== Explicit IPAddress comparison overload calls and IPAddress() zero-value comparisons ==='
rg -n -P -C 2 --glob '*.{c,cc,cpp,cxx,h,hpp,ino}' \
--glob '!wled00/src/dependencies/**' \
'(?:==|!=)\s*(?:IPAddress\s*\(\s*\)|INADDR_(?:NONE|ANY))|(?:IPAddress\s*\(\s*\)|INADDR_(?:NONE|ANY))\s*(?:==|!=)' \
. || trueLength of output: 16098
The only unsafe direct comparison was the Ethernet condition that this PR changes in The related locations are already safe:
Other checks use an octet index, for example Therefore, no additional source locations should receive the You are interacting with an AI system. |
@coderabbitai can the condition also be expressed by comparing to |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Candidate static-IP conditions ==='
rg -n -C 5 --glob '*.{c,cc,cpp,cxx,h,hpp,ino}' \
'uint32_t\s*\(\s*multiWiFi\[.*\]\.(?:staticIP|staticGW)\s*\).*!=\s*0U|multiWiFi\[.*\]\.(?:staticIP|staticGW).*!=\s*(?:0U|IPAddress\s*\()' \
wled00
echo '=== IPAddress zero-value comparison usage ==='
rg -n -C 3 --glob '*.{c,cc,cpp,cxx,h,hpp,ino}' \
'\b[A-Za-z_][A-Za-z0-9_]*\s*(?:==|!=)\s*IPAddress\s*\(\s*(?:0\s*,\s*0\s*,\s*0\s*,\s*0\s*)?\)' \
wled00
echo '=== Available IPAddress declarations in repository headers ==='
rg -n -C 4 --glob '*.{h,hpp,hh}' \
'class\s+IPAddress|struct\s+IPAddress|operator==\s*\(|IPAddress\s*\(' \
. -g '!**/.git/**' -g '!**/.pio/**' -g '!**/dependencies/**' || trueLength of output: 5761
if (multiWiFi[selectedWiFi].staticIP != IPAddress(0, 0, 0, 0) &&
multiWiFi[selectedWiFi].staticGW != IPAddress(0, 0, 0, 0)) {This compares two For this IPv4 check, You are interacting with an AI system. |
|
@coderabbitai are there recommendations in case we decided to use IPv6 (future support)? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Relevant network configuration code ==='
rg -n -C 10 '\bETH\.config\s*\(|\bWiFi\.config\s*\(|\bIPAddress\b|staticIP|staticGW|staticSN|dnsAddress' wled00/network.cpp
echo '=== Address configuration data model ==='
rg -n -C 6 --glob '*.{h,hpp,cpp,cxx,ino}' \
'\b(?:struct|class)\s+\w*WiFi|staticIP|staticGW|staticSN|dnsAddress|\bIPAddress\b' \
wled00
echo '=== Build platforms/framework versions ==='
rg -n -C 3 --glob 'platformio*.ini' --glob '*platform*.ini' \
'platform|framework|arduino|espressif32|idf' .
echo '=== Local IPAddress declarations, if vendored ==='
rg -n -C 5 --glob '*.{h,hpp}' \
'class\s+IPAddress|struct\s+IPAddress|operator==\s*\(' \
. -g '!**/.git/**' -g '!**/.pio/**' -g '!**/dependencies/**' || trueLength of output: 113786 🤖 web_search executed:
Length of output: 15119 🤖 web_search executed:
Length of output: 13385
Recommended approach:
For the current condition in You are interacting with an AI system. |
the previous implementation was still ambiguous - depending on compiler versions and framework details, it could select an overload that treats 0x00000000 as a pointer, causing a null pointer deference. IPAddress() is the same as IPAddress(0, 0, 0, 0).
|
cherry picked to 16_x |
2 participants
This PR fixes a nullptr crash when using ethernet.
The current implementation treats the passed value as a pointer, and since it is 0, it causes a crash.
Сomparing with
IPAddress()is a more reliable option.Summary by CodeRabbit