Re: [FFmpeg-devel] [PATCH 1/5] avformat/dashenc: fix invalid pointer access if avio_get_dyn_buf failed
On Thu, Apr 30, 2020 at 12:34:02PM +0200, Nicolas George wrote: > lance.lmw...@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-requ...@ffmpeg.org with subject "unsubscribe". -- Thanks, Limin Wang ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/5] avformat/dashenc: fix invalid pointer access if avio_get_dyn_buf failed
lance.lmw...@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, -- Nicolas George signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/5] avformat/dashenc: fix invalid pointer access if avio_get_dyn_buf failed
On Wed, Apr 29, 2020 at 06:55:33PM +0200, Nicolas George wrote: > lance.lmw...@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-requ...@ffmpeg.org with subject "unsubscribe". -- Thanks, Limin Wang ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/5] avformat/dashenc: fix invalid pointer access if avio_get_dyn_buf failed
lance.lmw...@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, -- Nicolas George signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/5] avformat/dashenc: fix invalid pointer access if avio_get_dyn_buf failed
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-requ...@ffmpeg.org with subject "unsubscribe". -- Thanks, Limin Wang ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/5] avformat/dashenc: fix invalid pointer access if avio_get_dyn_buf failed
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, -- Nicolas George signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/5] avformat/dashenc: fix invalid pointer access if avio_get_dyn_buf failed
On Wed, Apr 29, 2020 at 05:18:18PM +0200, Nicolas George wrote: > lance.lmw...@gmail.com (12020-04-29): > > From: Limin Wang > > > > 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 > > --- > > 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 -- Thanks, Limin Wang ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/5] avformat/dashenc: fix invalid pointer access if avio_get_dyn_buf failed
lance.lmw...@gmail.com (12020-04-29): > From: Limin Wang > > 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 > --- > 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, -- Nicolas George signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH 1/5] avformat/dashenc: fix invalid pointer access if avio_get_dyn_buf failed
From: Limin Wang
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
---
libavformat/dashenc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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, );
-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);
}
--
2.21.0
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
