| Message ID | 1593272784-8031-1-git-send-email-lance.lmwang@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [FFmpeg-devel] avcodec/mpeg12dec: Fix uninitialized data in fate-sub-cc-scte20 | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
Am Sa., 27. Juni 2020 um 17:46 Uhr schrieb <lance.lmwang@gmail.com>: > > From: Limin Wang <lance.lmwang@gmail.com> > > The issue is introduced from a705bcd763e344fa, please tested with below command line: > make V=1 fate-sub-cc-scte20 TARGET_EXEC="valgrind --error-exitcode=1" > > Reported-by: Martin Storsjö <martin@martin.st> > Signed-off-by: Limin Wang <lance.lmwang@gmail.com> > --- > libavcodec/mpeg12dec.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c > index f0f92ac..2562027 100644 > --- a/libavcodec/mpeg12dec.c > +++ b/libavcodec/mpeg12dec.c > @@ -2276,6 +2276,8 @@ static int mpeg_decode_a53_cc(AVCodecContext *avctx, > if (ret >= 0) { > uint8_t field, cc1, cc2; > uint8_t *cap = s1->a53_buf_ref->data; > + > + memset(s1->a53_buf_ref->data + old_size, 0, cc_count * UINT64_C(3)); Why is the promotion (?) to uint64 useful? Carl Eugen
On 6/27/2020 12:46 PM, lance.lmwang@gmail.com wrote: > From: Limin Wang <lance.lmwang@gmail.com> > > The issue is introduced from a705bcd763e344fa, please tested with below command line: > make V=1 fate-sub-cc-scte20 TARGET_EXEC="valgrind --error-exitcode=1" > > Reported-by: Martin Storsjö <martin@martin.st> > Signed-off-by: Limin Wang <lance.lmwang@gmail.com> > --- > libavcodec/mpeg12dec.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c > index f0f92ac..2562027 100644 > --- a/libavcodec/mpeg12dec.c > +++ b/libavcodec/mpeg12dec.c > @@ -2276,6 +2276,8 @@ static int mpeg_decode_a53_cc(AVCodecContext *avctx, > if (ret >= 0) { > uint8_t field, cc1, cc2; > uint8_t *cap = s1->a53_buf_ref->data; > + > + memset(s1->a53_buf_ref->data + old_size, 0, cc_count * UINT64_C(3)); Why is zeroing needed now to prevent use of uninitialized values but not before this patch? Wouldn't it hint at some issue in your port to AVBufferRef? Did you for example make sure to read and write in the correct place in the reallocated buffer when you're appending new captions to it? > for (i = 0; i < cc_count && get_bits_left(&gb) >= 26; i++) { > skip_bits(&gb, 2); // priority > field = get_bits(&gb, 2); >
On Sat, Jun 27, 2020 at 05:54:52PM +0200, Carl Eugen Hoyos wrote: > Am Sa., 27. Juni 2020 um 17:46 Uhr schrieb <lance.lmwang@gmail.com>: > > > > From: Limin Wang <lance.lmwang@gmail.com> > > > > The issue is introduced from a705bcd763e344fa, please tested with below command line: > > make V=1 fate-sub-cc-scte20 TARGET_EXEC="valgrind --error-exitcode=1" > > > > Reported-by: Martin Storsjö <martin@martin.st> > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com> > > --- > > libavcodec/mpeg12dec.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c > > index f0f92ac..2562027 100644 > > --- a/libavcodec/mpeg12dec.c > > +++ b/libavcodec/mpeg12dec.c > > @@ -2276,6 +2276,8 @@ static int mpeg_decode_a53_cc(AVCodecContext *avctx, > > if (ret >= 0) { > > uint8_t field, cc1, cc2; > > uint8_t *cap = s1->a53_buf_ref->data; > > + > > > + memset(s1->a53_buf_ref->data + old_size, 0, cc_count * UINT64_C(3)); > > Why is the promotion (?) to uint64 useful? I can't tell what's the real reason as the original code is coming in 80ea66112817c719b476de8f7 for h264, HEVC and mpeg2 are following the same way. > > Carl Eugen > _______________________________________________ > 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 6/27/2020 7:56 PM, lance.lmwang@gmail.com wrote: > On Sat, Jun 27, 2020 at 05:54:52PM +0200, Carl Eugen Hoyos wrote: >> Am Sa., 27. Juni 2020 um 17:46 Uhr schrieb <lance.lmwang@gmail.com>: >>> >>> From: Limin Wang <lance.lmwang@gmail.com> >>> >>> The issue is introduced from a705bcd763e344fa, please tested with below command line: >>> make V=1 fate-sub-cc-scte20 TARGET_EXEC="valgrind --error-exitcode=1" >>> >>> Reported-by: Martin Storsjö <martin@martin.st> >>> Signed-off-by: Limin Wang <lance.lmwang@gmail.com> >>> --- >>> libavcodec/mpeg12dec.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c >>> index f0f92ac..2562027 100644 >>> --- a/libavcodec/mpeg12dec.c >>> +++ b/libavcodec/mpeg12dec.c >>> @@ -2276,6 +2276,8 @@ static int mpeg_decode_a53_cc(AVCodecContext *avctx, >>> if (ret >= 0) { >>> uint8_t field, cc1, cc2; >>> uint8_t *cap = s1->a53_buf_ref->data; >>> + >> >>> + memset(s1->a53_buf_ref->data + old_size, 0, cc_count * UINT64_C(3)); >> >> Why is the promotion (?) to uint64 useful? > > I can't tell what's the real reason as the original code is coming in 80ea66112817c719b476de8f7 > for h264, HEVC and mpeg2 are following the same way. The cast is done when combining the current captions with the existing ones. old_size + cc_count * 3 in h264_sei.c can overflow, hence casting to uint64_t, but cc_count * 3 alone can't, both here or in h264_sei.c > >> >> Carl Eugen >> _______________________________________________ >> 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 Sat, Jun 27, 2020 at 01:02:52PM -0300, James Almer wrote: > On 6/27/2020 12:46 PM, lance.lmwang@gmail.com wrote: > > From: Limin Wang <lance.lmwang@gmail.com> > > > > The issue is introduced from a705bcd763e344fa, please tested with below command line: > > make V=1 fate-sub-cc-scte20 TARGET_EXEC="valgrind --error-exitcode=1" > > > > Reported-by: Martin Storsjö <martin@martin.st> > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com> > > --- > > libavcodec/mpeg12dec.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c > > index f0f92ac..2562027 100644 > > --- a/libavcodec/mpeg12dec.c > > +++ b/libavcodec/mpeg12dec.c > > @@ -2276,6 +2276,8 @@ static int mpeg_decode_a53_cc(AVCodecContext *avctx, > > if (ret >= 0) { > > uint8_t field, cc1, cc2; > > uint8_t *cap = s1->a53_buf_ref->data; > > + > > + memset(s1->a53_buf_ref->data + old_size, 0, cc_count * UINT64_C(3)); > > Why is zeroing needed now to prevent use of uninitialized values but not > before this patch? Wouldn't it hint at some issue in your port to > AVBufferRef? The old code use mallocz to memeset the allocate data. When switch to av_buffer_realloc, then memset is missing. Or the data is uninitialized if the following get_bits_left(&gb) checking is true. I didn't notice it for real testing is OK without trigger it. > > Did you for example make sure to read and write in the correct place in > the reallocated buffer when you're appending new captions to it? I think the fate testing have tested the data. Also I have tested with 2 CC mpeg2 sample case. > > > for (i = 0; i < cc_count && get_bits_left(&gb) >= 26; i++) { > > skip_bits(&gb, 2); // priority > > field = get_bits(&gb, 2); > > > > _______________________________________________ > 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 Sat, Jun 27, 2020 at 08:01:49PM -0300, James Almer wrote: > On 6/27/2020 7:56 PM, lance.lmwang@gmail.com wrote: > > On Sat, Jun 27, 2020 at 05:54:52PM +0200, Carl Eugen Hoyos wrote: > >> Am Sa., 27. Juni 2020 um 17:46 Uhr schrieb <lance.lmwang@gmail.com>: > >>> > >>> From: Limin Wang <lance.lmwang@gmail.com> > >>> > >>> The issue is introduced from a705bcd763e344fa, please tested with below command line: > >>> make V=1 fate-sub-cc-scte20 TARGET_EXEC="valgrind --error-exitcode=1" > >>> > >>> Reported-by: Martin Storsjö <martin@martin.st> > >>> Signed-off-by: Limin Wang <lance.lmwang@gmail.com> > >>> --- > >>> libavcodec/mpeg12dec.c | 2 ++ > >>> 1 file changed, 2 insertions(+) > >>> > >>> diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c > >>> index f0f92ac..2562027 100644 > >>> --- a/libavcodec/mpeg12dec.c > >>> +++ b/libavcodec/mpeg12dec.c > >>> @@ -2276,6 +2276,8 @@ static int mpeg_decode_a53_cc(AVCodecContext *avctx, > >>> if (ret >= 0) { > >>> uint8_t field, cc1, cc2; > >>> uint8_t *cap = s1->a53_buf_ref->data; > >>> + > >> > >>> + memset(s1->a53_buf_ref->data + old_size, 0, cc_count * UINT64_C(3)); > >> > >> Why is the promotion (?) to uint64 useful? > > > > I can't tell what's the real reason as the original code is coming in 80ea66112817c719b476de8f7 > > for h264, HEVC and mpeg2 are following the same way. > > The cast is done when combining the current captions with the existing > ones. old_size + cc_count * 3 in h264_sei.c can overflow, hence casting > to uint64_t, but cc_count * 3 alone can't, both here or in h264_sei.c Got it, I'll update to use cc_count * 3 here. > > > > >> > >> Carl Eugen > >> _______________________________________________ > >> 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/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c index f0f92ac..2562027 100644 --- a/libavcodec/mpeg12dec.c +++ b/libavcodec/mpeg12dec.c @@ -2276,6 +2276,8 @@ static int mpeg_decode_a53_cc(AVCodecContext *avctx, if (ret >= 0) { uint8_t field, cc1, cc2; uint8_t *cap = s1->a53_buf_ref->data; + + memset(s1->a53_buf_ref->data + old_size, 0, cc_count * UINT64_C(3)); for (i = 0; i < cc_count && get_bits_left(&gb) >= 26; i++) { skip_bits(&gb, 2); // priority field = get_bits(&gb, 2);