diff mbox series

[FFmpeg-devel,1/5] avformat/dashenc: fix invalid pointer access if avio_get_dyn_buf failed

Message ID 1588173257-14531-1-git-send-email-lance.lmwang@gmail.com
State New
Headers show
Series [FFmpeg-devel,1/5] avformat/dashenc: fix invalid pointer access if avio_get_dyn_buf failed
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Limin Wang April 29, 2020, 3:14 p.m. UTC
From: Limin Wang <lance.lmwang@gmail.com>

If an error occurs, avio_get_dyn_buf() will return 0 and buf is NULL, so it's necessary to check
the return value for the following code will access the buf pointer with index. In addition,
the buf len should be greater than written_len to avoid the buffer overflow access.

Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
---
 libavformat/dashenc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Nicolas George April 29, 2020, 3:18 p.m. UTC | #1
lance.lmwang@gmail.com (12020-04-29):
> From: Limin Wang <lance.lmwang@gmail.com>
> 
> If an error occurs, avio_get_dyn_buf() will return 0 and buf is NULL, so it's necessary to check
> the return value for the following code will access the buf pointer with index. In addition,
> the buf len should be greater than written_len to avoid the buffer overflow access.
> 
> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> ---
>  libavformat/dashenc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

And if the allocation fails, the data is silently discarded. Seems
broken. Did you test your change?

Regards,
Limin Wang April 29, 2020, 3:27 p.m. UTC | #2
On Wed, Apr 29, 2020 at 05:18:18PM +0200, Nicolas George wrote:
> lance.lmwang@gmail.com (12020-04-29):
> > From: Limin Wang <lance.lmwang@gmail.com>
> > 
> > If an error occurs, avio_get_dyn_buf() will return 0 and buf is NULL, so it's necessary to check
> > the return value for the following code will access the buf pointer with index. In addition,
> > the buf len should be greater than written_len to avoid the buffer overflow access.
> > 
> > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > ---
> >  libavformat/dashenc.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> And if the allocation fails, the data is silently discarded. Seems
> broken. Did you test your change?

yes, avio_write can process zero len with NULL pointer, but here it'll use buf+written_len, so
it's invalid access I think. So what's the broken? Maybe I haven't catch your point.

> 
> Regards,
> 
> -- 
>   Nicolas George
Nicolas George April 29, 2020, 3:39 p.m. UTC | #3
Limin Wang (12020-04-29):
> yes, avio_write can process zero len with NULL pointer, but here it'll use buf+written_len, so
> it's invalid access I think. So what's the broken? Maybe I haven't catch your point.

What's broken is that the code is supposed to do something, and with
your change it does not do it.

These changes are therefore not acceptable.

The invalid access need to be fixed, but they need to be fixed properly:
since they correspond to a memory allocation failure, there should be
some kind of AVERROR(ENOMEM) in there.

Regards,
Limin Wang April 29, 2020, 3:47 p.m. UTC | #4
On Wed, Apr 29, 2020 at 05:39:36PM +0200, Nicolas George wrote:
> Limin Wang (12020-04-29):
> > yes, avio_write can process zero len with NULL pointer, but here it'll use buf+written_len, so
> > it's invalid access I think. So what's the broken? Maybe I haven't catch your point.
> 
> What's broken is that the code is supposed to do something, and with
> your change it does not do it.
> 
> These changes are therefore not acceptable.
> 
> The invalid access need to be fixed, but they need to be fixed properly:
> since they correspond to a memory allocation failure, there should be
> some kind of AVERROR(ENOMEM) in there.

Thanks, I catch your point now. Most of existing code haven't return ERROR, so I
choose the same way to process it. If you think it's not OK, we'll change more
code I think . Also some code weren't check the result, but for the len is 0 and
don't broken anything. But yes, the data isn't expected.

> 
> Regards,
> 
> -- 
>   Nicolas George



> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Nicolas George April 29, 2020, 4:55 p.m. UTC | #5
lance.lmwang@gmail.com (12020-04-29):
> Thanks, I catch your point now. Most of existing code haven't return ERROR, so I
> choose the same way to process it. If you think it's not OK, we'll change more
> code I think.

Maybe the other code needs to be fixed the same way. Maybe the other
code needs not return an error and this one does. Maybe your change is
actually valid.

You cannot know unless you first understand what the code does and what
it is supposed to do. You cannot program by imitation, it does not work.

