GNOME Bugzilla – Bug 623104
[queue2] might create short buffer in get_range
Last modified: 2010-06-29 12:11:35 UTC
Demuxers and other elements operating in pull mode usually expect to get all the bytes they requested in pull_range(). pull_range() may return FLOW_OK + fewer bytes only when the end of the file has been reached and there are not enough bytes available. This means most demuxers will (correctly, I think) assume EOS if pull_range() delivers fewer bytes than requested. The following code looks like it could lead to a short buffer being returned in the middle of a stream: GstFlowReturn gst_queue2_create_read (GstQueue2 * queue, guint64 offset, guint length, GstBuffer ** buffer) { ... GST_LOG_OBJECT (queue, "Reading %d bytes", length); res = fread (GST_BUFFER_DATA (buf), 1, length, queue->temp_file); GST_LOG_OBJECT (queue, "read %" G_GSIZE_FORMAT " bytes", res); if (G_UNLIKELY (res == 0)) { /* check for errors or EOF */ .... } length = res; GST_BUFFER_SIZE (buf) = length; GST_BUFFER_OFFSET (buf) = offset; GST_BUFFER_OFFSET_END (buf) = offset + length; ... } Since it's called from _get_range() as well, this seems undesirable.
And we can't just switch the fread (1, length) to fread (length, 1) here because we still want to be able to read the remaining bytes at EOS if there are less than length bytes. So maybe what's needed is a switch to fread (length, 1) preceded by some if (queue->eos) length = MIN (length, remaining_bytes); logic.
I don't think that's true. gst_queue2_create_read() first calls gst_queue2_have_data(), which checks if there's a chunk with the requested offset and length. If there is the fread() must succeed, if queue2 is EOS fread() will return the remaining number of bytes. But a) the fseeks shouldn't be done if offset is after the EOS position and b) there should be an assertion below fread() to check that the read length is equal to the requested length unless queue2 is EOS.
> If there is [a chunk with the requested offset and length] > the fread() must succeed I was under the impression that this is not guaranteed, but maybe that problem only exists with read(). I've seen the fread man page, but I'm not sure how much I want to trust it, as I distinctly remember having seen/heard of this problem with plain fread() as well (ie. I remember the argument swap to ensure all-or-nothing reads).
Can't find anything to back that up though -> closing.