Complete code review on the pattern:

 if(b->endbyte*8+bits>b->storage*8)goto overflow;

Eliminate the possibility of b->endbyte overflow on buffer storage near or 
exactly at long storage limit; corrections made to both read and write.

Also, harden both read and write against requesting <0 or >32 read/write.  In both 
case, the packer is put into 'error' state.


svn path=/trunk/ogg/; revision=17268

1 files changed, 90 insertions, 56 deletions

diff --git a/src/bitwise.c b/src/bitwise.c
index a0a9076..48b7192 100644
--- a/src/bitwise.c
+++ b/src/bitwise.c
@@ -80,14 +80,13 @@ void oggpackB_writetrunc(oggpack_buffer *b,long bits){
 
 /* Takes only up to 32 bits. */
 void oggpack_write(oggpack_buffer *b,unsigned long value,int bits){
-  if(b->endbyte+4>=b->storage){
+  if(bits<0 || bits>32) goto err;
+  if(b->endbyte>=b->storage-4){
     void *ret;
     if(!b->ptr)return;
+    if(b->storage+BUFFER_INCREMENT<b->storage) goto err;
     ret=_ogg_realloc(b->buffer,b->storage+BUFFER_INCREMENT);
-    if(!ret){
-      oggpack_writeclear(b);
-      return;
-    }
+    if(!ret) goto err;
     b->buffer=ret;
     b->storage+=BUFFER_INCREMENT;
     b->ptr=b->buffer+b->endbyte;
@@ -117,18 +116,20 @@ void oggpack_write(oggpack_buffer *b,unsigned long value,int bits){
   b->endbyte+=bits/8;
   b->ptr+=bits/8;
   b->endbit=bits&7;
+  return;
+ err:
+  oggpack_writeclear(b);
 }
 
 /* Takes only up to 32 bits. */
 void oggpackB_write(oggpack_buffer *b,unsigned long value,int bits){
-  if(b->endbyte+4>=b->storage){
+  if(bits<0 || bits>32) goto err;
+  if(b->endbyte>=b->storage-4){
     void *ret;
     if(!b->ptr)return;
+    if(b->storage+BUFFER_INCREMENT<b->storage) goto err;
     ret=_ogg_realloc(b->buffer,b->storage+BUFFER_INCREMENT);
-    if(!ret){
-      oggpack_writeclear(b);
-      return;
-    }
+    if(!ret) goto err;
     b->buffer=ret;
     b->storage+=BUFFER_INCREMENT;
     b->ptr=b->buffer+b->endbyte;
@@ -158,6 +159,9 @@ void oggpackB_write(oggpack_buffer *b,unsigned long value,int bits){
   b->endbyte+=bits/8;
   b->ptr+=bits/8;
   b->endbit=bits&7;
+  return;
+ err:
+  oggpack_writeclear(b);
 }
 
 void oggpack_writealign(oggpack_buffer *b){
@@ -193,13 +197,11 @@ static void oggpack_writecopy_helper(oggpack_buffer *b,
     /* aligned block copy */
     if(b->endbyte+bytes+1>=b->storage){
       void *ret;
-      if(!b->ptr)return;
+      if(!b->ptr) goto err;
+      if(b->storage=b->endbyte+bytes+BUFFER_INCREMENT>b->storage) goto err;
       b->storage=b->endbyte+bytes+BUFFER_INCREMENT;
       ret=_ogg_realloc(b->buffer,b->storage);
-      if(!ret){
-        oggpack_writeclear(b);
-        return;
-      }
+      if(!ret) goto err;
       b->buffer=ret;
       b->ptr=b->buffer+b->endbyte;
     }
@@ -216,6 +218,9 @@ static void oggpack_writecopy_helper(oggpack_buffer *b,
     else
       w(b,(unsigned long)(ptr[bytes]),bits);    
   }
+  return;
+ err:
+  oggpack_writeclear(b);
 }
 
 void oggpack_writecopy(oggpack_buffer *b,void *source,long bits){
@@ -259,15 +264,20 @@ void oggpackB_readinit(oggpack_buffer *b,unsigned char *buf,int bytes){
 /* Read in bits without advancing the bitptr; bits <= 32 */
 long oggpack_look(oggpack_buffer *b,int bits){
   unsigned long ret;
-  unsigned long m=mask[bits];
+  unsigned long m;
 
+  if(bits<0 || bits>32) return -1;
+  m=mask[bits];
   bits+=b->endbit;
 
-  if(b->endbyte+4>=b->storage){
+  if(b->endbyte >= b->storage-4){
     /* not the main path */
-    if(b->endbyte*8+bits>b->storage*8)return(-1);
+    if(b->endbyte > b->storage-((bits+7)>>3)) return -1;
+    /* special case to avoid reading b->ptr[0], which might be past the end of
+        the buffer; also skips some useless accounting */
+    else if(!bits)return(0L);
   }
-  
+
   ret=b->ptr[0]>>b->endbit;
   if(bits>8){
     ret|=b->ptr[1]<<(8-b->endbit);  
@@ -288,13 +298,17 @@ long oggpackB_look(oggpack_buffer *b,int bits){
   unsigned long ret;
   int m=32-bits;
 
+  if(m<0 || m>32) return -1;
   bits+=b->endbit;
 
-  if(b->endbyte+4>=b->storage){
+  if(b->endbyte >= b->storage-4){
     /* not the main path */
-    if(b->endbyte*8+bits>b->storage*8)return(-1);
+    if(b->endbyte > b->storage-((bits+7)>>3)) return -1;
+    /* special case to avoid reading b->ptr[0], which might be past the end of
+        the buffer; also skips some useless accounting */
+    else if(!bits)return(0L);
   }
-  
+
   ret=b->ptr[0]<<(24+b->endbit);
   if(bits>8){
     ret|=b->ptr[1]<<(16+b->endbit);  
@@ -322,9 +336,18 @@ long oggpackB_look1(oggpack_buffer *b){
 
 void oggpack_adv(oggpack_buffer *b,int bits){
   bits+=b->endbit;
+
+  if(b->endbyte > b->storage-((bits+7)>>3)) goto overflow;
+
   b->ptr+=bits/8;
   b->endbyte+=bits/8;
   b->endbit=bits&7;
+  return;
+
+ overflow:
+  b->ptr=NULL;
+  b->endbyte=b->storage;
+  b->endbit=1;
 }
 
 void oggpackB_adv(oggpack_buffer *b,int bits){
@@ -346,16 +369,20 @@ void oggpackB_adv1(oggpack_buffer *b){
 /* bits <= 32 */
 long oggpack_read(oggpack_buffer *b,int bits){
   long ret;
-  unsigned long m=mask[bits];
+  unsigned long m;
 
+  if(bits<0 || bits>32) goto err;
+  m=mask[bits];
   bits+=b->endbit;
 
-  if(b->endbyte+4>=b->storage){
+  if(b->endbyte >= b->storage-4){
     /* not the main path */
-    ret=-1L;
-    if(b->endbyte*8+bits>b->storage*8)goto overflow;
+    if(b->endbyte > b->storage-((bits+7)>>3)) goto overflow;
+    /* special case to avoid reading b->ptr[0], which might be past the end of
+        the buffer; also skips some useless accounting */
+    else if(!bits)return(0L);
   }
-  
+
   ret=b->ptr[0]>>b->endbit;
   if(bits>8){
     ret|=b->ptr[1]<<(8-b->endbit);  
@@ -370,31 +397,35 @@ long oggpack_read(oggpack_buffer *b,int bits){
     }
   }
   ret&=m;
-  
- overflow:
-
   b->ptr+=bits/8;
   b->endbyte+=bits/8;
   b->endbit=bits&7;
-  return(ret);
+  return ret;
+
+ overflow:
+ err:
+  b->ptr=NULL;
+  b->endbyte=b->storage;
+  b->endbit=1;
+  return -1L;
 }
 
 /* bits <= 32 */
 long oggpackB_read(oggpack_buffer *b,int bits){
   long ret;
   long m=32-bits;
-  
+
+  if(m<0 || m>32) goto err;
   bits+=b->endbit;
 
   if(b->endbyte+4>=b->storage){
     /* not the main path */
-    ret=-1L;
-    if(b->endbyte*8+bits>b->storage*8)goto overflow;
+    if(b->endbyte > b->storage-((bits+7)>>3)) goto overflow;
     /* special case to avoid reading b->ptr[0], which might be past the end of
         the buffer; also skips some useless accounting */
     else if(!bits)return(0L);
   }
-  
+
   ret=b->ptr[0]<<(24+b->endbit);
   if(bits>8){
     ret|=b->ptr[1]<<(16+b->endbit);  
@@ -408,27 +439,25 @@ long oggpackB_read(oggpack_buffer *b,int bits){
     }
   }
   ret=((ret&0xffffffffUL)>>(m>>1))>>((m+1)>>1);
-  
- overflow:
 
   b->ptr+=bits/8;
   b->endbyte+=bits/8;
   b->endbit=bits&7;
-  return(ret);
+  return ret;
+
+ overflow:
+ err:
+  b->ptr=NULL;
+  b->endbyte=b->storage;
+  b->endbit=1;
+  return -1L;
 }
 
 long oggpack_read1(oggpack_buffer *b){
   long ret;
-  
-  if(b->endbyte>=b->storage){
-    /* not the main path */
-    ret=-1L;
-    goto overflow;
-  }
 
+  if(b->endbyte >= b->storage) goto overflow;
   ret=(b->ptr[0]>>b->endbit)&1;
-  
- overflow:
 
   b->endbit++;
   if(b->endbit>7){
@@ -436,21 +465,20 @@ long oggpack_read1(oggpack_buffer *b){
     b->ptr++;
     b->endbyte++;
   }
-  return(ret);
+  return ret;
+
+ overflow:
+  b->ptr=NULL;
+  b->endbyte=b->storage;
+  b->endbit=1;
+  return -1L;
 }
 
 long oggpackB_read1(oggpack_buffer *b){
   long ret;
-  
-  if(b->endbyte>=b->storage){
-    /* not the main path */
-    ret=-1L;
-    goto overflow;
-  }
 
+  if(b->endbyte >= b->storage) goto overflow;
   ret=(b->ptr[0]>>(7-b->endbit))&1;
-  
- overflow:
 
   b->endbit++;
   if(b->endbit>7){
@@ -458,7 +486,13 @@ long oggpackB_read1(oggpack_buffer *b){
     b->ptr++;
     b->endbyte++;
   }
-  return(ret);
+  return ret;
+
+ overflow:
+  b->ptr=NULL;
+  b->endbyte=b->storage;
+  b->endbit=1;
+  return -1L;
 }
 
 long oggpack_bytes(oggpack_buffer *b){