Message ID | 20210225063816.8644-6-yejun.guo@intel.com |
---|---|
State | New |
Headers | show |
Series | [FFmpeg-devel,V2,1/7] libavdevice/v4l2.c: fix build warning for [-Wformat-truncation=] | expand |
Context | Check | Description |
---|---|---|
andriy/x86_make | success | Make finished |
andriy/x86_make_fate | success | Make fate finished |
andriy/PPC64_make | success | Make finished |
andriy/PPC64_make_fate | success | Make fate finished |
> On 25 Feb 2021, at 07:38, Guo, Yejun <yejun.guo@intel.com> wrote: > --- a/libavformat/smoothstreamingenc.c > +++ b/libavformat/smoothstreamingenc.c > @@ -501,7 +501,7 @@ static int ism_flush(AVFormatContext *s, int final) > > for (i = 0; i < s->nb_streams; i++) { > OutputStream *os = &c->streams[i]; > - char filename[1024], target_filename[1024], header_filename[1024], curr_dirname[1024]; > + char filename[2048], target_filename[2048], header_filename[2048], curr_dirname[1024]; > int64_t size; > int64_t start_ts, duration, moof_size; > if (!os->packets_written) IMO some of these allocations are getting a bit too large for the stack (multi-page stack allocations weaken security measures even if the large arrays themselves do not overflow). And no matter what size you put, there’s always a larger filename possible where it breaks, so it feels like just warning polishing with marginal real benefit. Why not use av_asprintf, then at least the problem is actually solved for real? I don’t see that this code is performance relevant, so the only reason to put these onto stack is being too lazy to do the memory management, which I think is a fairly weak argument. Best regards, Reimar
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Reimar > D?ffinger > Sent: 2021年2月25日 16:22 > To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org> > Subject: Re: [FFmpeg-devel] [PATCH V2 6/7] > libavformat/smoothstreamingenc.c: fix build warning for > [-Wformat-truncation=] > > > > On 25 Feb 2021, at 07:38, Guo, Yejun <yejun.guo@intel.com> wrote: > > --- a/libavformat/smoothstreamingenc.c > > +++ b/libavformat/smoothstreamingenc.c > > @@ -501,7 +501,7 @@ static int ism_flush(AVFormatContext *s, int > > final) > > > > for (i = 0; i < s->nb_streams; i++) { > > OutputStream *os = &c->streams[i]; > > - char filename[1024], target_filename[1024], > header_filename[1024], curr_dirname[1024]; > > + char filename[2048], target_filename[2048], > > + header_filename[2048], curr_dirname[1024]; > > int64_t size; > > int64_t start_ts, duration, moof_size; > > if (!os->packets_written) > > IMO some of these allocations are getting a bit too large for the stack > (multi-page stack allocations weaken security measures even if the large arrays > themselves do not overflow). > And no matter what size you put, there’s always a larger filename possible > where it breaks, so it feels like just warning polishing with marginal real > benefit. > Why not use av_asprintf, then at least the problem is actually solved for real? > I don’t see that this code is performance relevant, so the only reason to put > these onto stack is being too lazy to do the memory management, which I > think is a fairly weak argument. > thanks, and get the point that it is a possible security issue. There's a max len for the filename, and so I use 'filename[1029] etc.' in V1 patch. But I think the magic number 1029 is weird and so just changed to 2048 in V2. There's no other reasons to choose 2048. av_asprintf is a good choice, I'll look at it and send patches in V3.
diff --git a/libavformat/smoothstreamingenc.c b/libavformat/smoothstreamingenc.c index ba5cc27ca0..5a9a5dddf0 100644 --- a/libavformat/smoothstreamingenc.c +++ b/libavformat/smoothstreamingenc.c @@ -501,7 +501,7 @@ static int ism_flush(AVFormatContext *s, int final) for (i = 0; i < s->nb_streams; i++) { OutputStream *os = &c->streams[i]; - char filename[1024], target_filename[1024], header_filename[1024], curr_dirname[1024]; + char filename[2048], target_filename[2048], header_filename[2048], curr_dirname[1024]; int64_t size; int64_t start_ts, duration, moof_size; if (!os->packets_written)
Part of the build message: src/libavformat/smoothstreamingenc.c: In function ‘ism_flush’: src/libavformat/smoothstreamingenc.c:510:49: warning: ‘/temp’ directive output may be truncated writing 5 bytes into a region of size between 1 and 1024 [-Wformat-truncation=] snprintf(filename, sizeof(filename), "%s/temp", os->dirname); ^~~~~ src/libavformat/smoothstreamingenc.c:510:9: note: ‘snprintf’ output between 6 and 1029 bytes into a destination of size 1024 snprintf(filename, sizeof(filename), "%s/temp", os->dirname); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ src/libavformat/smoothstreamingenc.c:538:53: warning: ‘/temp’ directive output may be truncated writing 5 bytes into a region of size between 1 and 1024 [-Wformat-truncation=] snprintf(filename, sizeof(filename), "%s/temp", os->dirname); ^~~~~ src/libavformat/smoothstreamingenc.c:538:13: note: ‘snprintf’ output between 6 and 1029 bytes into a destination of size 1024 snprintf(filename, sizeof(filename), "%s/temp", os->dirname); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ src/libavformat/smoothstreamingenc.c:545:63: warning: ‘/FragmentInfo(’ directive output may be truncated writing 14 bytes into a region of size between 1 and 1024 [-Wformat-truncation=] snprintf(header_filename, sizeof(header_filename), "%s/FragmentInfo(%s=%"PRIu64")", os->dirname, os->stream_type_tag, start_ts); ^~~~~~~~~~~~~~ src/libavformat/smoothstreamingenc.c:545:60: note: using the range [0, 18446744073709551615] for directive argument snprintf(header_filename, sizeof(header_filename), "%s/FragmentInfo(%s=%"PRIu64")", os->dirname, os->stream_type_tag, start_ts); ^~~~~~~~~~~~~~~~~~~~~~ src/libavformat/smoothstreamingenc.c:545:9: note: ‘snprintf’ output 18 or more bytes (assuming 1041) into a destination of size 1024 snprintf(header_filename, sizeof(header_filename), "%s/FragmentInfo(%s=%"PRIu64")", os->dirname, os->stream_type_tag, start_ts); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Signed-off-by: Guo, Yejun <yejun.guo@intel.com> --- libavformat/smoothstreamingenc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)