diff mbox series

[FFmpeg-devel,v2] lavc/hevc_ps: fix process failed when SPS before VPS in hvcC

Message ID tencent_0A34924B55FC1F859EA08CE25C55F99C8609@qq.com
State New
Headers show
Series [FFmpeg-devel,v2] lavc/hevc_ps: fix process failed when SPS before VPS in hvcC | expand

Checks

Context Check Description
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 success Make fate finished
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

wangyaqiang Sept. 26, 2022, 9:38 a.m. UTC 
From: Wang Yaqiang <wangyaqiang03@kuaishou.com>

In some videos, SPS will be stored before VPS in hvcC box,
parse SPS does not depend on VPS, so the video is expected to be processed normally.
Added "parsed_vps" parameter to indicate whether VPS have been parsed.
Only VPS have been parsed can be verified during SPS parsing.

Signed-off-by: Wang Yaqiang <wangyaqiang03@kuaishou.com>
---
 libavcodec/hevc_ps.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Comments

Michael Niedermayer Sept. 26, 2022, 8:21 p.m. UTC | #1 
On Mon, Sep 26, 2022 at 05:38:14PM +0800, 1035567130@qq.com wrote:
> From: Wang Yaqiang <wangyaqiang03@kuaishou.com>
> 
> In some videos, SPS will be stored before VPS in hvcC box,
> parse SPS does not depend on VPS, so the video is expected to be processed normally.
> Added "parsed_vps" parameter to indicate whether VPS have been parsed.
> Only VPS have been parsed can be verified during SPS parsing.
> 
> Signed-off-by: Wang Yaqiang <wangyaqiang03@kuaishou.com>
> ---
>  libavcodec/hevc_ps.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

This causes segfaults

==816== Invalid read of size 8
==816==    at 0xFAF178: hevc_parse (in ffmpeg_g)
==816==    by 0xA7A2A6: av_parser_parse2 (in ffmpeg_g)
==816==    by 0x5FC388: parse_packet (in ffmpeg_g)
==816==    by 0x5FDC5D: read_frame_internal (in ffmpeg_g)
==816==    by 0x5FFA10: avformat_find_stream_info (in ffmpeg_g)
==816==    by 0x2F6054: open_input_file (in ffmpeg_g)
==816==    by 0x2FC6AB: ffmpeg_parse_options (in ffmpeg_g)
==816==    by 0x2E8A34: main (in ffmpeg_g)
==816==  Address 0x8 is not stack'd, malloc'd or (recently) free'd


[...]
wangyaqiang Sept. 27, 2022, 12:20 p.m. UTC | #2 
> 2022年9月27日 04:21，Michael Niedermayer <michael@niedermayer.cc> 写道：
> 
> On Mon, Sep 26, 2022 at 05:38:14PM +0800, 1035567130@qq.com wrote:
>> From: Wang Yaqiang <wangyaqiang03@kuaishou.com>
>> 
>> In some videos, SPS will be stored before VPS in hvcC box,
>> parse SPS does not depend on VPS, so the video is expected to be processed normally.
>> Added "parsed_vps" parameter to indicate whether VPS have been parsed.
>> Only VPS have been parsed can be verified during SPS parsing.
>> 
>> Signed-off-by: Wang Yaqiang <wangyaqiang03@kuaishou.com>
>> ---
>> libavcodec/hevc_ps.c | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
> 
> This causes segfaults
> 
> ==816== Invalid read of size 8
> ==816==    at 0xFAF178: hevc_parse (in ffmpeg_g)
> ==816==    by 0xA7A2A6: av_parser_parse2 (in ffmpeg_g)
> ==816==    by 0x5FC388: parse_packet (in ffmpeg_g)
> ==816==    by 0x5FDC5D: read_frame_internal (in ffmpeg_g)
> ==816==    by 0x5FFA10: avformat_find_stream_info (in ffmpeg_g)
> ==816==    by 0x2F6054: open_input_file (in ffmpeg_g)
> ==816==    by 0x2FC6AB: ffmpeg_parse_options (in ffmpeg_g)
> ==816==    by 0x2E8A34: main (in ffmpeg_g)
> ==816==  Address 0x8 is not stack'd, malloc'd or (recently) free'd
> 
> 
> [...]
> 

