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.>> -- >> 2.15.0.448.gf294e3d99a-goog >>
Timothy B. Terriberry
2017-Dec-07 20:18 UTC
[opus] [PATCH 0/2] libopusfile int64 overflows
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 encodersFor 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.
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.