Message ID | CAPUDrwf5yeow+RhPVpZzWsiOj8HYLW0Ws7wbUfTfqpM6FBSYgQ@mail.gmail.com |
---|---|
State | New |
Headers | show |
Series | [FFmpeg-devel] Fix undefined behavior in ff_configure_buffers_for_index() | expand |
Context | Check | Description |
---|---|---|
andriy/ffmpeg-patchwork | success | Make fate finished |
On Tue, Jan 28, 2020 at 04:52:16PM -0800, Dale Curtis wrote: > When e2_pts == INT64_MIN and e1_pts >= 0 the calculation of > e2_pts - e1_pts will overflow an int64_t. So instead check > for overflow and default to |time_tolerance| if the value > is too large for an int64_t. > > Signed-off-by: Dale Curtis <dalecurtis@chromium.org> > utils.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > 9c3670236229794d325158a25f26fd0cdf459310 ubfix.patch > From 412751f4747faf34e3dba088dc55290783eb6bd5 Mon Sep 17 00:00:00 2001 > From: Dale Curtis <dalecurtis@chromium.org> > Date: Tue, 28 Jan 2020 16:49:14 -0800 > Subject: [PATCH] Fix undefined behavior in ff_configure_buffers_for_index() > > When e2_pts == INT64_MIN and e1_pts >= 0 the calculation of > e2_pts - e1_pts will overflow an int64_t. So instead check > for overflow and default to |time_tolerance| if the value > is too large for an int64_t. > > Signed-off-by: Dale Curtis <dalecurtis@chromium.org> > --- > libavformat/utils.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/libavformat/utils.c b/libavformat/utils.c > index e22ca7cab8..d6197358c9 100644 > --- a/libavformat/utils.c > +++ b/libavformat/utils.c > @@ -2135,7 +2135,13 @@ void ff_configure_buffers_for_index(AVFormatContext *s, int64_t time_tolerance) > for (; i2 < st2->nb_index_entries; i2++) { > AVIndexEntry *e2 = &st2->index_entries[i2]; > int64_t e2_pts = av_rescale_q(e2->timestamp, st2->time_base, AV_TIME_BASE_Q); > - if (e2_pts - e1_pts < time_tolerance) > + int64_t delta = e1_pts < 1 ? INT64_MAX + e1_pts >= e2_pts > + ? e2_pts - e1_pts > + : time_tolerance > + : INT64_MIN + e1_pts <= e2_pts > + ? e2_pts - e1_pts > + : time_tolerance; > + if (delta < time_tolerance) > continue; simpler solution, and also behaves arithmetically more correct when the overflow happens in the othert direction: av_assert0(time_tolerance >= 0); if (e2_pts < e1_pts || e2_pts - (uint64_t)e1_pts < time_tolerance) continue; [...]
On Wed, Jan 29, 2020 at 4:55 AM Michael Niedermayer <michael@niedermayer.cc> wrote: > simpler solution, and also behaves arithmetically more correct when the > overflow happens in the othert direction: > > av_assert0(time_tolerance >= 0); > > if (e2_pts < e1_pts || e2_pts - (uint64_t)e1_pts < time_tolerance) > Does that work? e1_pts is INT64_MIN in this case. So the (uint64_t)e1_pts > e2_pts.
On Wed, Jan 29, 2020 at 03:25:32PM -0800, Dale Curtis wrote: > On Wed, Jan 29, 2020 at 4:55 AM Michael Niedermayer <michael@niedermayer.cc> > wrote: > > > simpler solution, and also behaves arithmetically more correct when the > > overflow happens in the othert direction: > > > > av_assert0(time_tolerance >= 0); > > > > if (e2_pts < e1_pts || e2_pts - (uint64_t)e1_pts < time_tolerance) > > > > Does that work? e1_pts is INT64_MIN in this case. So the (uint64_t)e1_pts > > e2_pts. the original code was this: if (e2_pts - e1_pts < time_tolerance) time_tolerance is always positive e2_pts - e1_pts can be either positive or negative if its negative (e2_pts < e1_pts) then the original would be true and thats the first term of the new condition otherwise e2_pts - e1_pts is positive and can be represented as a uint64_t which is the 2nd term of the new condition so i think it works but maybe ive missed something, for which values of e2_pts do you see a problem with e1_pts = INT64_MIN? thx [...]
On Wed, Jan 29, 2020 at 10:23 PM Michael Niedermayer <michael@niedermayer.cc> wrote: > so i think it works but maybe ive missed something, for which values > of e2_pts do you see a problem with e1_pts = INT64_MIN? > For e1_pts = INT64_MIN and e2_pts >= 0 you end up with a negative int64_t result for e2_pts - (uint64_t)e1_pts, so it's always < time_tolerance. If that's what you intended, then sgtm. - dale
Bump to apply? - dale On Thu, Jan 30, 2020 at 3:21 PM Dale Curtis <dalecurtis@google.com> wrote: > On Thu, Jan 30, 2020 at 11:23 AM Dale Curtis <dalecurtis@chromium.org> > wrote: > >> On Wed, Jan 29, 2020 at 10:23 PM Michael Niedermayer >> <michael@niedermayer.cc> wrote: >> >>> so i think it works but maybe ive missed something, for which values >>> of e2_pts do you see a problem with e1_pts = INT64_MIN? >>> >> >> For e1_pts = INT64_MIN and e2_pts >= 0 you end up with a negative int64_t >> result for e2_pts - (uint64_t)e1_pts, so it's always < time_tolerance. If >> that's what you intended, then sgtm. >> > > Actually, this was a test construction error on my part. In a conditional > this does evaluate to a uint64_t vs int64_t, so the comparison is valid. > I've attached your recommended patch. Thanks for your patience. > > - dale > > >
On Thu, Jan 30, 2020 at 11:23:07AM -0800, Dale Curtis wrote: > On Wed, Jan 29, 2020 at 10:23 PM Michael Niedermayer <michael@niedermayer.cc> > wrote: > > > so i think it works but maybe ive missed something, for which values > > of e2_pts do you see a problem with e1_pts = INT64_MIN? > > > > For e1_pts = INT64_MIN and e2_pts >= 0 you end up with a negative int64_t > result for e2_pts - (uint64_t)e1_pts, so it's always < time_tolerance. If > that's what you intended, then sgtm. thats what the code would do if the elemnts where large enough to not overflow so that seems to match whats intended. Do you see some issue here ? If the idea is that extreem values are likely invalid and should be treated special, then yes but to detect such values using the equations overflow seems not correct. Similarly any other special values if they reach this function should be checked for more explicitly Thanks [...]
On Thu, Feb 6, 2020 at 3:38 PM Michael Niedermayer <michaelni@gmx.at> wrote: > On Thu, Jan 30, 2020 at 11:23:07AM -0800, Dale Curtis wrote: > > On Wed, Jan 29, 2020 at 10:23 PM Michael Niedermayer > <michael@niedermayer.cc> > > wrote: > > > > > so i think it works but maybe ive missed something, for which values > > > of e2_pts do you see a problem with e1_pts = INT64_MIN? > > > > > > > For e1_pts = INT64_MIN and e2_pts >= 0 you end up with a negative int64_t > > result for e2_pts - (uint64_t)e1_pts, so it's always < time_tolerance. If > > that's what you intended, then sgtm. > > thats what the code would do if the elemnts where large enough to not > overflow > so that seems to match whats intended. > > Do you see some issue here ? > Whoops, sorry, I just realized my previous comment was rejected from the list since I used the wrong e-mail address. My previous statement was incorrect. The code you proposed correctly promotes a uint64_t and not an int64_t when used in a conditional. Here's what I wrote that got lost: "Actually, this was a test construction error on my part. In a conditional this does evaluate to a uint64_t vs int64_t, so the comparison is valid. I've attached your recommended patch. Thanks for your patience." - dale
On Mon, Feb 10, 2020 at 04:20:56PM -0800, Dale Curtis wrote: > On Thu, Feb 6, 2020 at 3:38 PM Michael Niedermayer <michaelni@gmx.at> wrote: > > > On Thu, Jan 30, 2020 at 11:23:07AM -0800, Dale Curtis wrote: > > > On Wed, Jan 29, 2020 at 10:23 PM Michael Niedermayer > > <michael@niedermayer.cc> > > > wrote: > > > > > > > so i think it works but maybe ive missed something, for which values > > > > of e2_pts do you see a problem with e1_pts = INT64_MIN? > > > > > > > > > > For e1_pts = INT64_MIN and e2_pts >= 0 you end up with a negative int64_t > > > result for e2_pts - (uint64_t)e1_pts, so it's always < time_tolerance. If > > > that's what you intended, then sgtm. > > > > thats what the code would do if the elemnts where large enough to not > > overflow > > so that seems to match whats intended. > > > > Do you see some issue here ? > > > > Whoops, sorry, I just realized my previous comment was rejected from the > list since I used the wrong e-mail address. My previous statement was > incorrect. The code you proposed correctly promotes a uint64_t and not an > int64_t when used in a conditional. Here's what I wrote that got lost: > "Actually, this was a test construction error on my part. In a conditional > this does evaluate to a uint64_t vs int64_t, so the comparison is valid. > I've attached your recommended patch. Thanks for your patience." > > - dale > utils.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > 1a4608774ffd8058ff9e6f62845c51c91ccd84e0 ubfix_v2.patch > From c50d0a347fc779c71c07757d9cad8a7d56eb857b Mon Sep 17 00:00:00 2001 > From: Dale Curtis <dalecurtis@chromium.org> > Date: Tue, 28 Jan 2020 16:49:14 -0800 > Subject: [PATCH] Fix undefined behavior in ff_configure_buffers_for_index() > > When e2_pts == INT64_MIN and e1_pts >= 0 the calculation of > e2_pts - e1_pts will overflow an int64_t. > > Signed-off-by: Dale Curtis <dalecurtis@chromium.org> > --- > libavformat/utils.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) will apply thanks [...]
From 412751f4747faf34e3dba088dc55290783eb6bd5 Mon Sep 17 00:00:00 2001 From: Dale Curtis <dalecurtis@chromium.org> Date: Tue, 28 Jan 2020 16:49:14 -0800 Subject: [PATCH] Fix undefined behavior in ff_configure_buffers_for_index() When e2_pts == INT64_MIN and e1_pts >= 0 the calculation of e2_pts - e1_pts will overflow an int64_t. So instead check for overflow and default to |time_tolerance| if the value is too large for an int64_t. Signed-off-by: Dale Curtis <dalecurtis@chromium.org> --- libavformat/utils.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/libavformat/utils.c b/libavformat/utils.c index e22ca7cab8..d6197358c9 100644 --- a/libavformat/utils.c +++ b/libavformat/utils.c @@ -2135,7 +2135,13 @@ void ff_configure_buffers_for_index(AVFormatContext *s, int64_t time_tolerance) for (; i2 < st2->nb_index_entries; i2++) { AVIndexEntry *e2 = &st2->index_entries[i2]; int64_t e2_pts = av_rescale_q(e2->timestamp, st2->time_base, AV_TIME_BASE_Q); - if (e2_pts - e1_pts < time_tolerance) + int64_t delta = e1_pts < 1 ? INT64_MAX + e1_pts >= e2_pts + ? e2_pts - e1_pts + : time_tolerance + : INT64_MIN + e1_pts <= e2_pts + ? e2_pts - e1_pts + : time_tolerance; + if (delta < time_tolerance) continue; pos_delta = FFMAX(pos_delta, e1->pos - e2->pos); break; -- 2.25.0.341.g760bfbb309-goog
When e2_pts == INT64_MIN and e1_pts >= 0 the calculation of e2_pts - e1_pts will overflow an int64_t. So instead check for overflow and default to |time_tolerance| if the value is too large for an int64_t. Signed-off-by: Dale Curtis <dalecurtis@chromium.org>