| 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 | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
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,
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
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,
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".
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,
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".
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,
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 --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); }