From 3bbd3ac3c4effc7a6070e757b7ab10f02a1b90f5 Mon Sep 17 00:00:00 2001 From: Monty Date: Fri, 4 Jun 2010 05:19:12 +0000 Subject: 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. git-svn-id: http://svn.xiph.org/trunk/ogg@17268 0101bb08-14d6-0310-b084-bc0e0c8e3800 --- src/bitwise.c | 146 ++++++++++++++++++++++++++++++++++++---------------------- 1 file changed, 90 insertions(+), 56 deletions(-) (limited to 'src') 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_INCREMENTstorage) 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_INCREMENTstorage) 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){ -- cgit v1.2.1