GNOME Bugzilla – Bug 774846
asfdemux: Timeouts with corrupted files
Last modified: 2016-11-23 11:07:56 UTC
See description in patches
Created attachment 340525 [details] [review] asfdemux: Handle issues with "empty" files In some corrupted files, we could end up with no actual streams being exposed. In those cases, make sure we properly propagate the failure all the way to the loop function. This avoids ending up in cases where we are neither EOS'd nor ERROR'd out from a pipeline point of view.
Created attachment 340526 [details] [review] asfdemux: Handle incomplete header in pull mode pulling headers is meant to complete as a whole. If we don't have enough data, it's an error. Avoids pipeline hangs on corrupted files
Created attachment 340527 [details] [review] asfdemux: FILE headers are mandatory in the header As per the specification, also avoids ending up trying to play a file with plenty of un-initialized values.
Created attachment 340528 [details] [review] asfdemux: Handle EOS in push-mode on corrupted files It is possible no streams were activated when receiving EOS, if so handled it as if we hadn't seen the header
Review of attachment 340525 [details] [review]: ::: gst/asfdemux/gstasfdemux.c @@ +2086,3 @@ * send any pending payloads before sending EOS */ if (!demux->activated_streams) + flow = gst_asf_demux_push_complete_payloads (demux, TRUE); And someone is actually checking "flow"? @@ +2133,3 @@ /* For the error cases */ + if (flow == GST_FLOW_EOS && !demux->activated_streams) + GST_ELEMENT_FLOW_ERROR (demux, flow); We usually use WRONG_TYPE here
Review of attachment 340527 [details] [review]: If that header is missing, shouldn't something else somewhere already fail because information is missing? ::: gst/asfdemux/gstasfdemux.c @@ +3585,3 @@ } } + if (demux->saw_file_header == FALSE) { No boolean comparisons with ==
Review of attachment 340528 [details] [review]: ::: gst/asfdemux/gstasfdemux.c @@ +452,3 @@ + /* If we still haven't got activated streams, the file is most likely corrupt */ + GST_ELEMENT_ERROR (demux, STREAM, DEMUX, + (_("This stream contains no data.")), WRONG_TYPE
Review of attachment 340526 [details] [review]: How do we know the size of the complete header to pull, and then end up here with not enough data? A short read because end-of-file? Good to go in that case. Otherwise it seems like something is inconsistent in the code
(In reply to Sebastian Dröge (slomo) from comment #8) > Review of attachment 340526 [details] [review] [review]: > > How do we know the size of the complete header to pull, and then end up here > with not enough data? A short read because end-of-file? Good to go in that > case. Otherwise it seems like something is inconsistent in the code Because the header size it reports is invalid (like 99MB in a 2MB file)
Review of attachment 340525 [details] [review]: ::: gst/asfdemux/gstasfdemux.c @@ +2086,3 @@ * send any pending payloads before sending EOS */ if (!demux->activated_streams) + flow = gst_asf_demux_push_complete_payloads (demux, TRUE); Yes, it ends up in the pause: block below
(In reply to Edward Hervey from comment #9) > (In reply to Sebastian Dröge (slomo) from comment #8) > > Review of attachment 340526 [details] [review] [review] [review]: > > > > How do we know the size of the complete header to pull, and then end up here > > with not enough data? A short read because end-of-file? Good to go in that > > case. Otherwise it seems like something is inconsistent in the code > > Because the header size it reports is invalid (like 99MB in a 2MB file) And we pull 99MB but only get 2MB?
(In reply to Sebastian Dröge (slomo) from comment #11) > (In reply to Edward Hervey from comment #9) > > (In reply to Sebastian Dröge (slomo) from comment #8) > > > Review of attachment 340526 [details] [review] [review] [review] [review]: > > > > > > How do we know the size of the complete header to pull, and then end up here > > > with not enough data? A short read because end-of-file? Good to go in that > > > case. Otherwise it seems like something is inconsistent in the code > > > > Because the header size it reports is invalid (like 99MB in a 2MB file) > > And we pull 99MB but only get 2MB? Yes, so we end up with a custom need-more-data :)
Created attachment 340530 [details] [review] asfdemux: Handle issues with "empty" files In some corrupted files, we could end up with no actual streams being exposed. In those cases, make sure we properly propagate the failure all the way to the loop function. This avoids ending up in cases where we are neither EOS'd nor ERROR'd out from a pipeline point of view.
Created attachment 340531 [details] [review] asfdemux: Handle incomplete header in pull mode pulling headers is meant to complete as a whole. If we don't have enough data, it's an error. Avoids pipeline hangs on corrupted files
Created attachment 340532 [details] [review] asfdemux: FILE headers are mandatory in the header As per the specification, also avoids ending up trying to play a file with plenty of un-initialized values.
Created attachment 340533 [details] [review] asfdemux: Handle EOS in push-mode on corrupted files It is possible no streams were activated when receiving EOS, if so handled it as if we hadn't seen the header
Review of attachment 340530 [details] [review]: ::: gst/asfdemux/gstasfdemux.c @@ +2133,3 @@ /* For the error cases */ + if (flow == GST_FLOW_EOS && !demux->activated_streams) + GST_ELEMENT_FLOW_ERROR (demux, flow); WRONG_TYPE
Review of attachment 340533 [details] [review]: ::: gst/asfdemux/gstasfdemux.c @@ +451,3 @@ + if (!demux->activated_streams) { + /* If we still haven't got activated streams, the file is most likely corrupt */ + GST_ELEMENT_ERROR (demux, STREAM, DEMUX, WRONG_TYPE
Created attachment 340537 [details] [review] asfdemux: Handle issues with "empty" files In some corrupted files, we could end up with no actual streams being exposed. In those cases, make sure we properly propagate the failure all the way to the loop function. This avoids ending up in cases where we are neither EOS'd nor ERROR'd out from a pipeline point of view.
Created attachment 340538 [details] [review] asfdemux: Handle incomplete header in pull mode pulling headers is meant to complete as a whole. If we don't have enough data, it's an error. Avoids pipeline hangs on corrupted files
Created attachment 340539 [details] [review] asfdemux: FILE headers are mandatory in the header As per the specification, also avoids ending up trying to play a file with plenty of un-initialized values.
Created attachment 340540 [details] [review] asfdemux: Handle EOS in push-mode on corrupted files It is possible no streams were activated when receiving EOS, if so handled it as if we hadn't seen the header
commit 48a493070aec4b5426723a4a27f14eba107779cf Author: Edward Hervey <edward@centricular.com> Date: Tue Nov 22 17:14:44 2016 +0100 asfdemux: Handle EOS in push-mode on corrupted files It is possible no streams were activated when receiving EOS, if so handled it as if we hadn't seen the header https://bugzilla.gnome.org/show_bug.cgi?id=774846 commit e7ff86665a8e2c77a676546accf07f44b005f0dc Author: Edward Hervey <edward@centricular.com> Date: Tue Nov 22 16:56:04 2016 +0100 asfdemux: FILE headers are mandatory in the header As per the specification, also avoids ending up trying to play a file with plenty of un-initialized values. https://bugzilla.gnome.org/show_bug.cgi?id=774846 commit cc04255e946f28acb4a60cc570c0c460c2f0154f Author: Edward Hervey <edward@centricular.com> Date: Tue Nov 22 16:54:26 2016 +0100 asfdemux: Handle incomplete header in pull mode pulling headers is meant to complete as a whole. If we don't have enough data, it's an error. Avoids pipeline hangs on corrupted files https://bugzilla.gnome.org/show_bug.cgi?id=774846 commit 45c7826d76dfcb94cebbc57208c553d432fdc4a9 Author: Edward Hervey <edward@centricular.com> Date: Tue Nov 22 16:22:49 2016 +0100 asfdemux: Handle issues with "empty" files In some corrupted files, we could end up with no actual streams being exposed. In those cases, make sure we properly propagate the failure all the way to the loop function. This avoids ending up in cases where we are neither EOS'd nor ERROR'd out from a pipeline point of view. https://bugzilla.gnome.org/show_bug.cgi?id=774846