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

[Timothy B. Terriberry]

James Zern wrote:
> On Tue, Nov 28, 2017 at 3:22 PM, James Zern <jzern at google.com> wrote:
>> On Mon, Nov 20, 2017 at 1:07 PM, James Zern <jzern at google.com> wrote:
>>> Just an attempt to avoid overflows with an explicit check, I don't know if
>>> there's a better way to identify corrupt input here.
>>>
>>> James Zern (2):
>>>    op_pcm_seek: fix int64 overflow
>>>    op_fetch_and_process_page: fix int64 overflow
>>>
>>>   src/opusfile.c | 11 +++++++++--
>>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>>
>>
>> Any comments on these?
>>
>
> ping.

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.

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).

The way you have this now, if the gp of whatever current page we happen 
to be decoding comes before the start of the stream, you'll set 
discard_count to 0 and then we will pass the following if() and declare 
the seek a success, even if the target was in some other, uncorrupted 
part of the file. Conversely, if the gp of the current page is so large 
it would make the number of samples to discard overflow a signed 64-bit 
value, we'll do the same thing. I think in both cases we should fall 
back to trying a full seek. We don't guarantee seeking success in the 
face of invalid timestamps like this, but at least if we try a full seek 
and fail we'll report an error in most cases instead of pretending we 
succeeded.

> @@ -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.

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