bugfix in segment deconstructor#5687
Conversation
WalkthroughIn ChangesSegment Destructor Transition Cleanup
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wled00/FX.h`:
- Line 621: The destructor call to stopTransition() is missing a null guard that
all other callers use, and since stopTransition() dereferences _t->_oldSegment
in debug output without checking for null, it will crash when destroying a
Segment that never had an active transition. Add an `if (_t)` guard before the
stopTransition() call at the destructor to match the pattern used at other call
sites (like lines 101, 134, 455), or alternatively add a null check at the start
of the stopTransition() method itself that returns early if _t is null, which
would prevent similar issues at all call sites and aligns with the function's
documented behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| DEBUGFX_PRINTLN(); | ||
| #endif | ||
| clearName(); | ||
| stopTransition(); // deallocate "_t" (transition) and with it "_segOld" note: _segOld has _t=null, see copy constructor |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify stopTransition() has null guard before accessing _t
# Search for the complete stopTransition() implementation
ast-grep --pattern $'void Segment::stopTransition() {
$$$
}'
# Also check for any null checks on _t before the debug print
rg -nC5 'void Segment::stopTransition'Repository: wled/WLED
Length of output: 570
🏁 Script executed:
# Check Segment constructor to see if _t is initialized
rg -nC10 'Segment::Segment\(' wled00/FX.h wled00/FX_fcn.cpp | head -60
# Check if _t is initialized in constructors
rg -nC3 '_t\s*=' wled00/FX.h wled00/FX_fcn.cpp | head -30
# Check for any existing null checks before stopTransition() calls
rg -nB3 'stopTransition\(\)' wled00/Repository: wled/WLED
Length of output: 6782
Fix null pointer dereference in destructor's stopTransition() call.
The new destructor call to stopTransition() at line 621 lacks a null guard. However, all other callers in the codebase guard this call with if (_t) checks (see lines 101, 134, 455). The stopTransition() implementation (FX_fcn.cpp:335-339) dereferences _t->_oldSegment in its debug print without null checking, which means calling it with _t == nullptr will crash in debug builds when destroying a normal Segment that never had a transition active.
Change line 621 to:
if (_t) stopTransition(); // deallocate "_t" (transition) and with it "_segOld"Alternatively, add a null guard at the start of stopTransition():
void Segment::stopTransition() {
if (!_t) return; // does nothing if not in transition
DEBUG_PRINTF_P(PSTR("-- Stopping transition: S=%p T(%p) O[%p]\n"), this, _t, _t->_oldSegment);
delete _t;
_t = nullptr;
}The second approach aligns with the function's documented behavior ("does nothing if not in transition") and would prevent similar issues at other call sites.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wled00/FX.h` at line 621, The destructor call to stopTransition() is missing
a null guard that all other callers use, and since stopTransition() dereferences
_t->_oldSegment in debug output without checking for null, it will crash when
destroying a Segment that never had an active transition. Add an `if (_t)` guard
before the stopTransition() call at the destructor to match the pattern used at
other call sites (like lines 101, 134, 455), or alternatively add a null check
at the start of the stopTransition() method itself that returns early if _t is
null, which would prevent similar issues at all call sites and aligns with the
function's documented behavior.
Source: Learnings
1 participant
fix memory leak as discussed in #4304
fixes #4304
Summary by CodeRabbit