Skip to content

Commit 9d3c3a1

Browse files
jminorjminor
authored
Fix null pointer dereferences after dynamic_cast (rewritten) (#2020)
Fixed potential crash in algo::fill(), algo::insert(), and algo::overwrite(). Added tests to prevent regressions. Signed-off-by: Joshua Minor <jminor@users.noreply.github.com>
1 parent 6944f10 commit 9d3c3a1

3 files changed

Lines changed: 207 additions & 6 deletions

File tree

‎src/opentimelineio/algo/editAlgorithm.cpp‎

Lines changed: 54 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -181,9 +181,21 @@ overwrite(
181181
composition->insert_child(insert_index, item);
182182
if (!isEqual(second_duration.value(), 0.0))
183183
{
184-
auto second_item = dynamic_cast<Item*>(items.front()->clone());
185-
trimmed_range = second_item->trimmed_range();
186-
source_range = TimeRange(
184+
auto cloned_item = items.front()->clone(error_status);
185+
if (!cloned_item)
186+
{
187+
// error_status was already set within clone()
188+
return;
189+
}
190+
auto second_item = dynamic_cast<Item*>(cloned_item);
191+
if (!second_item)
192+
{
193+
if (error_status)
194+
*error_status = ErrorStatus::TYPE_MISMATCH;
195+
return;
196+
}
197+
trimmed_range = second_item->trimmed_range();
198+
source_range = TimeRange(
187199
trimmed_range.start_time() + first_duration
188200
+ range.duration(),
189201
second_duration);
@@ -362,7 +374,19 @@ insert(
362374
// Clone the item for the second partially overwritten item.
363375
if (!isEqual(second_source_range.duration().value(), 0.0))
364376
{
365-
auto second_item = dynamic_cast<Item*>(item->clone());
377+
auto cloned_item = item->clone(error_status);
378+
if (!cloned_item)
379+
{
380+
// error_status was already set within clone()
381+
return;
382+
}
383+
auto second_item = dynamic_cast<Item*>(cloned_item);
384+
if (!second_item)
385+
{
386+
if (error_status)
387+
*error_status = ErrorStatus::TYPE_MISMATCH;
388+
return;
389+
}
366390
second_item->set_source_range(second_source_range);
367391
composition->insert_child(insert_index + 1, second_item);
368392
}
@@ -530,7 +554,19 @@ slice(
530554
item->set_source_range(first_source_range);
531555

532556
// Clone the item for the second slice.
533-
auto second_item = dynamic_cast<Item*>(item->clone());
557+
auto cloned_item = item->clone(error_status);
558+
if (!cloned_item)
559+
{
560+
// error_status was already set within clone()
561+
return;
562+
}
563+
auto second_item = dynamic_cast<Item*>(cloned_item);
564+
if (!second_item)
565+
{
566+
if (error_status)
567+
*error_status = ErrorStatus::TYPE_MISMATCH;
568+
return;
569+
}
534570
const TimeRange second_source_range(
535571
first_source_range.start_time() + first_source_range.duration(),
536572
range.duration() - first_source_range.duration());
@@ -793,7 +829,19 @@ fill(
793829
case ReferencePoint::Sequence: {
794830
RationalTime start_time = clip_range.start_time();
795831
const RationalTime gap_start_time = gap_range.start_time();
796-
auto track_item = dynamic_cast<Item*>(item->clone());
832+
auto cloned_item = item->clone(error_status);
833+
if (!cloned_item)
834+
{
835+
// error_status was already set within clone()
836+
return;
837+
}
838+
auto track_item = dynamic_cast<Item*>(cloned_item);
839+
if (!track_item)
840+
{
841+
if (error_status)
842+
*error_status = ErrorStatus::TYPE_MISMATCH;
843+
return;
844+
}
797845

798846
// Check if start time is less than gap's start time (trim it if so)
799847
if (start_time < gap_start_time)

‎tests/test_editAlgorithm.cpp‎

Lines changed: 120 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2131,6 +2131,126 @@ main(int argc, char** argv)
21312131
TimeRange(RationalTime(0.0, 24.0), RationalTime(10.0, 24.0)) });
21322132
});
21332133

2134+
tests.add_test("regression: slice fails gracefully", [] {
2135+
SerializableObject::Retainer<Clip> big_clip = new Clip(
2136+
"big clip",
2137+
nullptr,
2138+
TimeRange(RationalTime(0.0, 24.0), RationalTime(24.0, 24.0)));
2139+
big_clip->metadata()["cycle"] = big_clip;
2140+
2141+
SerializableObject::Retainer<Track> track = new Track();
2142+
track->append_child(big_clip);
2143+
2144+
OTIO_NS::ErrorStatus error_status;
2145+
2146+
algo::slice(track, RationalTime(12.0, 24.0), false, &error_status);
2147+
2148+
assert(is_error(error_status));
2149+
assert(error_status.outcome == OTIO_NS::ErrorStatus::TYPE_MISMATCH);
2150+
});
2151+
2152+
tests.add_test("regression: overwrite fails gracefully", [] {
2153+
SerializableObject::Retainer<Clip> big_clip = new Clip(
2154+
"big clip",
2155+
nullptr,
2156+
TimeRange(RationalTime(0.0, 24.0), RationalTime(24.0, 24.0)));
2157+
big_clip->metadata()["cycle"] = big_clip;
2158+
2159+
SerializableObject::Retainer<Clip> small_clip = new Clip(
2160+
"small clip",
2161+
nullptr,
2162+
TimeRange(RationalTime(0.0, 24.0), RationalTime(5.0, 24.0)));
2163+
small_clip->metadata()["cycle"] = small_clip;
2164+
2165+
SerializableObject::Retainer<Track> track = new Track();
2166+
track->append_child(big_clip);
2167+
2168+
OTIO_NS::ErrorStatus error_status;
2169+
2170+
algo::overwrite(
2171+
small_clip,
2172+
track,
2173+
TimeRange(RationalTime(0.0, 24.0), RationalTime(12.0, 24.0)),
2174+
true,
2175+
nullptr,
2176+
&error_status);
2177+
2178+
assert(is_error(error_status));
2179+
assert(error_status.outcome == OTIO_NS::ErrorStatus::TYPE_MISMATCH);
2180+
});
2181+
2182+
tests.add_test("regression: insert fails gracefully", [] {
2183+
SerializableObject::Retainer<Clip> big_clip = new Clip(
2184+
"big clip",
2185+
nullptr,
2186+
TimeRange(RationalTime(0.0, 24.0), RationalTime(24.0, 24.0)));
2187+
big_clip->metadata()["cycle"] = big_clip;
2188+
2189+
SerializableObject::Retainer<Clip> small_clip = new Clip(
2190+
"small clip",
2191+
nullptr,
2192+
TimeRange(RationalTime(0.0, 24.0), RationalTime(5.0, 24.0)));
2193+
small_clip->metadata()["cycle"] = small_clip;
2194+
2195+
SerializableObject::Retainer<Track> track = new Track();
2196+
track->append_child(big_clip);
2197+
2198+
OTIO_NS::ErrorStatus error_status;
2199+
2200+
algo::insert(
2201+
small_clip,
2202+
track,
2203+
RationalTime(12.0, 24.0),
2204+
true,
2205+
nullptr,
2206+
&error_status);
2207+
2208+
assert(is_error(error_status));
2209+
assert(error_status.outcome == OTIO_NS::ErrorStatus::TYPE_MISMATCH);
2210+
});
2211+
2212+
tests.add_test("regression: fill fails gracefully", [] {
2213+
SerializableObject::Retainer<Clip> big_clip = new Clip(
2214+
"big clip",
2215+
nullptr,
2216+
TimeRange(RationalTime(0.0, 24.0), RationalTime(24.0, 24.0)));
2217+
big_clip->metadata()["cycle"] = big_clip;
2218+
2219+
SerializableObject::Retainer<Clip> small_clip = new Clip(
2220+
"small clip",
2221+
nullptr,
2222+
TimeRange(RationalTime(0.0, 24.0), RationalTime(5.0, 24.0)));
2223+
small_clip->metadata()["cycle"] = small_clip;
2224+
2225+
SerializableObject::Retainer<Clip> small_clip2 = new Clip(
2226+
"small clip 2",
2227+
nullptr,
2228+
TimeRange(RationalTime(0.0, 24.0), RationalTime(5.0, 24.0)));
2229+
small_clip2->metadata()["cycle"] = small_clip2;
2230+
2231+
SerializableObject::Retainer<Gap> gap = new Gap(
2232+
TimeRange(RationalTime(0.0, 24.0), RationalTime(20.0, 24.0)),
2233+
"gap");
2234+
2235+
SerializableObject::Retainer<Track> track = new Track();
2236+
// two small clips with a gap between them
2237+
track->append_child(small_clip);
2238+
track->append_child(gap);
2239+
track->append_child(small_clip2);
2240+
2241+
OTIO_NS::ErrorStatus error_status;
2242+
2243+
algo::fill(
2244+
big_clip,
2245+
track,
2246+
RationalTime(12.0, 24.0),
2247+
ReferencePoint::Sequence,
2248+
&error_status);
2249+
2250+
assert(is_error(error_status));
2251+
assert(error_status.outcome == OTIO_NS::ErrorStatus::TYPE_MISMATCH);
2252+
});
2253+
21342254
tests.run(argc, argv);
21352255
return 0;
21362256
}

‎tests/test_serialization.cpp‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -110,6 +110,39 @@ main(int argc, char** argv)
110110
})CONTENT");
111111
});
112112

113+
tests.add_test("clone", [] {
114+
auto json = R"CONTENT({
115+
"OTIO_SCHEMA": "SerializableObjectWithMetadata.1",
116+
"metadata": {
117+
"a": 1,
118+
"b": "two",
119+
"c": [
120+
3,
121+
4,
122+
5
123+
],
124+
"d": {
125+
"hello": "nested"
126+
}
127+
},
128+
"name": ""
129+
})CONTENT";
130+
auto original = SerializableObjectWithMetadata::from_json_string(json);
131+
auto cloned = original->clone();
132+
assert(cloned != nullptr);
133+
OTIO_NS::ErrorStatus err;
134+
auto cloned_json = cloned->to_json_string(&err, {}, 2);
135+
assertFalse(is_error(err));
136+
assertEqual(json, cloned_json.c_str());
137+
});
138+
139+
tests.add_test("clone with cycle returns nullptr", [] {
140+
auto original = new SerializableObjectWithMetadata();
141+
original->metadata()["cycle"] = original;
142+
auto cloned = original->clone();
143+
assert(cloned == nullptr);
144+
});
145+
113146
tests.run(argc, argv);
114147
return 0;
115148
}

0 commit comments

Comments
 (0)