Re: wddx serialize segmentation faults fixs.

On Jun 04, Gerrit Thomson wrote: > Hi Folks, > I managed to fix the segmantation fuatls in the wddx module when > serialising data. I am not sure of the desrialise operation at this > point. There is still possibly a problem with capacity but I am still > investigating it. > > My approach was to spot where there seemd to memory allocation problems. > In doing so I removed many uses of erealloc as a potential problem, > these may not be a problem, but the removal makes the code more > understandable. > > The other changes involved not relying on the len variable to hold the > current len of the buffer to be allocated. Instead the new allocation > size is determoined where possible form the current string length + the > length of the additional string + 1 for the string terminator. I found > many cases where the allowance for the terminator was left out or > accounted for by being "generous". > > the attached dif file should be usefull. A couple of comments -- 1. You included patches for doc/phpweb.dsl and doc/html.dsl. (Actually, those shouldn't even be in the tree any more, I think.) 2. I see you use "buf = sprintf("%s", str); str = buf;" in at least one spot. This is almost undoubtedly wrong. If there's something going wrong, it's real cause should be found and fixed. 3. You call strlen(var) after having just done a sprintf(lem, ...) -- you should instead use the return value of sprintf. 4. You're using sprintf instead of snprintf(), which means a possible buffer overflow. Same goes for strcpy(). These might theoretically bet safe because you've been careful in calculating the allocated string length, but better safe than sorry, I think. (And I believe we've tried to be fairly consistent in our use of snprintf/strncpy/strncat.) I don't want to sound like I'm picking on you at all, your patch for the problem is appreciated, especially since nobody else seems to be maintaining that code, but those flaws preclude me from commiting the patch to the repository. Jim > Cheers, > Gerrit Thomson. > Only in php-3.0.8-p: Makefile > Only in php-3.0.8-p: build-defs.h > Only in php-3.0.8-p: config.cache > Only in php-3.0.8-p: config.h > Only in php-3.0.8-p: config.log > Only in php-3.0.8-p: config.status > Only in php-3.0.8-p/dbase: Makefile > Only in php-3.0.8-p/doc: Makefile > Only in php-3.0.8-p/doc: checkdoc > diff -ru php-3.0.8/doc/html.dsl php-3.0.8-p/doc/html.dsl > --- php-3.0.8/doc/html.dsl Sun May 9 15:31:19 1999 > +++ php-3.0.8-p/doc/html.dsl Wed May 26 12:35:14 1999 > @@ -1,12 +1,12 @@ > <!DOCTYPE style-sheet PUBLIC "-//James Clark//DTD DSSSL Style Sheet//EN" [ > -<!ENTITY docbook.dsl SYSTEM "/usr/lib/dsssl/stylesheets/docbook/html/docbook.dsl" > CDATA DSSSL> > +<!ENTITY docbook.dsl SYSTEM > "/usr/lib/sgml/stylesheets/nwalsh-modular/html/docbook.dsl" CDATA DSSSL> > <!ENTITY html-common.dsl SYSTEM "html-common.dsl"> > <!ENTITY common.dsl SYSTEM "common.dsl"> > ]> > > <!-- > > - $Id: html.dsl,v 1.9 1999/05/09 05:31:19 eschmid Exp $ > + $Id: html.dsl.in,v 1.2 1999/05/17 03:13:39 ssb Exp $ > > HTML-specific stylesheet customization. > > diff -ru php-3.0.8/doc/phpweb.dsl php-3.0.8-p/doc/phpweb.dsl > --- php-3.0.8/doc/phpweb.dsl Fri Oct 23 21:36:33 1998 > +++ php-3.0.8-p/doc/phpweb.dsl Wed May 26 12:35:19 1999 > @@ -1,12 +1,12 @@ > <!DOCTYPE style-sheet PUBLIC "-//James Clark//DTD DSSSL Style Sheet//EN" [ > -<!ENTITY docbook.dsl SYSTEM "/usr/lib/sgml/docbook/html/docbook.dsl" CDATA > DSSSL> > +<!ENTITY docbook.dsl SYSTEM > "/usr/lib/sgml/stylesheets/nwalsh-modular/html/docbook.dsl" CDATA DSSSL> > <!ENTITY html-common.dsl SYSTEM "html-common.dsl"> > <!ENTITY common.dsl SYSTEM "common.dsl"> > ]> > > <!-- > > - $Id: phpweb.dsl,v 1.19 1998/10/23 11:36:33 ssb Exp $ > + $Id: phpweb.dsl.in,v 1.2 1999/05/17 03:13:39 ssb Exp $ > > HTML-specific stylesheet customization for use by the online manual. > > diff -ru php-3.0.8/doc/print.dsl php-3.0.8-p/doc/print.dsl > --- php-3.0.8/doc/print.dsl Wed Mar 11 14:13:57 1998 > +++ php-3.0.8-p/doc/print.dsl Wed May 26 12:35:16 1999 > @@ -1,11 +1,11 @@ > <!DOCTYPE style-sheet PUBLIC "-//James Clark//DTD DSSSL Style Sheet//EN" [ > -<!ENTITY docbook.dsl SYSTEM "/usr/lib/sgml/docbook/html/docbook.dsl" CDATA > DSSSL> > +<!ENTITY docbook.dsl SYSTEM > "/usr/lib/sgml/stylesheets/nwalsh-modular/print/docbook.dsl" CDATA DSSSL> > <!ENTITY common.dsl SYSTEM "common.dsl"> > ]> > > <!-- > > - $Id: print.dsl,v 1.3 1998/03/11 03:13:57 ssb Exp $ > + $Id: print.dsl.in,v 1.2 1999/05/17 03:13:39 ssb Exp $ > > This file contains printout-specific stylesheet customization. > > Only in php-3.0.8-p/doc: version.ent > Only in php-3.0.8-p/extra/gd: bdf2gdfont > diff -ru php-3.0.8/functions/wddx.c php-3.0.8-p/functions/wddx.c > --- php-3.0.8/functions/wddx.c Mon Jan 11 05:06:40 1999 > +++ php-3.0.8-p/functions/wddx.c Thu Jun 3 10:04:15 1999 > @@ -85,10 +85,15 @@ > > buf = emalloc(strlen(str) + strlen(name) + 30); > sprintf(buf, "<var name='%s'>%s</var>", name, str); > - efree(str); > + /* efree(str); */ > + str = buf; > + } else { > + char * buf; > + buf = emalloc(strlen(str)); > + sprintf(buf, "%s", str); > + /* efree(str); */ > str = buf; > } > - > return str; > } > /* }}} */ > @@ -128,21 +133,35 @@ > > buf = emalloc(len); > sprintf(buf, "<array length='%d'>", _php3_hash_num_elements(ht)); > + len = strlen( buf ); > > _php3_hash_internal_pointer_reset(ht); > while(_php3_hash_get_current_data(ht, (void **) &obj) == SUCCESS) { > new = _php3_wddx_build(NULL, obj); > - len += strlen(new); > - off = erealloc(buf, len); > + len = strlen(buf)+ strlen(new) + 1; > + off = emalloc( len ); > + strcpy( off, buf); > + efree(buf); > +/* off = erealloc(buf, len); > if(!off) { > efree(buf); > return empty_string; > - } > + } */ > buf = off; > strcat(buf, new); > efree(new); > _php3_hash_move_forward(ht); > } > + len = strlen(buf) + strlen("</array>") + 1; > + off = emalloc( len ); > + strcpy( off, buf ); > + efree( buf ); > +/* off = erealloc(buf, len); > + if(!off) { > + efree(buf); > + return empty_string; > + } */ > + buf = off; > strcat(buf, "</array>"); > > return buf; > @@ -172,17 +191,30 @@ > } > new = _php3_wddx_build(key, obj); > efree(key); > - len += strlen(new); > - off = erealloc(buf, len); > + len = strlen(buf) + strlen(new) +1; > + off = emalloc( len ); > + strcpy( off, buf ); > + efree( buf ); > +/* off = erealloc(buf, len); > if(!off) { > efree(buf); > return empty_string; > - } > + } */ > buf = off; > strcat(buf, new); > efree(new); > _php3_hash_move_forward(ht); > } > + len = strlen(buf) + strlen("</struct>") +1; > + off = emalloc( len ); > + strcpy( off, buf ); > + efree( buf ); > +/* off = erealloc(buf, len); > + if(!off) { > + efree(buf); > + return empty_string; > + } */ > + buf = off; > strcat(buf, "</struct>"); > > return buf; > @@ -267,6 +299,7 @@ > int freename; > char *str = NULL; > char *new; > + char *buf; > char *name; > pval *obj; > > @@ -290,9 +323,12 @@ > freename = 0; > } > new = _php3_wddx_build(name, obj); > - len += strlen(new); > if(str) { > - str = erealloc(str, len + 1); > + len = strlen(str) + strlen(new) + 1; > + buf = emalloc( len ); > + strcpy( buf, str ); > + str=buf; > + /* str = erealloc(str, len + 1); */ > strcat(str, new); > efree(new); > } else { > diff -ru php-3.0.8/functions/wddx_a.c php-3.0.8-p/functions/wddx_a.c > --- php-3.0.8/functions/wddx_a.c Wed Mar 10 04:49:56 1999 > +++ php-3.0.8-p/functions/wddx_a.c Thu Jun 3 11:19:11 1999 > @@ -27,7 +27,7 @@ > +----------------------------------------------------------------------+ > */ > > -/* $Id: wddx_a.c,v 1.4 1999/03/09 17:49:56 rasmus Exp $ */ > +/* $Id: wddx_a.c,v 1.6 1999/05/24 18:08:53 sas Exp $ */ > > #include "php.h" > #include "internal_functions.h" > @@ -245,11 +245,19 @@ > { > char **chunk; > char *buf; > + int len; > + char *off; > > buf = (char *)emalloc(packet->packet_length+1); > + *buf = '\0'; > for(chunk=dlst_first(packet->packet_head); > chunk!=NULL; > chunk = dlst_next(chunk)) { > + len = strlen( buf ) + strlen ( *chunk ) +1; > + off = emalloc( len ); > + strcpy( off, buf ); > + efree( buf ); > + buf = off; > strcat(buf, *chunk); > } > > @@ -296,7 +304,7 @@ > _php3_wddx_add_chunk(packet, WDDX_STRING_S); > > i = 0; > - buf = (char *)emalloc(var->value.str.len); > + buf = (char *)emalloc(var->value.str.len + 1); > for(c=var->value.str.val; *c!='\0'; c++) > { > if (iscntrl((int)*c)) > @@ -644,23 +652,8 @@ > char *buf; > > argc = ARG_COUNT(ht); > - switch (argc) > - { > - case 1: > - if (getParameters(ht, 1, &var) == FAILURE) { > - RETURN_FALSE; > - } > - break; > - > - case 2: > - if (getParameters(ht, 1, &var, &comment) == FAILURE) { > - RETURN_FALSE; > - } > - break; > - > - default: > - WRONG_PARAM_COUNT; > - break; > + if(argc < 1 || argc > 2 || getParameters(ht, argc, &var, &comment) == FAILURE) > { > + WRONG_PARAM_COUNT; > } > > packet = emalloc(sizeof(wddx_packet)); > Only in php-3.0.8-p: libphp3.module > Only in php-3.0.8-p/regex: Makefile > Only in php-3.0.8-p: stamp-h