Correct oggpack_writecopy bug reported by Ian Nartowicz: Integer

overflow checking in oggpack_writecopy_helper got the reallocation
size test condition backwards and so would error out when it needed to
expand the destination's internal buffer.

At the same time, do preexpansion of both aligned and unaligned copies
to avoid possible heap thrashing in the unaligned case.

Add black and glass box unit tests for oggpack_writecopy and
oggpackB_writecopy.


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

1 files changed, 246 insertions, 15 deletions

diff --git a/src/bitwise.c b/src/bitwise.c
index eea7858..ab2e58c 100644
--- a/src/bitwise.c
+++ b/src/bitwise.c
@@ -187,8 +187,22 @@ static void oggpack_writecopy_helper(oggpack_buffer *b,
   unsigned char *ptr=(unsigned char *)source;
 
   long bytes=bits/8;
+  long pbytes=(b->endbit+bits)/8;
   bits-=bytes*8;
 
+  /* expand storage up-front */
+  if(b->endbyte+pbytes>=b->storage){
+    void *ret;
+    if(!b->ptr) goto err;
+    if(b->storage>b->endbyte+pbytes+BUFFER_INCREMENT) goto err;
+    b->storage=b->endbyte+pbytes+BUFFER_INCREMENT;
+    ret=_ogg_realloc(b->buffer,b->storage);
+    if(!ret) goto err;
+    b->buffer=ret;
+    b->ptr=b->buffer+b->endbyte;
+  }
+
+  /* copy whole octets */
   if(b->endbit){
     int i;
     /* unaligned copy.  Do it the hard way. */
@@ -196,23 +210,13 @@ static void oggpack_writecopy_helper(oggpack_buffer *b,
       w(b,(unsigned long)(ptr[i]),8);
   }else{
     /* aligned block copy */
-    if(b->endbyte+bytes+1>=b->storage){
-      void *ret;
-      if(!b->ptr) goto err;
-      if(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) goto err;
-      b->buffer=ret;
-      b->ptr=b->buffer+b->endbyte;
-    }
-
     memmove(b->ptr,source,bytes);
     b->ptr+=bytes;
     b->endbyte+=bytes;
     *b->ptr=0;
-
   }
+
+  /* copy trailing bits */
   if(bits){
     if(msb)
       w(b,(unsigned long)(ptr[bytes]>>(8-bits)),bits);
@@ -613,9 +617,190 @@ void cliptestB(unsigned long *b,int vals,int bits,int *comp,int compsize){
   if(oggpackB_bytes(&r)!=bytes)report("leftover bytes after read!\n");
 }
 
+void copytest(int prefill, int copy){
+  oggpack_buffer source_write;
+  oggpack_buffer dest_write;
+  oggpack_buffer source_read;
+  oggpack_buffer dest_read;
+  unsigned char *source;
+  unsigned char *dest;
+  long source_bytes,dest_bytes;
+  int i;
+
+  oggpack_writeinit(&source_write);
+  oggpack_writeinit(&dest_write);
+
+  for(i=0;i<(prefill+copy+7)/8;i++)
+    oggpack_write(&source_write,(i^0x5a)&0xff,8);
+  source=oggpack_get_buffer(&source_write);
+  source_bytes=oggpack_bytes(&source_write);
+
+  /* prefill */
+  oggpack_writecopy(&dest_write,source,prefill);
+
+  /* check buffers; verify end byte masking */
+  dest=oggpack_get_buffer(&dest_write);
+  dest_bytes=oggpack_bytes(&dest_write);
+  if(dest_bytes!=(prefill+7)/8){
+    fprintf(stderr,"wrong number of bytes after prefill! %d!=%d\n",dest_bytes,(prefill+7)/8);
+    exit(1);
+  }
+  oggpack_readinit(&source_read,source,source_bytes);
+  oggpack_readinit(&dest_read,dest,dest_bytes);
+
+  for(i=0;i<prefill;i+=8){
+    int s=oggpack_read(&source_read,prefill-i<8?prefill-i:8);
+    int d=oggpack_read(&dest_read,prefill-i<8?prefill-i:8);
+    if(s!=d){
+      fprintf(stderr,"prefill=%d mismatch! byte %d, %x!=%x\n",prefill,i/8,s,d);
+      exit(1);
+    }
+  }
+  if(prefill<dest_bytes){
+    if(oggpack_read(&dest_read,dest_bytes-prefill)!=0){
+      fprintf(stderr,"prefill=%d mismatch! trailing bits not zero\n",prefill);
+      exit(1);
+    }
+  }
+
+  /* second copy */
+  oggpack_writecopy(&dest_write,source,copy);
+
+  /* check buffers; verify end byte masking */
+  dest=oggpack_get_buffer(&dest_write);
+  dest_bytes=oggpack_bytes(&dest_write);
+  if(dest_bytes!=(copy+prefill+7)/8){
+    fprintf(stderr,"wrong number of bytes after prefill+copy! %d!=%d\n",dest_bytes,(copy+prefill+7)/8);
+    exit(1);
+  }
+  oggpack_readinit(&source_read,source,source_bytes);
+  oggpack_readinit(&dest_read,dest,dest_bytes);
+
+  for(i=0;i<prefill;i+=8){
+    int s=oggpack_read(&source_read,prefill-i<8?prefill-i:8);
+    int d=oggpack_read(&dest_read,prefill-i<8?prefill-i:8);
+    if(s!=d){
+      fprintf(stderr,"prefill=%d mismatch! byte %d, %x!=%x\n",prefill,i/8,s,d);
+      exit(1);
+    }
+  }
+
+  oggpack_readinit(&source_read,source,source_bytes);
+  for(i=0;i<copy;i+=8){
+    int s=oggpack_read(&source_read,copy-i<8?copy-i:8);
+    int d=oggpack_read(&dest_read,copy-i<8?copy-i:8);
+    if(s!=d){
+      fprintf(stderr,"prefill=%d copy=%d mismatch! byte %d, %x!=%x\n",prefill,copy,i/8,s,d);
+      exit(1);
+    }
+  }
+
+  if(copy+prefill<dest_bytes){
+    if(oggpack_read(&dest_read,dest_bytes-copy-prefill)!=0){
+      fprintf(stderr,"prefill=%d copy=%d mismatch! trailing bits not zero\n",prefill,copy);
+      exit(1);
+    }
+  }
+
+  oggpack_writeclear(&source_write);
+  oggpack_writeclear(&dest_write);
+
+
+}
+
+void copytestB(int prefill, int copy){
+  oggpack_buffer source_write;
+  oggpack_buffer dest_write;
+  oggpack_buffer source_read;
+  oggpack_buffer dest_read;
+  unsigned char *source;
+  unsigned char *dest;
+  long source_bytes,dest_bytes;
+  int i;
+
+  oggpackB_writeinit(&source_write);
+  oggpackB_writeinit(&dest_write);
+
+  for(i=0;i<(prefill+copy+7)/8;i++)
+    oggpackB_write(&source_write,(i^0x5a)&0xff,8);
+  source=oggpackB_get_buffer(&source_write);
+  source_bytes=oggpackB_bytes(&source_write);
+
+  /* prefill */
+  oggpackB_writecopy(&dest_write,source,prefill);
+
+  /* check buffers; verify end byte masking */
+  dest=oggpackB_get_buffer(&dest_write);
+  dest_bytes=oggpackB_bytes(&dest_write);
+  if(dest_bytes!=(prefill+7)/8){
+    fprintf(stderr,"wrong number of bytes after prefill! %d!=%d\n",dest_bytes,(prefill+7)/8);
+    exit(1);
+  }
+  oggpackB_readinit(&source_read,source,source_bytes);
+  oggpackB_readinit(&dest_read,dest,dest_bytes);
+
+  for(i=0;i<prefill;i+=8){
+    int s=oggpackB_read(&source_read,prefill-i<8?prefill-i:8);
+    int d=oggpackB_read(&dest_read,prefill-i<8?prefill-i:8);
+    if(s!=d){
+      fprintf(stderr,"prefill=%d mismatch! byte %d, %x!=%x\n",prefill,i/8,s,d);
+      exit(1);
+    }
+  }
+  if(prefill<dest_bytes){
+    if(oggpackB_read(&dest_read,dest_bytes-prefill)!=0){
+      fprintf(stderr,"prefill=%d mismatch! trailing bits not zero\n",prefill);
+      exit(1);
+    }
+  }
+
+  /* second copy */
+  oggpackB_writecopy(&dest_write,source,copy);
+
+  /* check buffers; verify end byte masking */
+  dest=oggpackB_get_buffer(&dest_write);
+  dest_bytes=oggpackB_bytes(&dest_write);
+  if(dest_bytes!=(copy+prefill+7)/8){
+    fprintf(stderr,"wrong number of bytes after prefill+copy! %d!=%d\n",dest_bytes,(copy+prefill+7)/8);
+    exit(1);
+  }
+  oggpackB_readinit(&source_read,source,source_bytes);
+  oggpackB_readinit(&dest_read,dest,dest_bytes);
+
+  for(i=0;i<prefill;i+=8){
+    int s=oggpackB_read(&source_read,prefill-i<8?prefill-i:8);
+    int d=oggpackB_read(&dest_read,prefill-i<8?prefill-i:8);
+    if(s!=d){
+      fprintf(stderr,"prefill=%d mismatch! byte %d, %x!=%x\n",prefill,i/8,s,d);
+      exit(1);
+    }
+  }
+
+  oggpackB_readinit(&source_read,source,source_bytes);
+  for(i=0;i<copy;i+=8){
+    int s=oggpackB_read(&source_read,copy-i<8?copy-i:8);
+    int d=oggpackB_read(&dest_read,copy-i<8?copy-i:8);
+    if(s!=d){
+      fprintf(stderr,"prefill=%d copy=%d mismatch! byte %d, %x!=%x\n",prefill,copy,i/8,s,d);
+      exit(1);
+    }
+  }
+
+  if(copy+prefill<dest_bytes){
+    if(oggpackB_read(&dest_read,dest_bytes-copy-prefill)!=0){
+      fprintf(stderr,"prefill=%d copy=%d mismatch! trailing bits not zero\n",prefill,copy);
+      exit(1);
+    }
+  }
+
+  oggpackB_writeclear(&source_write);
+  oggpackB_writeclear(&dest_write);
+
+}
+
 int main(void){
   unsigned char *buffer;
-  long bytes,i;
+  long bytes,i,j;
   static unsigned long testbuffer1[]=
     {18,12,103948,4325,543,76,432,52,3,65,4,56,32,42,34,21,1,23,32,546,456,7,
        567,56,8,8,55,3,52,342,341,4,265,7,67,86,2199,21,7,1,5,1,4};
@@ -761,7 +946,31 @@ int main(void){
       exit(1);
   }
   oggpack_writeclear(&o);
-  fprintf(stderr,"ok.\n");
+  fprintf(stderr,"ok.");
+
+  /* this is partly glassbox; we're mostly concerned about the allocation boundaries */
+
+  fprintf(stderr,"\nTesting aligned writecopies (LSb): ");
+  for(i=0;i<71;i++)
+    for(j=0;j<5;j++)
+      copytest(j*8,i);
+  for(i=BUFFER_INCREMENT*8-71;i<BUFFER_INCREMENT*8+71;i++)
+    for(j=0;j<5;j++)
+      copytest(j*8,i);
+  fprintf(stderr,"ok.      ");
+
+  fprintf(stderr,"\nTesting unaligned writecopies (LSb): ");
+  for(i=0;i<71;i++)
+    for(j=1;j<40;j++)
+      if(j&0x7)
+        copytest(j,i);
+  for(i=BUFFER_INCREMENT*8-71;i<BUFFER_INCREMENT*8+71;i++)
+    for(j=1;j<40;j++)
+      if(j&0x7)
+        copytest(j,i);
+  
+  fprintf(stderr,"ok.      \n");
+
 
   /********** lazy, cut-n-paste retest with MSb packing ***********/
 
@@ -846,9 +1055,31 @@ int main(void){
     fprintf(stderr,"failed; read past end without -1.\n");
       exit(1);
   }
+  fprintf(stderr,"ok.");
   oggpackB_writeclear(&o);
-  fprintf(stderr,"ok.\n\n");
 
+  /* this is partly glassbox; we're mostly concerned about the allocation boundaries */
+
+  fprintf(stderr,"\nTesting aligned writecopies (MSb): ");
+  for(i=0;i<71;i++)
+    for(j=0;j<5;j++)
+      copytestB(j*8,i);
+  for(i=BUFFER_INCREMENT*8-71;i<BUFFER_INCREMENT*8+71;i++)
+    for(j=0;j<5;j++)
+      copytestB(j*8,i);
+  fprintf(stderr,"ok.      ");
+
+  fprintf(stderr,"\nTesting unaligned writecopies (MSb): ");
+  for(i=0;i<71;i++)
+    for(j=1;j<40;j++)
+      if(j&0x7)
+        copytestB(j,i);
+  for(i=BUFFER_INCREMENT*8-71;i<BUFFER_INCREMENT*8+71;i++)
+    for(j=1;j<40;j++)
+      if(j&0x7)
+        copytestB(j,i);
+  
+  fprintf(stderr,"ok.      \n\n");
 
   return(0);
 }