And you have to test your changes: run a ffmpeg command line, make sure
the new code is triggered, and check the output file, compare it with
when the change is not triggered. If they are both correct, good. If one
is corrupted, then you know your change was bogus. And if you did not
test, then your change cannot be accepted.

Ideally we can trust regular contributors to have tested their changes
and run FATE before submitting patches.

Regards,
Limin Wang April 30, 2020, 1:17 a.m. UTC | #6
On Wed, Apr 29, 2020 at 06:55:33PM +0200, Nicolas George wrote:
> lance.lmwang@gmail.com (12020-04-29):
> > Thanks, I catch your point now. Most of existing code haven't return ERROR, so I
> > choose the same way to process it. If you think it's not OK, we'll change more
> > code I think.
> 
> Maybe the other code needs to be fixed the same way. Maybe the other
> code needs not return an error and this one does. Maybe your change is
> actually valid.
> 
> You cannot know unless you first understand what the code does and what
> it is supposed to do. You cannot program by imitation, it does not work.
> 
> And you have to test your changes: run a ffmpeg command line, make sure
> the new code is triggered, and check the output file, compare it with
> when the change is not triggered. If they are both correct, good. If one
> is corrupted, then you know your change was bogus. And if you did not
> test, then your change cannot be accepted.

Sorry, the old code will segment fault but the new code will not if error
happened. so I have no idea what's to compare the output as it's error.
Also all of the code which are using avio_get_dyn_buf() didn't check the size
for error. I had to say the avio_get_dyn_buf() is designed very well.

Can we change avio_get_dyn_buf() to return the error directly? also the 
description of API isn't clear for the error condition.


> 
> Ideally we can trust regular contributors to have tested their changes
> and run FATE before submitting patches.
> 
> Regards,
> 
> -- 
>   Nicolas George



> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Nicolas George April 30, 2020, 10:34 a.m. UTC | #7
lance.lmwang@gmail.com (12020-04-30):
> Sorry, the old code will segment fault but the new code will not if error
> happened. so I have no idea what's to compare the output as it's error.

A segfault is better than corrupted data and better than lost data. So
your task is to examine the output file and check whether it is
corrupted and whether it contains all the data it should contain.

> Also all of the code which are using avio_get_dyn_buf() didn't check the size
> for error. I had to say the avio_get_dyn_buf() is designed very well.
> 
> Can we change avio_get_dyn_buf() to return the error directly? also the 
> description of API isn't clear for the error condition.

It seems avio_get_dyn_buf() is not a very good API, indeed. I do not
know if it can be fixed. It is public, and therefore hard to change.

I have been working on a better API for this kind of thing. Can I count
on your support when I propose it again?

Regards,
Limin Wang April 30, 2020, 12:52 p.m. UTC | #8
On Thu, Apr 30, 2020 at 12:34:02PM +0200, Nicolas George wrote:
> lance.lmwang@gmail.com (12020-04-30):
> > Sorry, the old code will segment fault but the new code will not if error
> > happened. so I have no idea what's to compare the output as it's error.
> 
> A segfault is better than corrupted data and better than lost data. So
> your task is to examine the output file and check whether it is
> corrupted and whether it contains all the data it should contain.
> 
> > Also all of the code which are using avio_get_dyn_buf() didn't check the size
> > for error. I had to say the avio_get_dyn_buf() is designed very well.
> > 
> > Can we change avio_get_dyn_buf() to return the error directly? also the 
> > description of API isn't clear for the error condition.
> 
> It seems avio_get_dyn_buf() is not a very good API, indeed. I do not
> know if it can be fixed. It is public, and therefore hard to change.
> 
> I have been working on a better API for this kind of thing. Can I count
> on your support when I propose it again?

That's good to hear, please count me.

> 
> Regards,
> 
> -- 
>   Nicolas George



> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
diff mbox series

Patch

diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c
index 9f83785792..99fb7d67af 100644
--- a/libavformat/dashenc.c
+++ b/libavformat/dashenc.c
@@ -2260,7 +2260,7 @@  static int dash_write_packet(AVFormatContext *s, AVPacket *pkt)
         uint8_t *buf = NULL;
         avio_flush(os->ctx->pb);
         len = avio_get_dyn_buf (os->ctx->pb, &buf);
-        if (os->out) {
+        if (os->out && len > os->written_len) {
             avio_write(os->out, buf + os->written_len, len - os->written_len);
             avio_flush(os->out);
         }