Skip to content

Fix null pointer dereferences after dynamic_cast (rewritten) #2020

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jminor wants to merge 7 commits into
base: main
Choose a base branch
from
+206 −6
Open

Fix null pointer dereferences after dynamic_cast (rewritten) #2020

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
774f1ce
Add tests for clone and clone-with-cycle
jminor May 1, 2026
54cef4f
Prevent crash in algo::fill()
jminor May 1, 2026
3ffb351
Fix crash in algo::insert
jminor May 1, 2026
934380a
Fix crash in algo::overwrite
jminor May 1, 2026
cd70b27
Fixed crash in algo::fill() + some refactoring of tests
jminor May 1, 2026
d993049
Introduced ErrorStatus::CANNOT_CLONE_ITEM
jminor May 1, 2026
7e8876a
Pass through error_status from clone() instead of inventing a new CAN…
jminor Jun 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 54 additions & 6 deletions src/opentimelineio/algo/editAlgorithm.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,9 +181,21 @@ overwrite(
composition->insert_child(insert_index, item);
if (!isEqual(second_duration.value(), 0.0))
{
auto second_item = dynamic_cast<Item*>(items.front()->clone());
trimmed_range = second_item->trimmed_range();
source_range = TimeRange(
auto cloned_item = items.front()->clone(error_status);
if (!cloned_item)
{
// error_status was already set within clone()
return;
}
auto second_item = dynamic_cast<Item*>(cloned_item);
if (!second_item)
{
if (error_status)
*error_status = ErrorStatus::TYPE_MISMATCH;
return;
}
trimmed_range = second_item->trimmed_range();
source_range = TimeRange(
trimmed_range.start_time() + first_duration
+ range.duration(),
second_duration);
Expand Down Expand Up @@ -362,7 +374,19 @@ insert(
// Clone the item for the second partially overwritten item.
if (!isEqual(second_source_range.duration().value(), 0.0))
{
auto second_item = dynamic_cast<Item*>(item->clone());
auto cloned_item = item->clone(error_status);
if (!cloned_item)
{
// error_status was already set within clone()
return;
}
auto second_item = dynamic_cast<Item*>(cloned_item);
if (!second_item)
{
if (error_status)
*error_status = ErrorStatus::TYPE_MISMATCH;
return;
}
second_item->set_source_range(second_source_range);
composition->insert_child(insert_index + 1, second_item);
}
Expand Down Expand Up @@ -530,7 +554,19 @@ slice(
item->set_source_range(first_source_range);

// Clone the item for the second slice.
auto second_item = dynamic_cast<Item*>(item->clone());
auto cloned_item = item->clone(error_status);
if (!cloned_item)
{
// error_status was already set within clone()
return;
}
auto second_item = dynamic_cast<Item*>(cloned_item);
if (!second_item)
{
if (error_status)
*error_status = ErrorStatus::TYPE_MISMATCH;
return;
}
const TimeRange second_source_range(
first_source_range.start_time() + first_source_range.duration(),
range.duration() - first_source_range.duration());
Expand Down Expand Up @@ -793,7 +829,19 @@ fill(
case ReferencePoint::Sequence: {
RationalTime start_time = clip_range.start_time();
const RationalTime gap_start_time = gap_range.start_time();
auto track_item = dynamic_cast<Item*>(item->clone());
auto cloned_item = item->clone(error_status);
if (!cloned_item)
{
// error_status was already set within clone()
return;
}
auto track_item = dynamic_cast<Item*>(cloned_item);
if (!track_item)
{
if (error_status)
*error_status = ErrorStatus::TYPE_MISMATCH;
return;
}

// Check if start time is less than gap's start time (trim it if so)
if (start_time < gap_start_time)
Expand Down
119 changes: 119 additions & 0 deletions tests/test_editAlgorithm.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2131,6 +2131,125 @@ main(int argc, char** argv)
TimeRange(RationalTime(0.0, 24.0), RationalTime(10.0, 24.0)) });
});

tests.add_test("regression: crash in slice", [] {

@darbyjohnston darbyjohnston Jun 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a big deal, but since the crash is fixed maybe the test name should change?

SerializableObject::Retainer<Clip> big_clip = new Clip(
"big clip",
nullptr,
TimeRange(RationalTime(0.0, 24.0), RationalTime(24.0, 24.0)));
big_clip->metadata()["cycle"] = big_clip;

SerializableObject::Retainer<Clip> small_clip = new Clip(

@darbyjohnston darbyjohnston Jun 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't look like small_clip is being used in this test?

"small clip",
nullptr,
TimeRange(RationalTime(0.0, 24.0), RationalTime(5.0, 24.0)));
small_clip->metadata()["cycle"] = small_clip;

SerializableObject::Retainer<Track> track = new Track();
track->append_child(big_clip);

OTIO_NS::ErrorStatus error_status;

algo::slice(track, RationalTime(12.0, 24.0), false, &error_status);

assert(is_error(error_status));
assert(error_status.outcome == OTIO_NS::ErrorStatus::TYPE_MISMATCH);
});

tests.add_test("regression: crash in overwrite", [] {
SerializableObject::Retainer<Clip> big_clip = new Clip(
"big clip",
nullptr,
TimeRange(RationalTime(0.0, 24.0), RationalTime(24.0, 24.0)));
big_clip->metadata()["cycle"] = big_clip;

SerializableObject::Retainer<Clip> small_clip = new Clip(
"small clip",
nullptr,
TimeRange(RationalTime(0.0, 24.0), RationalTime(5.0, 24.0)));
small_clip->metadata()["cycle"] = small_clip;

SerializableObject::Retainer<Track> track = new Track();
track->append_child(big_clip);

OTIO_NS::ErrorStatus error_status;

algo::overwrite(
small_clip,
track,
TimeRange(RationalTime(0.0, 24.0), RationalTime(12.0, 24.0)),
true,
nullptr,
&error_status);

assert(is_error(error_status));
assert(error_status.outcome == OTIO_NS::ErrorStatus::TYPE_MISMATCH);
});

tests.add_test("regression: crash in insert", [] {
SerializableObject::Retainer<Clip> big_clip = new Clip(
"big clip",
nullptr,
TimeRange(RationalTime(0.0, 24.0), RationalTime(24.0, 24.0)));
big_clip->metadata()["cycle"] = big_clip;

SerializableObject::Retainer<Clip> small_clip = new Clip(
"small clip",
nullptr,
TimeRange(RationalTime(0.0, 24.0), RationalTime(5.0, 24.0)));
small_clip->metadata()["cycle"] = small_clip;

SerializableObject::Retainer<Track> track = new Track();
track->append_child(big_clip);

OTIO_NS::ErrorStatus error_status;

algo::insert(
small_clip,
track,
RationalTime(12.0, 24.0),
true,
nullptr,
&error_status);

assert(is_error(error_status));
assert(error_status.outcome == OTIO_NS::ErrorStatus::TYPE_MISMATCH);
});

tests.add_test("regression: crash in fill", [] {
SerializableObject::Retainer<Clip> big_clip = new Clip(
"big clip",
nullptr,
TimeRange(RationalTime(0.0, 24.0), RationalTime(24.0, 24.0)));
big_clip->metadata()["cycle"] = big_clip;

SerializableObject::Retainer<Clip> small_clip = new Clip(
"small clip",
nullptr,
TimeRange(RationalTime(0.0, 24.0), RationalTime(5.0, 24.0)));
small_clip->metadata()["cycle"] = small_clip;

SerializableObject::Retainer<Gap> gap = new Gap(
TimeRange(RationalTime(0.0, 24.0), RationalTime(20.0, 24.0)),
"gap");

SerializableObject::Retainer<Track> track = new Track();
track->append_child(small_clip);
track->append_child(gap);
track->append_child(small_clip);

@darbyjohnston darbyjohnston Jun 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is adding the small_clip twice intentional?


OTIO_NS::ErrorStatus error_status;

algo::fill(
big_clip,
track,
RationalTime(12.0, 24.0),
ReferencePoint::Sequence,
&error_status);

assert(is_error(error_status));
assert(error_status.outcome == OTIO_NS::ErrorStatus::TYPE_MISMATCH);
});

tests.run(argc, argv);
return 0;
}
33 changes: 33 additions & 0 deletions tests/test_serialization.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,39 @@ main(int argc, char** argv)
})CONTENT");
});

tests.add_test("clone", [] {
auto json = R"CONTENT({
"OTIO_SCHEMA": "SerializableObjectWithMetadata.1",
"metadata": {
"a": 1,
"b": "two",
"c": [
3,
4,
5
],
"d": {
"hello": "nested"
}
},
"name": ""
})CONTENT";
auto original = SerializableObjectWithMetadata::from_json_string(json);
auto cloned = original->clone();
assert(cloned != nullptr);
OTIO_NS::ErrorStatus err;
auto cloned_json = cloned->to_json_string(&err, {}, 2);
assertFalse(is_error(err));
assertEqual(json, cloned_json.c_str());
});

tests.add_test("clone with cycle returns nullptr", [] {
auto original = new SerializableObjectWithMetadata();
original->metadata()["cycle"] = original;
auto cloned = original->clone();
assert(cloned == nullptr);
});

tests.run(argc, argv);
return 0;
}
Loading