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

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

    