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