[Monetdb-developers] [Monetdb-checkins] MonetDB5/src/modules/atoms xml.mx, , 1.21, 1.22

Sjoerd Mullender Sjoerd.Mullender at cwi.nl
Sun Aug 3 22:53:32 CEST 2008


When you "fix" code by hiding the problem, you're just doing that: 
hiding the problem.  In this case, you were hiding the problem by 
casting to a potentially smaller size which would potentially cause 
overflow.  The compiler warned about that, but you chose to ignore the 
warning.  My change was to prevent the overflow from occurring.  The 
code is in that sense more "correct".

It is true that this is probably still not the ultimate solution.  That 
would indeed require an investment of time that we're not currently 
wishing to do.  But at least, in the very unlikely event that a string 
would ever grow to > 2GiB in length then your version would crash 
whereas mine would struggle on (and presumably crash somewhere else). 
If it would ever get to this code in the first place.

But anyway, I don't like hiding potential problems, even if they are 
only a very remote potential.

By the way, there are now lots of places where we use size_t to hold 
string lengths that would never grow to sizes even remotely close to 2 
GiB.  But since strlen() returns a size_t and malloc and snprintf both 
take a size_t, it is easier to just use size_t.

On 2008-08-03 12:16, Martin Kersten wrote:
> Sjoerd Mullender wrote:
>> Update of /cvsroot/monetdb/MonetDB5/src/modules/atoms
>> In directory sc8-pr-cvs16.sourceforge.net:/tmp/cvs-serv1588
>>
>> Modified Files:
>> 	xml.mx 
>> Log Message:
>> By using the correct types, you loose a lot of casts...
>>   
> Point is that strings in GDK ValRecord are limited to int length.
> The consequence, for me, is that we should limit all string manipulation
> in the system to this size. I consider xml a subtype of str.
> 
> All places where we use or construct strings, we should check
> the size overflow or we have to live with the truncation semantics.
> Since such large large strings are (not yet) at the horizon
> and would cause problems way in before, we can live
> with a truncation policy.
> 
> A real solution requires many weeks of development investment
> for a case not eminent. Now the patch was aimed at removing the
> compiler complaints. All practical cases are well within the size limit.
>> U xml.mx
>> Index: xml.mx
>> ===================================================================
>> RCS file: /cvsroot/monetdb/MonetDB5/src/modules/atoms/xml.mx,v
>> retrieving revision 1.21
>> retrieving revision 1.22
>> diff -u -d -r1.21 -r1.22
>> --- xml.mx	3 Aug 2008 06:55:59 -0000	1.21
>> +++ xml.mx	3 Aug 2008 09:07:03 -0000	1.22
>> @@ -214,7 +214,7 @@
>>  
>>  str
>>  XMLcomment(xml *x, str *s){
>> -	int len = (int) strlen(*s) + 10;
>> +	size_t len = strlen(*s) + 10;
>>  	str buf = (str) GDKmalloc(len);
>>  
>>  	snprintf(buf, len, "<!-- %s -->", *s);
>> @@ -241,9 +241,9 @@
>>  str
>>  XMLroot(str *ret, str *val, str *version, str *standalone)
>>  {
>> -	int len;
>> -	str buf = (str) GDKmalloc(len= (int) strlen(*val) + 
>> -		(int) strlen("<? version=\"\" standalone=\"\"?>"));
>> +	size_t len;
>> +	str buf = (str) GDKmalloc(len= strlen(*val) + 
>> +		strlen("<? version=\"\" standalone=\"\"?>"));
>>  	snprintf(buf,len,"<? version=\"%s\" stanalone=\"%s\"?>%s",
>>  		*version,*standalone,*val);
>>  	*ret= buf;
>> @@ -253,8 +253,8 @@
>>  str
>>  XMLattribute(xml *ret, str *name, str *val)
>>  {
>> -	int len;
>> -	str buf= (str) GDKmalloc(len= (int)(2*strlen(*name)+strlen(*val)+5));
>> +	size_t len;
>> +	str buf= (str) GDKmalloc(len= (2*strlen(*name)+strlen(*val)+5));
>>  	snprintf(buf,len," %s=\"%s\"",*name,*val);
>>  	*ret = buf;
>>  	return MAL_SUCCEED;
>> @@ -264,8 +264,8 @@
>>  XMLelement(xml *ret, str *name, str *nspace, xml *attr, xml *Val)
>>  {
>>  	char *val = *Val;
>> -	int len;
>> -	str buf= (str) GDKmalloc(len=2* (int)(strlen(*name) +
>> +	size_t len;
>> +	str buf= (str) GDKmalloc(len=2* (strlen(*name) +
>>  				strlen(*nspace) + strlen(*attr)+
>>  				strlen("<></> ")+strlen(val)+1));
>>  	if (strNil(val))  /* todo short hand <elementname /> */
>> @@ -290,8 +290,8 @@
>>  str
>>  XMLelementSmall(xml *ret, str *name, xml *val)
>>  {
>> -	int len;
>> -	str buf= (str) GDKmalloc(len=2*(int) (strlen(*name) +
>> +	size_t len;
>> +	str buf= (str) GDKmalloc(len=2*(strlen(*name) +
>>  				strlen("<></> ")+strlen(*val)));
>>  	snprintf(buf,len,"<%s>%s</%s>",*name, *val, *name);
>>  	*ret= buf;
>> @@ -320,7 +320,7 @@
>>  	(void) cntxt;
>>  	for( i=p->retc; i<p->argc; i++)
>>  		len += strlen(*(str*)getArgReference(stk,p,i));
>> -	buf= (str) GDKmalloc( (int) (len+1));
>> +	buf= (str) GDKmalloc( len+1);
>>  	buf[0]=0;
>>  	
>>  	assert(len <INT_MAX);
>>


-- 
Sjoerd Mullender

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 369 bytes
Desc: OpenPGP digital signature
URL: <http://www.monetdb.org/pipermail/developers-list/attachments/20080803/b6f8634d/attachment.sig>


More information about the developers-list mailing list