[opus] [PATCH 0/2] libopusfile int64 overflows

[James Zern]

On Thu, Dec 7, 2017 at 12:18 PM, Timothy B. Terriberry
<tterribe at xiph.org> wrote:
> [...]
>
> Sorry, I can't reply to the original patches because I didn't actually get
> that e-mail due to local trouble with my mail server. I could pull the
> patches from the list archive, however. Thanks for the reports.
>

Thanks for recovering them and having a look. I updated both patches.

> For the first patch:
>
>> @@ -2605,7 +2605,11 @@ int op_pcm_seek(OggOpusFile *_of,ogg_int64_t
>> _pcm_offset){
>>           would be better just to do a full seek.*/
>>        if(OP_LIKELY(!op_granpos_diff(&diff,gp,pcm_start))){
>>          ogg_int64_t discard_count;
>> -        discard_count=_pcm_offset-diff;
>> +        /*Check for overflow.*/
>> +        if(diff<0&&OP_UNLIKELY(OP_INT64_MAX+diff<_pcm_offset)){
>> +          discard_count=0;
>> +        }
>> +        else discard_count=_pcm_offset-diff;
>>          /*We use a threshold of 90 ms instead of 80, since 80 ms is the
>>             _minimum_ we would have discarded after a full seek.
>>            Assuming 20 ms frames (the default), we'd discard 90 ms on
>> average.*/
>>         if(discard_count>=0&&OP_UNLIKELY(discard_count<90*48)){
>
>
> I think that better than adding a custom overflow check here, we should use
> if(OP_LIKELY(!op_granpos_diff(&discard_count,target_gp,gp))) directly
> (because _pcm_offset == (target_gp - pcm_start) and diff == (gp -
> pcm_start).
>

This works.

> [...]
>
>> @@ -2078,7 +2078,10 @@ static int op_fetch_and_process_page(OggOpusFile
>> *_of,
>>           &&OP_LIKELY(diff<total_duration)){
>>            cur_packet_gp=prev_packet_gp;
>>            for(pi=0;pi<op_count;pi++){
>> -            diff=durations[pi]-diff;
>> +            /*Check for overflow.*/
>> +            if(diff<0&&OP_UNLIKELY(OP_INT64_MAX+diff<durations[pi])){
>> +              diff=0;
>> +            } else diff=durations[pi]-diff;
>>              /*If we have samples to trim...*/
>>              if(diff>0){
>>                /*If we trimmed the entire packet, stop (the spec says
>> encoders
>
>
> For this one, the only way that durations[pi] - diff can overflow on the
> positive side is if cur_page_gp < prev_packet_gp (in fact, it must be very
> much less). So the question becomes what behavior we would like in that
> case. In the case where cur_page_gp < prev_packet_gp but diff -
> durations[pi] does not overflow, then we will have diff > durations[pi]
> after the subtraction and we will trim all of the packets on the page. But
> with your patch, once cur_page_gp comes early enough to cause an overflow,
> you'll set diff=0, and then we go into the other case in the loop, where we
> propagate timestamps forward from the _previous_ page, ignoring the
> timestamp of the current page. Now, since the timestamps are clearly
> invalid, maybe it doesn't matter much which we do, but it seems like it's
> kind of an arbitrary place to suddenly switch from discarding all of the
> packets to instead playing them back with timestamps replaced from somewhere
> else. So I think the more consistent behavior would be to discard them in
> all cases (e.g., by setting diff=durations[pi]+1). durations[pi] is bounded
> (no more than 5760), so this has no chance of overflow itself.
>

That works, I wasn't sure which behavior would be preferable.

> Also, just as a point of style in this codebase, the else keyword should be
> on a new line after the brace.

I missed that, fixed in the latest.