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(-) -- 2.15.0.448.gf294e3d99a-goog
check for overflow with a negative diff --- src/opusfile.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/opusfile.c b/src/opusfile.c index 72f1272..df326af 100644 --- a/src/opusfile.c +++ b/src/opusfile.c @@ -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.*/ -- 2.15.0.448.gf294e3d99a-goog
James Zern
2017-Nov-20 21:07 UTC
[opus] [PATCH 2/2] op_fetch_and_process_page: fix int64 overflow
check for overflow with a negative diff --- src/opusfile.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/opusfile.c b/src/opusfile.c index df326af..2bef277 100644 --- a/src/opusfile.c +++ b/src/opusfile.c @@ -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 -- 2.15.0.448.gf294e3d99a-goog
Ulrich Windl
2017-Nov-21 07:32 UTC
[opus] Antw: [PATCH 1/2] op_pcm_seek: fix int64 overflow
Hi! I never inspected the Opus sources, but di you guys think that adding blanks (e.g. around binary operators) is a waste of time and storage space? Regards, Ulrich>>> James Zern <jzern at google.com> schrieb am 20.11.2017 um 22:07 in Nachricht<20171120210721.186099-2-jzern at google.com>:> check for overflow with a negative diff > --- > src/opusfile.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/src/opusfile.c b/src/opusfile.c > index 72f1272..df326af 100644 > --- a/src/opusfile.c > +++ b/src/opusfile.c > @@ -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.*/ > -- > 2.15.0.448.gf294e3d99a-goog > > _______________________________________________ > opus mailing list > opus at xiph.org > http://lists.xiph.org/mailman/listinfo/opus
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?> -- > 2.15.0.448.gf294e3d99a-goog >
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 >>
calculate discard_count directly with op_granpos_diff. this works because _pcm_offset == (target_gp - pcm_start) and diff == (gp - pcm_start). --- src/opusfile.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/opusfile.c b/src/opusfile.c index 72f1272..140503e 100644 --- a/src/opusfile.c +++ b/src/opusfile.c @@ -2597,15 +2597,14 @@ int op_pcm_seek(OggOpusFile *_of,ogg_int64_t _pcm_offset){ ogg_int64_t gp; gp=_of->prev_packet_gp; if(OP_LIKELY(gp!=-1)){ - int nbuffered; + ogg_int64_t discard_count; + int nbuffered; nbuffered=OP_MAX(_of->od_buffer_size-_of->od_buffer_pos,0); OP_ALWAYS_TRUE(!op_granpos_add(&gp,gp,-nbuffered)); /*We do _not_ add cur_discard_count to gp. Otherwise the total amount to discard could grow without bound, and it 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; + if(OP_LIKELY(!op_granpos_diff(&discard_count,target_gp,gp))){ /*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.*/ -- 2.15.1.424.g9478a66081-goog
James Zern
2017-Dec-07 23:25 UTC
[opus] [PATCH] op_fetch_and_process_page: fix int64 overflow
check for overflow with a negative diff --- src/opusfile.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/opusfile.c b/src/opusfile.c index 140503e..8b000a2 100644 --- a/src/opusfile.c +++ b/src/opusfile.c @@ -2078,7 +2078,11 @@ 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=durations[pi]+1; + } + 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 -- 2.15.1.424.g9478a66081-goog