This is a serious problem, but I haven't occur it. Could you please provide with test materials or test methods? Thanks

> -- 
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> 
> When the tyrant has disposed of foreign enemies by conquest or treaty, and
> there is nothing more to fear from them, then he is always stirring up
> some war or other, in order that the people may require a leader. -- Plato
> _______________________________________________
> 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".
wangyaqiang Oct. 14, 2022, 10:13 a.m. UTC | #3 
> 2022年9月27日 04:21，Michael Niedermayer <michael@niedermayer.cc> 写道：
> 
> On Mon, Sep 26, 2022 at 05:38:14PM +0800, 1035567130@qq.com wrote:
>> From: Wang Yaqiang <wangyaqiang03@kuaishou.com>
>> 
>> In some videos, SPS will be stored before VPS in hvcC box,
>> parse SPS does not depend on VPS, so the video is expected to be processed normally.
>> Added "parsed_vps" parameter to indicate whether VPS have been parsed.
>> Only VPS have been parsed can be verified during SPS parsing.
>> 
>> Signed-off-by: Wang Yaqiang <wangyaqiang03@kuaishou.com>
>> ---
>> libavcodec/hevc_ps.c | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
> 
> This causes segfaults
> 
> ==816== Invalid read of size 8
> ==816==    at 0xFAF178: hevc_parse (in ffmpeg_g)
> ==816==    by 0xA7A2A6: av_parser_parse2 (in ffmpeg_g)
> ==816==    by 0x5FC388: parse_packet (in ffmpeg_g)
> ==816==    by 0x5FDC5D: read_frame_internal (in ffmpeg_g)
> ==816==    by 0x5FFA10: avformat_find_stream_info (in ffmpeg_g)
> ==816==    by 0x2F6054: open_input_file (in ffmpeg_g)
> ==816==    by 0x2FC6AB: ffmpeg_parse_options (in ffmpeg_g)
> ==816==    by 0x2E8A34: main (in ffmpeg_g)
> ==816==  Address 0x8 is not stack'd, malloc'd or (recently) free'd
> 
> 
> [...]

Excuse me, we have run tests on our own business and have not found this problem, but it is a hidden risk,really hope you can tell me how to reproduce this problem. Thanks

> 
> -- 
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> 
> When the tyrant has disposed of foreign enemies by conquest or treaty, and
> there is nothing more to fear from them, then he is always stirring up
> some war or other, in order that the people may require a leader. -- Plato
> _______________________________________________
> 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".
Michael Niedermayer Oct. 14, 2022, 11 p.m. UTC | #4 
On Fri, Oct 14, 2022 at 06:13:14PM +0800, wangyaqiang wrote:
> 
> 
> > 2022年9月27日 04:21，Michael Niedermayer <michael@niedermayer.cc> 写道：
> > 
> > On Mon, Sep 26, 2022 at 05:38:14PM +0800, 1035567130@qq.com wrote:
> >> From: Wang Yaqiang <wangyaqiang03@kuaishou.com>
> >> 
> >> In some videos, SPS will be stored before VPS in hvcC box,
> >> parse SPS does not depend on VPS, so the video is expected to be processed normally.
> >> Added "parsed_vps" parameter to indicate whether VPS have been parsed.
> >> Only VPS have been parsed can be verified during SPS parsing.
> >> 
> >> Signed-off-by: Wang Yaqiang <wangyaqiang03@kuaishou.com>
> >> ---
> >> libavcodec/hevc_ps.c | 3 +--
> >> 1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > This causes segfaults
> > 
> > ==816== Invalid read of size 8
> > ==816==    at 0xFAF178: hevc_parse (in ffmpeg_g)
> > ==816==    by 0xA7A2A6: av_parser_parse2 (in ffmpeg_g)
> > ==816==    by 0x5FC388: parse_packet (in ffmpeg_g)
> > ==816==    by 0x5FDC5D: read_frame_internal (in ffmpeg_g)
> > ==816==    by 0x5FFA10: avformat_find_stream_info (in ffmpeg_g)
> > ==816==    by 0x2F6054: open_input_file (in ffmpeg_g)
> > ==816==    by 0x2FC6AB: ffmpeg_parse_options (in ffmpeg_g)
> > ==816==    by 0x2E8A34: main (in ffmpeg_g)
> > ==816==  Address 0x8 is not stack'd, malloc'd or (recently) free'd
> > 
> > 
> > [...]
> 
> Excuse me, we have run tests on our own business and have not found this problem, but it is a hidden risk,really hope you can tell me how to reproduce this problem. Thanks

