stream_curl: don't set initial stream pos to > 0 if it's not seekable - #18249
stream_curl: don't set initial stream pos to > 0 if it's not seekable#18249MoSal wants to merge 1 commit into
Conversation
Stream creation happens before calling `demux_open_url()` which calls `demux_open()` which goes through a demuxer list and calls `open_given_type()` until success. `open_given_type()` has an unconditional seek back to pos=0. This works with non-seekable streams because if we start with pos=0, these calls buffer the stream, and seeking back happens within the buffer. But if we start with pos > 0, a seek back will fail as we have no filled buffer yet. And to make matters worse, `open_given_type()` doesn't check if the seek happened/succeeded or not, and continues to parse with the assumption that it's at pos 0, Which leads to weird brokenness at best, and potential security issues at worst. Signed-off-by: Mohammad AlSaleh <CE.Mohammad.AlSaleh@gmail.com>
73633b1 to
d7bd3c6
Compare
|
Updated the commit message with correct info about the real cause of extra brokenness. The last part of the commit message is talking about the fact that the return value here is not checked: Line 3440 in e5486b9 This needs a separate fix where failure returns |
|
Yes. Should this potential security hole be proactively patched too? demux/demux.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/demux/demux.c b/demux/demux.c
index b63038b6c4..164fdaa239 100644
--- a/demux/demux.c
+++ b/demux/demux.c
@@ -3436,8 +3436,12 @@ static struct demuxer *open_given_type(struct mpv_global *global,
mp_dbg(log, "Trying demuxer: %s (force-level: %s)\n",
desc->name, d_level(check));
- if (stream)
- stream_seek(stream, 0);
+ if (stream && stream->pos != 0) {
+ if (!stream_seek(stream, 0)) {
+ mp_fatal(log, "Seek to start failed while trying to detect file format\n");
+ exit(1);
+ }
+ }
in->d_thread->params = params; // temporary during open()
int ret = demuxer->desc->open(in->d_thread, check);Random open stream that triggers the issue for me, if anyone wants to test: Correct behavior is opening as an HLS stream. Incorrect parsing in this case manifests as playing this as a list of MPEGTS streams. |
|
Superseded by: #18289 |
It shouldn't be an issue anymore though? I added commit to fail probing in this case early, but as I understand with stream_curl fix, we should have enough buffer to rewind it. There is possibility that we drop the demuxing buffer during probing and fail seeking back to 0, but not sure if this is possible in practice. May be improved in the future. |
Yes. I meant testing before and after your fix is applied. Apologies if that was not clear. I just mentioned it because you couldn't reproduce when I first mentioned the issue in IRC. |
2 participants
Stream creation happens before calling
demux_open_url()which callsdemux_open()which goes through a demuxer list and callsopen_given_type()until success.open_given_type()has an unconditional seek back to pos=0. This workswith non-seekable streams because if we start with pos=0, these calls
buffer the stream, and seeking back happens within the buffer.
But if we start with pos > 0, a seek back will fail as we have no
filled buffer yet. And to make matters worse,
open_given_type()doesn't check if the seek happened/succeeded or not, and continues to
parse with the assumption that it's at pos 0, Which leads to weird
brokenness at best, and potential security issues at worst.