| Message ID | 21f10d45-db51-62fd-9fdd-3d01b8eecd3e@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [FFmpeg-devel] dashenc: check pts to prevent division by zero error | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | fail | Failed to apply patch |
On 1/30/20 3:28 PM, Alfred E. Heggestad wrote: > this usecase will cause a division by zero trap: > > 1. dashenc has received one frame > 2. os->max_pts and os->start_pts have same value > 3. delta between max_pts and start_pts is 0 > 4. av_rescale_q(0, x, y) returns 0 > 5. this value is used as denominator in division > 6. Bang! -> segfault > > this fix checks that max_pts > start_pts. > the fix has been tested and works. > > please review and suggest better fixes. > > Signed-off-by: Alfred E. Heggestad <alfred.heggestad@gmail.com> > --- > libavformat/dashenc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c > index f555f208a7..3b651b9514 100644 > --- a/libavformat/dashenc.c > +++ b/libavformat/dashenc.c > @@ -1883,7 +1883,7 @@ static int dash_flush(AVFormatContext *s, int > final, int stream) > > st->time_base, > > AV_TIME_BASE_Q)); > > - if (!os->muxer_overhead) > + if (!os->muxer_overhead && os->max_pts > os->start_pts) > os->muxer_overhead = ((int64_t) (range_length - > os->total_pkt_size) * > 8 * AV_TIME_BASE) / > av_rescale_q(os->max_pts - os->start_pts, LGTM. This actually exposes a corner case bug in overhead calculation logic. Guess we will need to come up with a better logic for it. Until then, this fix will atleast make sure things don’t crash. Thanks, Karthick
On 1/31/2020 10:08 AM, Jeyapal, Karthick wrote: > > On 1/30/20 3:28 PM, Alfred E. Heggestad wrote: >> this usecase will cause a division by zero trap: >> >> 1. dashenc has received one frame >> 2. os->max_pts and os->start_pts have same value >> 3. delta between max_pts and start_pts is 0 >> 4. av_rescale_q(0, x, y) returns 0 >> 5. this value is used as denominator in division >> 6. Bang! -> segfault >> >> this fix checks that max_pts > start_pts. >> the fix has been tested and works. >> >> please review and suggest better fixes. >> >> Signed-off-by: Alfred E. Heggestad <alfred.heggestad@gmail.com> >> --- >> libavformat/dashenc.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c >> index f555f208a7..3b651b9514 100644 >> --- a/libavformat/dashenc.c >> +++ b/libavformat/dashenc.c >> @@ -1883,7 +1883,7 @@ static int dash_flush(AVFormatContext *s, int >> final, int stream) >> >> st->time_base, >> >> AV_TIME_BASE_Q)); >> >> - if (!os->muxer_overhead) >> + if (!os->muxer_overhead && os->max_pts > os->start_pts) >> os->muxer_overhead = ((int64_t) (range_length - >> os->total_pkt_size) * >> 8 * AV_TIME_BASE) / >> av_rescale_q(os->max_pts - os->start_pts, > LGTM. > > This actually exposes a corner case bug in overhead calculation logic. > Guess we will need to come up with a better logic for it. > Until then, this fix will atleast make sure things don’t crash. > > Thanks, > Karthick Applied.
On 31/01/2020 14:17, James Almer wrote: > On 1/31/2020 10:08 AM, Jeyapal, Karthick wrote: >> >> On 1/30/20 3:28 PM, Alfred E. Heggestad wrote: >>> this usecase will cause a division by zero trap: >>> >>> 1. dashenc has received one frame >>> 2. os->max_pts and os->start_pts have same value >>> 3. delta between max_pts and start_pts is 0 >>> 4. av_rescale_q(0, x, y) returns 0 >>> 5. this value is used as denominator in division >>> 6. Bang! -> segfault >>> >>> this fix checks that max_pts > start_pts. >>> the fix has been tested and works. >>> >>> please review and suggest better fixes. >>> >>> Signed-off-by: Alfred E. Heggestad <alfred.heggestad@gmail.com> >>> --- >>> libavformat/dashenc.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c >>> index f555f208a7..3b651b9514 100644 >>> --- a/libavformat/dashenc.c >>> +++ b/libavformat/dashenc.c >>> @@ -1883,7 +1883,7 @@ static int dash_flush(AVFormatContext *s, int >>> final, int stream) >>> >>> st->time_base, >>> >>> AV_TIME_BASE_Q)); >>> >>> - if (!os->muxer_overhead) >>> + if (!os->muxer_overhead && os->max_pts > os->start_pts) >>> os->muxer_overhead = ((int64_t) (range_length - >>> os->total_pkt_size) * >>> 8 * AV_TIME_BASE) / >>> av_rescale_q(os->max_pts - os->start_pts, >> LGTM. >> >> This actually exposes a corner case bug in overhead calculation logic. >> Guess we will need to come up with a better logic for it. >> Until then, this fix will atleast make sure things don’t crash. >> >> Thanks, >> Karthick > > Applied. Great! Thanks Jeyapal and James. /alfred
diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c index f555f208a7..3b651b9514 100644 --- a/libavformat/dashenc.c +++ b/libavformat/dashenc.c @@ -1883,7 +1883,7 @@ static int dash_flush(AVFormatContext *s, int final, int stream) st->time_base, AV_TIME_BASE_Q)); - if (!os->muxer_overhead) + if (!os->muxer_overhead && os->max_pts > os->start_pts)
this usecase will cause a division by zero trap: 1. dashenc has received one frame 2. os->max_pts and os->start_pts have same value 3. delta between max_pts and start_pts is 0 4. av_rescale_q(0, x, y) returns 0 5. this value is used as denominator in division 6. Bang! -> segfault this fix checks that max_pts > start_pts. the fix has been tested and works. please review and suggest better fixes. Signed-off-by: Alfred E. Heggestad <alfred.heggestad@gmail.com> --- libavformat/dashenc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) os->muxer_overhead = ((int64_t) (range_length - os->total_pkt_size) * 8 * AV_TIME_BASE) / av_rescale_q(os->max_pts - os->start_pts,