From b0f33db24199f28ec843fd6be4563f1d5a4ce2f0 Mon Sep 17 00:00:00 2001 From: Joshua Minor Date: Sun, 26 Apr 2026 20:58:30 -0700 Subject: [PATCH 1/3] "Red" test to prove null deref is possible. Assisted-by: Claude Code / Opus 4.7 --- tests/test_editAlgorithm.cpp | 119 +++++++++++++++++++++++++++++++++++ 1 file changed, 119 insertions(+) diff --git a/tests/test_editAlgorithm.cpp b/tests/test_editAlgorithm.cpp index 30b82858d..fac14df4c 100644 --- a/tests/test_editAlgorithm.cpp +++ b/tests/test_editAlgorithm.cpp @@ -2131,6 +2131,125 @@ main(int argc, char** argv) TimeRange(RationalTime(0.0, 24.0), RationalTime(10.0, 24.0)) }); }); + // Null Pointer Dereference from Unchecked dynamic_cast + // + // The editAlgorithm functions call dynamic_cast(item->clone()) and + // immediately dereference the result. SerializableObject::clone() can + // return nullptr (for example, when the object's metadata contains a + // cycle, which causes the cloning encoder to error out). When that + // happens, the dynamic_cast also yields nullptr and the next member call + // is a null pointer dereference (denial of service / crash). + // + // The tests below construct Items whose metadata contains a cycle (the + // clip references itself) and exercise each of the four affected call + // sites. After the fix, the algorithms must not crash and must report a + // non-OK error status rather than dereferencing nullptr. + // + // Helper: make a clip whose clone() will fail because of a metadata cycle. + auto make_clip_with_cyclic_metadata = [](std::string const& name, + TimeRange const& source_range) + -> SerializableObject::Retainer { + SerializableObject::Retainer clip = + new Clip(name, nullptr, source_range); + // Insert a self-reference in the metadata so that clone() (which + // round-trips through serialization) detects an OBJECT_CYCLE and + // returns nullptr. + clip->metadata()["self"] = SerializableObject::Retainer<>(clip.value); + // Sanity check: cloning this clip should fail. + OTIO_NS::ErrorStatus err; + SerializableObject* cloned = clip->clone(&err); + assertTrue(cloned == nullptr); + assertTrue(is_error(err)); + return clip; + }; + + // Line 185: overwrite() splits a single clip whose middle is overwritten. + tests.add_test("test_edit_overwrite_null_clone_safe", [&] { + auto clip = make_clip_with_cyclic_metadata( + "cyclic", + TimeRange(RationalTime(0.0, 24.0), RationalTime(24.0, 24.0))); + SerializableObject::Retainer track = new Track(); + track->append_child(clip); + + SerializableObject::Retainer insert_clip = new Clip( + "insert", + nullptr, + TimeRange(RationalTime(0.0, 24.0), RationalTime(4.0, 24.0))); + + OTIO_NS::ErrorStatus error_status; + // Overwrite a 4-frame range in the middle of the cyclic clip. This + // forces the code path that clones items.front() to produce the + // trailing slice (line 185). + algo::overwrite( + insert_clip, + track, + TimeRange(RationalTime(8.0, 24.0), RationalTime(4.0, 24.0)), + true, + nullptr, + &error_status); + // Must not crash; must report an error. + assertTrue(is_error(error_status)); + }); + + // Line 367: insert() splits an existing clip and clones it for the tail. + tests.add_test("test_edit_insert_null_clone_safe", [&] { + auto clip = make_clip_with_cyclic_metadata( + "cyclic", + TimeRange(RationalTime(0.0, 24.0), RationalTime(24.0, 24.0))); + SerializableObject::Retainer track = new Track(); + track->append_child(clip); + + SerializableObject::Retainer insert_clip = new Clip( + "insert", + nullptr, + TimeRange(RationalTime(0.0, 24.0), RationalTime(4.0, 24.0))); + + OTIO_NS::ErrorStatus error_status; + algo::insert( + insert_clip, + track, + RationalTime(12.0, 24.0), + true, + nullptr, + &error_status); + assertTrue(is_error(error_status)); + }); + + // Line 534: slice() clones an item to create the second slice. + tests.add_test("test_edit_slice_null_clone_safe", [&] { + auto clip = make_clip_with_cyclic_metadata( + "cyclic", + TimeRange(RationalTime(0.0, 24.0), RationalTime(24.0, 24.0))); + SerializableObject::Retainer track = new Track(); + track->append_child(clip); + + OTIO_NS::ErrorStatus error_status; + algo::slice(track, RationalTime(12.0, 24.0), true, &error_status); + assertTrue(is_error(error_status)); + }); + + // Line 795: fill() clones the source item before placing it on the track. + tests.add_test("test_edit_fill_null_clone_safe", [&] { + // Track with a gap so that fill() can find a slot to fill. + SerializableObject::Retainer gap = new Gap( + TimeRange(RationalTime(0.0, 24.0), RationalTime(24.0, 24.0))); + SerializableObject::Retainer track = new Track(); + track->append_child(gap); + + auto clip = make_clip_with_cyclic_metadata( + "cyclic", + TimeRange(RationalTime(0.0, 24.0), RationalTime(24.0, 24.0))); + + OTIO_NS::ErrorStatus error_status; + algo::fill( + clip, + track, + RationalTime(0.0, 24.0), + ReferencePoint::Sequence, + &error_status); + assertTrue(is_error(error_status)); + }); + tests.run(argc, argv); return 0; } From 1a6318c7f427645dee95f6da0acc29afcb983b10 Mon Sep 17 00:00:00 2001 From: Joshua Minor Date: Sun, 26 Apr 2026 20:58:56 -0700 Subject: [PATCH 2/3] Fix for null pointer deref after dynamic_cast. Test now passes "green". Assisted-by: Claude Code / Opus 4.7 Note: Claude Code Opus 4.6 was used to find this bug, and Opus 4.7 was used to fix it. --- src/opentimelineio/algo/editAlgorithm.cpp | 46 +++++++++++++++++++++-- 1 file changed, 43 insertions(+), 3 deletions(-) diff --git a/src/opentimelineio/algo/editAlgorithm.cpp b/src/opentimelineio/algo/editAlgorithm.cpp index 55e41f226..25c0b300e 100644 --- a/src/opentimelineio/algo/editAlgorithm.cpp +++ b/src/opentimelineio/algo/editAlgorithm.cpp @@ -182,8 +182,18 @@ overwrite( if (!isEqual(second_duration.value(), 0.0)) { auto second_item = dynamic_cast(items.front()->clone()); - trimmed_range = second_item->trimmed_range(); - source_range = TimeRange( + if (!second_item) + { + // clone() failed or returned an object that is not an Item. + // Bail out before dereferencing nullptr (CWE-476). + if (error_status) + *error_status = ErrorStatus( + ErrorStatus::INTERNAL_ERROR, + "failed to clone item for overwrite split"); + return; + } + trimmed_range = second_item->trimmed_range(); + source_range = TimeRange( trimmed_range.start_time() + first_duration + range.duration(), second_duration); @@ -363,6 +373,16 @@ insert( if (!isEqual(second_source_range.duration().value(), 0.0)) { auto second_item = dynamic_cast(item->clone()); + if (!second_item) + { + // clone() failed or returned an object that is not an Item. + // Bail out before dereferencing nullptr (CWE-476). + if (error_status) + *error_status = ErrorStatus( + ErrorStatus::INTERNAL_ERROR, + "failed to clone item for insert split"); + return; + } second_item->set_source_range(second_source_range); composition->insert_child(insert_index + 1, second_item); } @@ -530,7 +550,17 @@ slice( item->set_source_range(first_source_range); // Clone the item for the second slice. - auto second_item = dynamic_cast(item->clone()); + auto second_item = dynamic_cast(item->clone()); + if (!second_item) + { + // clone() failed or returned an object that is not an Item. + // Bail out before dereferencing nullptr (CWE-476). + if (error_status) + *error_status = ErrorStatus( + ErrorStatus::INTERNAL_ERROR, + "failed to clone item for slice"); + return; + } const TimeRange second_source_range( first_source_range.start_time() + first_source_range.duration(), range.duration() - first_source_range.duration()); @@ -794,6 +824,16 @@ fill( RationalTime start_time = clip_range.start_time(); const RationalTime gap_start_time = gap_range.start_time(); auto track_item = dynamic_cast(item->clone()); + if (!track_item) + { + // clone() failed or did not return an Item; bail out safely + // instead of dereferencing nullptr (CWE-476). + if (error_status) + *error_status = ErrorStatus( + ErrorStatus::INTERNAL_ERROR, + "failed to clone item for fill"); + return; + } // Check if start time is less than gap's start time (trim it if so) if (start_time < gap_start_time) From 8452d567df54fa4809fb68bff1c37f73e1c9438d Mon Sep 17 00:00:00 2001 From: Joshua Minor Date: Sun, 26 Apr 2026 21:06:01 -0700 Subject: [PATCH 3/3] Rephrased comments to reflect the fixed bug. Removed references to specific line numbers. --- tests/test_editAlgorithm.cpp | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/tests/test_editAlgorithm.cpp b/tests/test_editAlgorithm.cpp index fac14df4c..7bb8c4bf2 100644 --- a/tests/test_editAlgorithm.cpp +++ b/tests/test_editAlgorithm.cpp @@ -2131,18 +2131,19 @@ main(int argc, char** argv) TimeRange(RationalTime(0.0, 24.0), RationalTime(10.0, 24.0)) }); }); - // Null Pointer Dereference from Unchecked dynamic_cast + // Regression Test: Null Pointer Dereference from Unchecked dynamic_cast // // The editAlgorithm functions call dynamic_cast(item->clone()) and - // immediately dereference the result. SerializableObject::clone() can + // _used to_ immediately dereference the result. + // SerializableObject::clone() can // return nullptr (for example, when the object's metadata contains a // cycle, which causes the cloning encoder to error out). When that // happens, the dynamic_cast also yields nullptr and the next member call - // is a null pointer dereference (denial of service / crash). + // _used to_ dereference the null pointer (denial of service / crash). // // The tests below construct Items whose metadata contains a cycle (the // clip references itself) and exercise each of the four affected call - // sites. After the fix, the algorithms must not crash and must report a + // sites. After the fix, the algorithms does not crash and reports a // non-OK error status rather than dereferencing nullptr. // // Helper: make a clip whose clone() will fail because of a metadata cycle. @@ -2163,7 +2164,7 @@ main(int argc, char** argv) return clip; }; - // Line 185: overwrite() splits a single clip whose middle is overwritten. + // overwrite() splits a single clip whose middle is overwritten. tests.add_test("test_edit_overwrite_null_clone_safe", [&] { auto clip = make_clip_with_cyclic_metadata( "cyclic", @@ -2179,7 +2180,7 @@ main(int argc, char** argv) OTIO_NS::ErrorStatus error_status; // Overwrite a 4-frame range in the middle of the cyclic clip. This // forces the code path that clones items.front() to produce the - // trailing slice (line 185). + // trailing slice. algo::overwrite( insert_clip, track, @@ -2191,7 +2192,7 @@ main(int argc, char** argv) assertTrue(is_error(error_status)); }); - // Line 367: insert() splits an existing clip and clones it for the tail. + // insert() splits an existing clip and clones it for the tail. tests.add_test("test_edit_insert_null_clone_safe", [&] { auto clip = make_clip_with_cyclic_metadata( "cyclic", @@ -2215,7 +2216,7 @@ main(int argc, char** argv) assertTrue(is_error(error_status)); }); - // Line 534: slice() clones an item to create the second slice. + // slice() clones an item to create the second slice. tests.add_test("test_edit_slice_null_clone_safe", [&] { auto clip = make_clip_with_cyclic_metadata( "cyclic", @@ -2228,7 +2229,7 @@ main(int argc, char** argv) assertTrue(is_error(error_status)); }); - // Line 795: fill() clones the source item before placing it on the track. + // fill() clones the source item before placing it on the track. tests.add_test("test_edit_fill_null_clone_safe", [&] { // Track with a gap so that fill() can find a slot to fill. SerializableObject::Retainer gap = new Gap(