Fix null-pointer crash when a STREAMING object isn't a manifest - #67
jordijoangimenez wants to merge 1 commit into
Conversation
ManifestHandlerFactory::makeManifestHandler() correctly returns nullptr for non-manifest media types (e.g. audio/mp4, video/mp4 media segments), but ObjectStreamingController stored that result without checking it, crashing on the first subsequent manifestHandler()->... call. Add the same null-check ObjectCarouselController already has for this exact case: log and forward the object to the packager instead of trying to track it as a manifest.
dsilhavy
left a comment
There was a problem hiding this comment.
Thanks @jordijoangimenez , one finding by Fable that I consider worth checking:
2. Object-store retention leak: By the time this branch runs, check_if_object_added_is_manifest() has already called metadata.keepAfterSend(true) on the object. Since ObjectController::processEvent() only deletes objects after send when keepAfterSend() is false, every non-manifest object taking this path is kept in the ObjectStore forever — unbounded growth in a long-running STREAMING session (exactly the scenario in your test plan). Suggest resetting keepAfterSend(false) in the new branch.
davidjwbbc
left a comment
There was a problem hiding this comment.
Although this goes some of the way to handling a bad manifest, the other parts of the code should be changed to handle the manifest handler being a nullptr, instead of assuming that it will always be set.
The ObjectCarouselController throws a std::runtime_error if the manifest handler cannot be made, rather than just reporting the error. We'd have to check if throwing the exception here is a better solution (may avoid other calls to the manifest handler and deal with the error more gracefully).
@dsilhavy has a point about an unknown manifest hanging around, there should probably be a object->second.keepAfterSend(false); in there somewhere (and maybe in ObjectCarouselController.cc too).
We need a PR that addresses both issues including #66. |
|
Replaced by issue #70. |
Summary
ManifestHandlerFactory::makeManifestHandler()correctly returnsnullptrfor non-manifest media types (e.g.audio/mp4,video/mp4media segments), butObjectStreamingController(used forSTREAMINGoperating mode) stored that result without checking it, crashing on the first subsequentmanifestHandler()->...call.Adds the same null-check
ObjectCarouselController(used forCAROUSELmode) already has for this exact case: log and forward the object to the packager instead of trying to track it as a manifest.Test plan