Re: [libav-devel] [PATCH 3/5] escape124: fix integer overflow leading to excessive memory allocation

2012-10-19 Thread Reinhard Tartler 
On Wed, Oct 17, 2012 at 11:59 PM, Måns Rullgård m...@mansr.com wrote:
 Reinhard Tartler siret...@gmail.com writes:

 On Wed, Oct 17, 2012 at 11:39 PM, Måns Rullgård m...@mansr.com wrote:
 Reinhard Tartler siret...@tauware.de writes:

 From: Michael Niedermayer michae...@gmx.at

 Fixes Ticket1629

 Signed-off-by: Michael Niedermayer michae...@gmx.at
 (cherry picked from commit 3d7817048cb387de87600f2152075f78b37b60a6)

 Signed-off-by: Reinhard Tartler siret...@tauware.de
 ---
  libavcodec/escape124.c |4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

 diff --git a/libavcodec/escape124.c b/libavcodec/escape124.c
 index 40224fb..c04af69 100644
 --- a/libavcodec/escape124.c
 +++ b/libavcodec/escape124.c
 @@ -48,7 +48,7 @@ typedef struct Escape124Context {
  CodeBook codebooks[3];
  } Escape124Context;

 -static int can_safely_read(GetBitContext* gb, int bits) {
 +static int can_safely_read(GetBitContext* gb, uint64_t bits) {
  return get_bits_left(gb) = bits;
  }

 @@ -89,7 +89,7 @@ static CodeBook unpack_codebook(GetBitContext* gb, 
 unsigned depth,
  unsigned i, j;
  CodeBook cb = { 0 };

 -if (!can_safely_read(gb, size * 34))
 +if (!can_safely_read(gb, size * 34L))
  return cb;

 Would this be a better condition then:

 if (size=UINT_MAX/34 || !can_safely_read(gb, size * 34U))
   return cb;

 size is unsigned so the U suffix isn't needed.

 Moreover, I wonder if the can_safely_read() thing is even needed in most
 places now that we have the safe bitstream reader.  Certainly some of
 them are unnecessary.


Would the following do then?

diff --git a/libavcodec/escape124.c b/libavcodec/escape124.c
index 40224fb..5d050dc 100644
--- a/libavcodec/escape124.c
+++ b/libavcodec/escape124.c
@@ -48,7 +48,7 @@ typedef struct Escape124Context {
 CodeBook codebooks[3];
 } Escape124Context;

-static int can_safely_read(GetBitContext* gb, int bits) {
+static int can_safely_read(GetBitContext* gb, uint64_t bits) {
 return get_bits_left(gb) = bits;
 }

@@ -89,7 +89,7 @@ static CodeBook unpack_codebook(GetBitContext* gb,
unsigned depth,
 unsigned i, j;
 CodeBook cb = { 0 };

-if (!can_safely_read(gb, size * 34))
+if (size=UINT_MAX/34 || !can_safely_read(gb, size * 34))
 return cb;

 if (size = INT_MAX / sizeof(MacroBlock))



-- 
regards,
Reinhard
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH 3/5] escape124: fix integer overflow leading to excessive memory allocation

2012-10-19 Thread Måns Rullgård 
Reinhard Tartler siret...@gmail.com writes:

 Would the following do then?

 diff --git a/libavcodec/escape124.c b/libavcodec/escape124.c
 index 40224fb..5d050dc 100644
 --- a/libavcodec/escape124.c
 +++ b/libavcodec/escape124.c
 @@ -48,7 +48,7 @@ typedef struct Escape124Context {
  CodeBook codebooks[3];
  } Escape124Context;

 -static int can_safely_read(GetBitContext* gb, int bits) {
 +static int can_safely_read(GetBitContext* gb, uint64_t bits) {
  return get_bits_left(gb) = bits;
  }

 @@ -89,7 +89,7 @@ static CodeBook unpack_codebook(GetBitContext* gb,
 unsigned depth,
  unsigned i, j;
  CodeBook cb = { 0 };

 -if (!can_safely_read(gb, size * 34))
 +if (size=UINT_MAX/34 || !can_safely_read(gb, size * 34))
  return cb;

If you check against overflow here there is no need to make the argument
64-bit.  In fact, in this form doing so has no effect at all since the
overflow would have already happened before the 64-bit conversion.

-- 
Måns Rullgård
m...@mansr.com
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

[libav-devel] [PATCH 3/5] escape124: fix integer overflow leading to excessive memory allocation

2012-10-17 Thread Reinhard Tartler 
From: Michael Niedermayer michae...@gmx.at

Fixes Ticket1629

Signed-off-by: Michael Niedermayer michae...@gmx.at
(cherry picked from commit 3d7817048cb387de87600f2152075f78b37b60a6)

Signed-off-by: Reinhard Tartler siret...@tauware.de
---
 libavcodec/escape124.c |4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libavcodec/escape124.c b/libavcodec/escape124.c
index 40224fb..c04af69 100644
--- a/libavcodec/escape124.c
+++ b/libavcodec/escape124.c
@@ -48,7 +48,7 @@ typedef struct Escape124Context {
 CodeBook codebooks[3];
 } Escape124Context;
 
-static int can_safely_read(GetBitContext* gb, int bits) {
+static int can_safely_read(GetBitContext* gb, uint64_t bits) {
 return get_bits_left(gb) = bits;
 }
 
@@ -89,7 +89,7 @@ static CodeBook unpack_codebook(GetBitContext* gb, unsigned 
depth,
 unsigned i, j;
 CodeBook cb = { 0 };
 
-if (!can_safely_read(gb, size * 34))
+if (!can_safely_read(gb, size * 34L))
 return cb;
 
 if (size = INT_MAX / sizeof(MacroBlock))
-- 
1.7.9.5

___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH 3/5] escape124: fix integer overflow leading to excessive memory allocation

2012-10-17 Thread Måns Rullgård 
Reinhard Tartler siret...@tauware.de writes:

 From: Michael Niedermayer michae...@gmx.at

 Fixes Ticket1629

 Signed-off-by: Michael Niedermayer michae...@gmx.at
 (cherry picked from commit 3d7817048cb387de87600f2152075f78b37b60a6)

 Signed-off-by: Reinhard Tartler siret...@tauware.de
 ---
  libavcodec/escape124.c |4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

 diff --git a/libavcodec/escape124.c b/libavcodec/escape124.c
 index 40224fb..c04af69 100644
 --- a/libavcodec/escape124.c
 +++ b/libavcodec/escape124.c
 @@ -48,7 +48,7 @@ typedef struct Escape124Context {
  CodeBook codebooks[3];
  } Escape124Context;

 -static int can_safely_read(GetBitContext* gb, int bits) {
 +static int can_safely_read(GetBitContext* gb, uint64_t bits) {
  return get_bits_left(gb) = bits;
  }

 @@ -89,7 +89,7 @@ static CodeBook unpack_codebook(GetBitContext* gb, unsigned 
 depth,
  unsigned i, j;
  CodeBook cb = { 0 };

 -if (!can_safely_read(gb, size * 34))
 +if (!can_safely_read(gb, size * 34L))
  return cb;

This is wrong.  The L suffix has no effect on 32-bit systems.

-- 
Måns Rullgård
m...@mansr.com
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH 3/5] escape124: fix integer overflow leading to excessive memory allocation

2012-10-17 Thread Reinhard Tartler 
On Wed, Oct 17, 2012 at 11:39 PM, Måns Rullgård m...@mansr.com wrote:
 Reinhard Tartler siret...@tauware.de writes:

 From: Michael Niedermayer michae...@gmx.at

 Fixes Ticket1629

 Signed-off-by: Michael Niedermayer michae...@gmx.at
 (cherry picked from commit 3d7817048cb387de87600f2152075f78b37b60a6)

 Signed-off-by: Reinhard Tartler siret...@tauware.de
 ---
  libavcodec/escape124.c |4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

 diff --git a/libavcodec/escape124.c b/libavcodec/escape124.c
 index 40224fb..c04af69 100644
 --- a/libavcodec/escape124.c
 +++ b/libavcodec/escape124.c
 @@ -48,7 +48,7 @@ typedef struct Escape124Context {
  CodeBook codebooks[3];
  } Escape124Context;

 -static int can_safely_read(GetBitContext* gb, int bits) {
 +static int can_safely_read(GetBitContext* gb, uint64_t bits) {
  return get_bits_left(gb) = bits;
  }

 @@ -89,7 +89,7 @@ static CodeBook unpack_codebook(GetBitContext* gb, 
 unsigned depth,
  unsigned i, j;
  CodeBook cb = { 0 };

 -if (!can_safely_read(gb, size * 34))
 +if (!can_safely_read(gb, size * 34L))
  return cb;

Would this be a better condition then:

if (size=UINT_MAX/34 || !can_safely_read(gb, size * 34U))
  return cb;

?


-- 
regards,
Reinhard
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH 3/5] escape124: fix integer overflow leading to excessive memory allocation

2012-10-17 Thread Måns Rullgård 
Reinhard Tartler siret...@gmail.com writes:

 On Wed, Oct 17, 2012 at 11:39 PM, Måns Rullgård m...@mansr.com wrote:
 Reinhard Tartler siret...@tauware.de writes:

 From: Michael Niedermayer michae...@gmx.at

 Fixes Ticket1629

 Signed-off-by: Michael Niedermayer michae...@gmx.at
 (cherry picked from commit 3d7817048cb387de87600f2152075f78b37b60a6)

 Signed-off-by: Reinhard Tartler siret...@tauware.de
 ---
  libavcodec/escape124.c |4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

 diff --git a/libavcodec/escape124.c b/libavcodec/escape124.c
 index 40224fb..c04af69 100644
 --- a/libavcodec/escape124.c
 +++ b/libavcodec/escape124.c
 @@ -48,7 +48,7 @@ typedef struct Escape124Context {
  CodeBook codebooks[3];
  } Escape124Context;

 -static int can_safely_read(GetBitContext* gb, int bits) {
 +static int can_safely_read(GetBitContext* gb, uint64_t bits) {
  return get_bits_left(gb) = bits;
  }

 @@ -89,7 +89,7 @@ static CodeBook unpack_codebook(GetBitContext* gb, 
 unsigned depth,
  unsigned i, j;
  CodeBook cb = { 0 };

 -if (!can_safely_read(gb, size * 34))
 +if (!can_safely_read(gb, size * 34L))
  return cb;

 Would this be a better condition then:

 if (size=UINT_MAX/34 || !can_safely_read(gb, size * 34U))
   return cb;

size is unsigned so the U suffix isn't needed.

Moreover, I wonder if the can_safely_read() thing is even needed in most
places now that we have the safe bitstream reader.  Certainly some of
them are unnecessary.

-- 
Måns Rullgård
m...@mansr.com
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel