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) diff --git a/tests/test_editAlgorithm.cpp b/tests/test_editAlgorithm.cpp index 30b82858d..7bb8c4bf2 100644 --- a/tests/test_editAlgorithm.cpp +++ b/tests/test_editAlgorithm.cpp @@ -2131,6 +2131,126 @@ main(int argc, char** argv) TimeRange(RationalTime(0.0, 24.0), RationalTime(10.0, 24.0)) }); }); + // Regression Test: Null Pointer Dereference from Unchecked dynamic_cast + // + // The editAlgorithm functions call dynamic_cast(item->clone()) and + // _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 + // _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 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. + 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; + }; + + // 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. + 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)); + }); + + // 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)); + }); + + // 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)); + }); + + // 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; }