| Message ID | 20190408121427.196572-1-jdorfman@google.com |
|---|---|
| State | Accepted |
| Commit | bb5efd1727eeecc9be8f1402810c7ab72344eed3 |
| Headers | show |
On 4/8/2019 9:14 AM, Jeremy Dorfman via ffmpeg-devel wrote: > Otherwise, AV1 encodes with FFmpeg trigger use-of-uninitialized-value > warnings under MemorySanitizer, and the output buffer potentially > changes from run to run. > --- > libavformat/av1.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/libavformat/av1.c b/libavformat/av1.c > index a0aad436a6..5fde8df97e 100644 > --- a/libavformat/av1.c > +++ b/libavformat/av1.c > @@ -372,6 +372,7 @@ int ff_isom_write_av1c(AVIOContext *pb, const uint8_t *buf, int size) > put_bits(&pbc, 1, seq_params.chroma_subsampling_x); > put_bits(&pbc, 1, seq_params.chroma_subsampling_y); > put_bits(&pbc, 2, seq_params.chroma_sample_position); > + put_bits(&pbc, 8, 0); // padding Shouldn't flush_put_bits() below do this? The doxy says "Pad the end of the output stream with zeros". In any case, i just tried remuxing a single file several times, and in all cases the hash of the output file was the same, so it could be a false positive from MemorySanitizer. Patch is fine either way, but i'd like to know why you're getting those warnings. > flush_put_bits(&pbc); > > avio_write(pb, header, sizeof(header)); >
Judging by the code, I think flush_put_bits only pads out to a byte boundary rather than the end of the buffer. By my read it will run the body of the while loop 3 times in this case, for bit_left = 8, 16, 24, so only 3 bytes are filled in header. I'd expect this to only affect the actual output in uncommon cases. It requires the reuse of a stack with non-zero values at the location of uint8_t header[4]. In the common case of running FFmpeg, it looks like this is called from the main thread so it should typically be zeroed by the OS. MemorySanitizer is tagging that region of the stack as uninitialized, so I don't think this is a false positive -- it doesn't appear to ever be intentionally written. Thanks, -Jeremy On Mon, Apr 8, 2019 at 9:33 AM James Almer <jamrial@gmail.com> wrote: > On 4/8/2019 9:14 AM, Jeremy Dorfman via ffmpeg-devel wrote: > > Otherwise, AV1 encodes with FFmpeg trigger use-of-uninitialized-value > > warnings under MemorySanitizer, and the output buffer potentially > > changes from run to run. > > --- > > libavformat/av1.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/libavformat/av1.c b/libavformat/av1.c > > index a0aad436a6..5fde8df97e 100644 > > --- a/libavformat/av1.c > > +++ b/libavformat/av1.c > > @@ -372,6 +372,7 @@ int ff_isom_write_av1c(AVIOContext *pb, const > uint8_t *buf, int size) > > put_bits(&pbc, 1, seq_params.chroma_subsampling_x); > > put_bits(&pbc, 1, seq_params.chroma_subsampling_y); > > put_bits(&pbc, 2, seq_params.chroma_sample_position); > > + put_bits(&pbc, 8, 0); // padding > > Shouldn't flush_put_bits() below do this? The doxy says "Pad the end of > the output stream with zeros". > In any case, i just tried remuxing a single file several times, and in > all cases the hash of the output file was the same, so it could be a > false positive from MemorySanitizer. > > Patch is fine either way, but i'd like to know why you're getting those > warnings. > > > flush_put_bits(&pbc); > > > > avio_write(pb, header, sizeof(header)); > > > > _______________________________________________ > 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".
On 4/8/2019 10:53 AM, Jeremy Dorfman via ffmpeg-devel wrote: > Judging by the code, I think flush_put_bits only pads out to a byte > boundary rather than the end of the buffer. By my read it will run the body > of the while loop 3 times in this case, for bit_left = 8, 16, 24, so only 3 > bytes are filled in header. > > I'd expect this to only affect the actual output in uncommon cases. It > requires the reuse of a stack with non-zero values at the location of > uint8_t header[4]. In the common case of running FFmpeg, it looks like this > is called from the main thread so it should typically be zeroed by the OS. > MemorySanitizer is tagging that region of the stack as uninitialized, so I > don't think this is a false positive -- it doesn't appear to ever be > intentionally written. > > Thanks, > -Jeremy Alright, thanks for the explanation. Pushed. > > On Mon, Apr 8, 2019 at 9:33 AM James Almer <jamrial@gmail.com> wrote: > >> On 4/8/2019 9:14 AM, Jeremy Dorfman via ffmpeg-devel wrote: >>> Otherwise, AV1 encodes with FFmpeg trigger use-of-uninitialized-value >>> warnings under MemorySanitizer, and the output buffer potentially >>> changes from run to run. >>> --- >>> libavformat/av1.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/libavformat/av1.c b/libavformat/av1.c >>> index a0aad436a6..5fde8df97e 100644 >>> --- a/libavformat/av1.c >>> +++ b/libavformat/av1.c >>> @@ -372,6 +372,7 @@ int ff_isom_write_av1c(AVIOContext *pb, const >> uint8_t *buf, int size) >>> put_bits(&pbc, 1, seq_params.chroma_subsampling_x); >>> put_bits(&pbc, 1, seq_params.chroma_subsampling_y); >>> put_bits(&pbc, 2, seq_params.chroma_sample_position); >>> + put_bits(&pbc, 8, 0); // padding >> >> Shouldn't flush_put_bits() below do this? The doxy says "Pad the end of >> the output stream with zeros". >> In any case, i just tried remuxing a single file several times, and in >> all cases the hash of the output file was the same, so it could be a >> false positive from MemorySanitizer. >> >> Patch is fine either way, but i'd like to know why you're getting those >> warnings. >> >>> flush_put_bits(&pbc); >>> >>> avio_write(pb, header, sizeof(header)); >>> >> >> _______________________________________________ >> 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". > _______________________________________________ > 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/av1.c b/libavformat/av1.c index a0aad436a6..5fde8df97e 100644 --- a/libavformat/av1.c +++ b/libavformat/av1.c @@ -372,6 +372,7 @@ int ff_isom_write_av1c(AVIOContext *pb, const uint8_t *buf, int size) put_bits(&pbc, 1, seq_params.chroma_subsampling_x); put_bits(&pbc, 1, seq_params.chroma_subsampling_y); put_bits(&pbc, 2, seq_params.chroma_sample_position); + put_bits(&pbc, 8, 0); // padding flush_put_bits(&pbc); avio_write(pb, header, sizeof(header));