Fix null pointer dereferences after dynamic_cast (rewritten)#2020
Fix null pointer dereferences after dynamic_cast (rewritten)#2020jminor wants to merge 7 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2020 +/- ##
==========================================
- Coverage 84.51% 84.33% -0.18%
==========================================
Files 181 181
Lines 13239 13267 +28
Branches 1202 1214 +12
==========================================
Hits 11189 11189
- Misses 1867 1895 +28
Partials 183 183
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
meshula
left a comment
meshula
left a comment
There was a problem hiding this comment.
This looks more rigorous than the initial fix.
darbyjohnston
left a comment
darbyjohnston
left a comment
There was a problem hiding this comment.
Thanks, I think this is clearer about what is being tested than the previous PR. When we are happy with this one, try asking Claude to compare the two and see what it thinks about the changes.
jminor
force-pushed
the
fix-null-ptr-deref-no-llm
branch
from
9ea57a5 to
896b205
Compare
Signed-off-by: Joshua Minor <jminor@users.noreply.github.com>
Signed-off-by: Joshua Minor <jminor@users.noreply.github.com>
Signed-off-by: Joshua Minor <jminor@users.noreply.github.com>
Signed-off-by: Joshua Minor <jminor@users.noreply.github.com>
Signed-off-by: Joshua Minor <jminor@users.noreply.github.com>
Signed-off-by: Joshua Minor <jminor@users.noreply.github.com>
…NOT_CLONE_ITEM error status. Signed-off-by: Joshua Minor <jminor@users.noreply.github.com>
jminor
force-pushed
the
fix-null-ptr-deref-no-llm
branch
from
896b205 to
7e8876a
Compare
| TimeRange(RationalTime(0.0, 24.0), RationalTime(24.0, 24.0))); | ||
| big_clip->metadata()["cycle"] = big_clip; | ||
|
|
||
| SerializableObject::Retainer<Clip> small_clip = new Clip( |
There was a problem hiding this comment.
It doesn't look like small_clip is being used in this test?
| SerializableObject::Retainer<Track> track = new Track(); | ||
| track->append_child(small_clip); | ||
| track->append_child(gap); | ||
| track->append_child(small_clip); |
There was a problem hiding this comment.
Is adding the small_clip twice intentional?
| TimeRange(RationalTime(0.0, 24.0), RationalTime(10.0, 24.0)) }); | ||
| }); | ||
|
|
||
| tests.add_test("regression: crash in slice", [] { |
There was a problem hiding this comment.
Not a big deal, but since the crash is fixed maybe the test name should change?
4 participants
Certain invalid OTIO object graphs may cause crashes due to code that assumes a dynamic_cast always returns a valid object. This PR introduces tests which demonstrate the issue and a fix to prevent the crash in those cases.
I made sure to write the tests first, ensure that they caused the crash (SEGFAULT) and then made the fix to each of the affected functions. I introduced a new ErrorStatus::CANNOT_CLONE_ITEM for this specific case. The code may also return ErrorStatus::TYPE_MISMATCH in a related edge case, but I was not able to create a scenario which triggers that case.
Note that the discovery of this issue was assisted by Claude Code Opus 4.6 and Opus 4.7, but all of the code in this PR was written by a human (me). This PR replaces #2017 which had LLM-generated code in it.