heres a more complete stack trace, i willmail you the input sample privatly

Trailing option(s) found in the command: may be ignored.
[hevc @ 0x16a0ad40] Invalid NAL unit 0, skipping.
[hevc @ 0x16a0ad40] PTL information too short
==24589==    at 0x12A19DF: VALGRIND_PRINTF_BACKTRACE (valgrind.h:6303)
==24589==    by 0x12A259D: av_log_default_callback (log.c:399)
==24589==    by 0x12A2844: av_vlog (log.c:434)
==24589==    by 0x12A26A3: av_log (log.c:413)
==24589==    by 0x10A7216: parse_ptl (hevc_ps.c:342)
==24589==    by 0x10A78B4: ff_hevc_decode_nal_vps (hevc_ps.c:503)
==24589==    by 0x10A579C: parse_nal_units (hevc_parser.c:212)
==24589==    by 0x10A5B46: hevc_parse (hevc_parser.c:331)
==24589==    by 0xB82366: av_parser_parse2 (parser.c:163)
==24589==    by 0x61BD17: parse_packet (demux.c:1140)
==24589==    by 0x61C936: read_frame_internal (demux.c:1334)
==24589==    by 0x6217A7: avformat_find_stream_info (demux.c:2612)
==24589==    by 0x246A51: open_input_file (ffmpeg_opt.c:1315)
==24589==    by 0x255E38: open_files (ffmpeg_opt.c:3703)
==24589==    by 0x255FEC: ffmpeg_parse_options (ffmpeg_opt.c:3742)
==24589==    by 0x26EFEE: main (ffmpeg.c:4236)
[hevc @ 0x16a0ad40] VPS 0 does not exist
==24589== Invalid read of size 8
==24589==    at 0x10A5189: hevc_parse_slice_header (hevc_parser.c:88)
==24589==    by 0x10A584E: parse_nal_units (hevc_parser.c:245)
==24589==    by 0x10A5B46: hevc_parse (hevc_parser.c:331)
==24589==    by 0xB82366: av_parser_parse2 (parser.c:163)
==24589==    by 0x61BD17: parse_packet (demux.c:1140)
==24589==    by 0x61C936: read_frame_internal (demux.c:1334)
==24589==    by 0x6217A7: avformat_find_stream_info (demux.c:2612)
==24589==    by 0x246A51: open_input_file (ffmpeg_opt.c:1315)
==24589==    by 0x255E38: open_files (ffmpeg_opt.c:3703)
==24589==    by 0x255FEC: ffmpeg_parse_options (ffmpeg_opt.c:3742)
==24589==    by 0x26EFEE: main (ffmpeg.c:4236)
==24589==  Address 0x8 is not stack'd, malloc'd or (recently) free'd
==24589== 
==24589== 
==24589== Process terminating with default action of signal 11 (SIGSEGV)
==24589==  Access not within mapped region at address 0x8
==24589==    at 0x10A5189: hevc_parse_slice_header (hevc_parser.c:88)
==24589==    by 0x10A584E: parse_nal_units (hevc_parser.c:245)
==24589==    by 0x10A5B46: hevc_parse (hevc_parser.c:331)
==24589==    by 0xB82366: av_parser_parse2 (parser.c:163)
==24589==    by 0x61BD17: parse_packet (demux.c:1140)
==24589==    by 0x61C936: read_frame_internal (demux.c:1334)
==24589==    by 0x6217A7: avformat_find_stream_info (demux.c:2612)
==24589==    by 0x246A51: open_input_file (ffmpeg_opt.c:1315)
==24589==    by 0x255E38: open_files (ffmpeg_opt.c:3703)
==24589==    by 0x255FEC: ffmpeg_parse_options (ffmpeg_opt.c:3742)
==24589==    by 0x26EFEE: main (ffmpeg.c:4236)

