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 6 commits into
base: main
Choose a base branch
from
Open

Fix null pointer dereferences after dynamic_cast (rewritten) #2020

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3d51797
Add tests for clone and clone-with-cycle
jminor May 1, 2026
310b244
Prevent crash in algo::fill()
jminor May 1, 2026
264aa1c
Fix crash in algo::insert
jminor May 1, 2026
c5c4741
Fix crash in algo::overwrite
jminor May 1, 2026
11ac50e
Fixed crash in algo::fill() + some refactoring of tests
jminor May 1, 2026
40478d6
Introduced ErrorStatus::CANNOT_CLONE_ITEM
jminor May 1, 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
64 changes: 58 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,22 @@ 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();
if (!cloned_item)
{
if (error_status)
*error_status = ErrorStatus::CANNOT_CLONE_ITEM;
Copy link
Copy Markdown
Contributor

@darbyjohnston darbyjohnston May 2, 2026

Choose a reason for hiding this comment

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

I think this error status should be set in clone(), so it is set only once and not at each call site. Something like:

auto cloned_item = items.front()->clone(error_status);
if (!cloned_item || (error_status && is_error(error_status))
    return;

SerializableObject*
SerializableObject::clone(ErrorStatus* error_status) const
{
    ...
    SerializableObject*r = e._root.type() == typeid(SerializableObject::Retainer<>)
               ? std::any_cast<SerializableObject::Retainer<>&>(e._root)
                     .take_value()
               : nullptr;
    if (!r && error_status)
        *error_status = ErrorStatus::CANNOT_CLONE_ITEM;
    return r;
}

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 +375,20 @@ 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();
if (!cloned_item)
{
if (error_status)
*error_status = ErrorStatus::CANNOT_CLONE_ITEM;
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 +556,20 @@ 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();
if (!cloned_item)
{
if (error_status)
*error_status = ErrorStatus::CANNOT_CLONE_ITEM;
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 +832,20 @@ 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();
if (!cloned_item)
{
if (error_status)
*error_status = ErrorStatus::CANNOT_CLONE_ITEM;
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
2 changes: 2 additions & 0 deletions src/opentimelineio/errorStatus.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,8 @@ ErrorStatus::outcome_to_string(Outcome o)
return "the media references cannot contain an empty key";
case NOT_A_GAP:
return "object is not descendent of Gap type";
case CANNOT_CLONE_ITEM:
return "cannot clone item";
default:
return "unknown/illegal ErrorStatus::Outcome code";
};
Expand Down
3 changes: 2 additions & 1 deletion src/opentimelineio/errorStatus.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,8 @@ struct OTIO_API_TYPE ErrorStatus
CANNOT_COMPUTE_BOUNDS,
MEDIA_REFERENCES_DO_NOT_CONTAIN_ACTIVE_KEY,
MEDIA_REFERENCES_CONTAIN_EMPTY_KEY,
NOT_A_GAP
NOT_A_GAP,
CANNOT_CLONE_ITEM
};

/// @brief Construct a new status with no error.
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", [] {
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::slice(track, RationalTime(12.0, 24.0), false, &error_status);

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

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::CANNOT_CLONE_ITEM);
});

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::CANNOT_CLONE_ITEM);
});

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);

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::CANNOT_CLONE_ITEM);
});

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