GNOME Bugzilla – Bug 652642
typefind: NULL check in degas_type_find
Last modified: 2011-06-28 09:31:01 UTC
I got a segfault in degas_type_find, with this backtrace:
+ Trace 227489
Created attachment 189979 [details] [review] NULL check result of gst_type_find_peek
I wonder how that happens. Something else seems wrong here, ie if it does: len = gst_type_find_get_length (tf); if (len < 34) return; then peek(0, 4) must succeed, or the length reporting is broken. Any idea what's going on? Can you reproduce the issue? The NULL checks aren't wrong of course, but we shouldn't work around bugs elsewhere IMHO.
I thought that was a bit odd too, but looking at the documentation of gst_type_find_get_length it claims to be "the length of the data stream", not that the data is actually available. Is the documentation incorrect? In the case where it crashed, _get_length had returned something suspiciously close to 2^16, but since I couldn't see any 16-bit integers involved I shrugged that off. I can try reverting my fix and have a closer look inside the typefinder object when it happens again.
I could reproduce this yesterday twice, but it is very racy (since probably one has to have loaded only 1-3 bytes for this to happen) and I'm not having any luck today. Can someone confirm if gst_type_find_get_length is supposed to guarantee that gst_type_find_peek cannot fail?
OK, so it happened again, but with another input: 2samples.wav attached to https://bugzilla.gnome.org/show_bug.cgi?id=652562 What's happening here is that our custom source is fails with GST_FLOW_UNEXPECTED in _create because the pipeline needs to be destroyed. This happens when a user leaves a page with a <video> element and is necessary, at that point we can't provide the data and need to be able to fail in _create. Perhaps few other sources can fail in _create like this, but _create does return GstFlowReturn, so reasonably no code should assume that it succeeds. Reopening, as it would seem that this is either a bug in typefind or a bug in the documentation of basesrc _create to say that it must always succeed.
It's quite clear from <https://bugzilla.gnome.org/show_bug.cgi?id=652562> and <https://bugzilla.gnome.org/show_bug.cgi?id=617733> that _create shouldn't return less data than expected in the case of GST_FLOW_OK. However, I think that my patch is correct, because the current code assumes that the source cannot fail, finally. Clearly, it's possible for a network source to fail and just not be able to deliver more data.
I think the same is assumed in many other typefinders too btw. Your patch is correct though but do you want to review/patch the other typefinders too? :)
> I think the same is assumed in many other typefinders too btw. Didn't see any others. Pushed, thanks for the patch: commit f3e65f1c9374df13e4fa082bc750ce94e38c58de Author: Philip Jägenstedt <philipj@opera.com> Date: Wed Jun 15 13:51:31 2011 +0200 typefind: NULL check in degas_type_find The length check isn't sufficient, an source might report the correct length, but then still fail to read the requested number of bytes for some reason. https://bugzilla.gnome.org/show_bug.cgi?id=652642
(In reply to comment #7) > I think the same is assumed in many other typefinders too btw. Your patch is > correct though but do you want to review/patch the other typefinders too? :) I did look through the code when writing the patch looking for other offenders, but like you, I didn't find anything obvious.