[...]
wangyaqiang Oct. 25, 2022, 2:29 p.m. UTC | #5 
> 2022年10月15日 07:00，Michael Niedermayer <michael@niedermayer.cc> 写道：
> 
> On Fri, Oct 14, 2022 at 06:13:14PM +0800, wangyaqiang wrote:
>> 
>> 
>>> 2022年9月27日 04:21，Michael Niedermayer <michael@niedermayer.cc> 写道：
>>> 
>>> On Mon, Sep 26, 2022 at 05:38:14PM +0800, 1035567130@qq.com wrote:
>>>> From: Wang Yaqiang <wangyaqiang03@kuaishou.com>
>>>> 
>>>> In some videos, SPS will be stored before VPS in hvcC box,
>>>> parse SPS does not depend on VPS, so the video is expected to be processed normally.
>>>> Added "parsed_vps" parameter to indicate whether VPS have been parsed.
>>>> Only VPS have been parsed can be verified during SPS parsing.
>>>> 
>>>> Signed-off-by: Wang Yaqiang <wangyaqiang03@kuaishou.com>
>>>> ---
>>>> libavcodec/hevc_ps.c | 3 +--
>>>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>> 
>>> This causes segfaults
>>> 
>>> ==816== Invalid read of size 8
>>> ==816==    at 0xFAF178: hevc_parse (in ffmpeg_g)
>>> ==816==    by 0xA7A2A6: av_parser_parse2 (in ffmpeg_g)
>>> ==816==    by 0x5FC388: parse_packet (in ffmpeg_g)
>>> ==816==    by 0x5FDC5D: read_frame_internal (in ffmpeg_g)
>>> ==816==    by 0x5FFA10: avformat_find_stream_info (in ffmpeg_g)
>>> ==816==    by 0x2F6054: open_input_file (in ffmpeg_g)
>>> ==816==    by 0x2FC6AB: ffmpeg_parse_options (in ffmpeg_g)
>>> ==816==    by 0x2E8A34: main (in ffmpeg_g)
>>> ==816==  Address 0x8 is not stack'd, malloc'd or (recently) free'd
>>> 
>>> 
>>> [...]
>> 
>> Excuse me, we have run tests on our own business and have not found this problem, but it is a hidden risk,really hope you can tell me how to reproduce this problem. Thanks
> 
> heres a more complete stack trace, i willmail you the input sample privatly
> 

Thanks very much for the materials. Under the condition that vps is not strongly checked when parsing sps, it is necessary to determine whether vps is empty when vps is used. I submitted a new patch to fix the crash, but I feel that this way is a little trick,any good ideas?

