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

Attachment: signature.asc
Description: OpenPGP digital signature

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Monetdb-developers mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/monetdb-developers

Reply via email to