[FFmpeg-devel] 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>

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

[...]