> Trailing option(s) found in the command: may be ignored.
> [hevc @ 0x16a0ad40] Invalid NAL unit 0, skipping.
> [hevc @ 0x16a0ad40] PTL information too short
> ==24589==    at 0x12A19DF: VALGRIND_PRINTF_BACKTRACE (valgrind.h:6303)
> ==24589==    by 0x12A259D: av_log_default_callback (log.c:399)
> ==24589==    by 0x12A2844: av_vlog (log.c:434)
> ==24589==    by 0x12A26A3: av_log (log.c:413)
> ==24589==    by 0x10A7216: parse_ptl (hevc_ps.c:342)
> ==24589==    by 0x10A78B4: ff_hevc_decode_nal_vps (hevc_ps.c:503)
> ==24589==    by 0x10A579C: parse_nal_units (hevc_parser.c:212)
> ==24589==    by 0x10A5B46: hevc_parse (hevc_parser.c:331)
> ==24589==    by 0xB82366: av_parser_parse2 (parser.c:163)
> ==24589==    by 0x61BD17: parse_packet (demux.c:1140)
> ==24589==    by 0x61C936: read_frame_internal (demux.c:1334)
> ==24589==    by 0x6217A7: avformat_find_stream_info (demux.c:2612)
> ==24589==    by 0x246A51: open_input_file (ffmpeg_opt.c:1315)
> ==24589==    by 0x255E38: open_files (ffmpeg_opt.c:3703)
> ==24589==    by 0x255FEC: ffmpeg_parse_options (ffmpeg_opt.c:3742)
> ==24589==    by 0x26EFEE: main (ffmpeg.c:4236)
> [hevc @ 0x16a0ad40] VPS 0 does not exist
> ==24589== Invalid read of size 8
> ==24589==    at 0x10A5189: hevc_parse_slice_header (hevc_parser.c:88)
> ==24589==    by 0x10A584E: parse_nal_units (hevc_parser.c:245)
> ==24589==    by 0x10A5B46: hevc_parse (hevc_parser.c:331)
> ==24589==    by 0xB82366: av_parser_parse2 (parser.c:163)
> ==24589==    by 0x61BD17: parse_packet (demux.c:1140)
> ==24589==    by 0x61C936: read_frame_internal (demux.c:1334)
> ==24589==    by 0x6217A7: avformat_find_stream_info (demux.c:2612)
> ==24589==    by 0x246A51: open_input_file (ffmpeg_opt.c:1315)
> ==24589==    by 0x255E38: open_files (ffmpeg_opt.c:3703)
> ==24589==    by 0x255FEC: ffmpeg_parse_options (ffmpeg_opt.c:3742)
> ==24589==    by 0x26EFEE: main (ffmpeg.c:4236)
> ==24589==  Address 0x8 is not stack'd, malloc'd or (recently) free'd
> ==24589== 
> ==24589== 
> ==24589== Process terminating with default action of signal 11 (SIGSEGV)
> ==24589==  Access not within mapped region at address 0x8
> ==24589==    at 0x10A5189: hevc_parse_slice_header (hevc_parser.c:88)
> ==24589==    by 0x10A584E: parse_nal_units (hevc_parser.c:245)
> ==24589==    by 0x10A5B46: hevc_parse (hevc_parser.c:331)
> ==24589==    by 0xB82366: av_parser_parse2 (parser.c:163)
> ==24589==    by 0x61BD17: parse_packet (demux.c:1140)
> ==24589==    by 0x61C936: read_frame_internal (demux.c:1334)
> ==24589==    by 0x6217A7: avformat_find_stream_info (demux.c:2612)
> ==24589==    by 0x246A51: open_input_file (ffmpeg_opt.c:1315)
> ==24589==    by 0x255E38: open_files (ffmpeg_opt.c:3703)
> ==24589==    by 0x255FEC: ffmpeg_parse_options (ffmpeg_opt.c:3742)
> ==24589==    by 0x26EFEE: main (ffmpeg.c:4236)
> 
> [...]
> 
> -- 
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> 
> Concerning the gods, I have no means of knowing whether they exist or not
> or of what sort they may be, because of the obscurity of the subject, and
> the brevity of human life -- Protagoras
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org <mailto:ffmpeg-devel@ffmpeg.org>
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel <https://ffmpeg.org/mailman/listinfo/ffmpeg-devel>
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org <mailto:ffmpeg-devel-request@ffmpeg.org> with subject "unsubscribe".
diff mbox series

Patch

diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c
index f665d8053c..9abee8bd90 100644
--- a/libavcodec/hevc_ps.c
+++ b/libavcodec/hevc_ps.c
@@ -916,9 +916,8 @@  int ff_hevc_parse_sps(HEVCSPS *sps, GetBitContext *gb, unsigned int *sps_id,
     sps->vps_id = get_bits(gb, 4);
 
     if (vps_list && !vps_list[sps->vps_id]) {
-        av_log(avctx, AV_LOG_ERROR, "VPS %d does not exist\n",
+        av_log(avctx, AV_LOG_WARNING, "VPS %d does not exist\n",
                sps->vps_id);
-        return AVERROR_INVALIDDATA;
     }
 
     sps->max_sub_layers = get_bits(gb, 3) + 1;