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",*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

-- Sjoerd Mullender