2007-11-22 Tatsuhiro Tsujikawa <tujikawa at rednoah dot com>

Replaced strtol with Util::parseInt
	* src/ChunkedEncoding.cc
	* src/HttpConnection.cc
	* src/CookieBoxFactory.cc
	* src/ParameterizedStringParser.cc
	* src/Util.cc
	* test/UtilTest.cc
	* test/OptionHandlerTest.cc
	* src/Request.cc

	Throw exception when empty string is given. The message for 
exception
	changed.
	* src/Util.cc (parseInt)(parseLLInt)
	* src/message.h

ChangeLog

@@ -1,3 +1,20 @@
+2007-11-22  Tatsuhiro Tsujikawa  <tujikawa at rednoah dot com>
+
+	Replaced strtol with Util::parseInt
+	* src/ChunkedEncoding.cc
+	* src/HttpConnection.cc
+	* src/CookieBoxFactory.cc
+	* src/ParameterizedStringParser.cc
+	* src/Util.cc
+	* test/UtilTest.cc
+	* test/OptionHandlerTest.cc
+	* src/Request.cc
+
+	Throw exception when empty string is given. The message for exception
+	changed.
+	* src/Util.cc (parseInt)(parseLLInt)
+	* src/message.h
+
 2007-11-22  Tatsuhiro Tsujikawa  <tujikawa at rednoah dot com>
 
 	Set precision to 2 because share ratio is rounded into 1.0 if precision

TODO

@@ -53,7 +53,7 @@
     DlAbortEx .... Abort download with the connection/url. Should be renamed to PermanentFailureException
     DownloadFailureException .... RequestGroup should halt.
   FatalException .... Program should abort.
+* replace strtol with Util::parseInt
 
 -- remaining issues to be implemented for 0.12.0 release
 * Update translation
-* replace strtol with Util::parseInt

src/ChunkedEncoding.cc

@@ -35,6 +35,7 @@
 #include "ChunkedEncoding.h"
 #include "DlAbortEx.h"
 #include "message.h"
+#include "Util.h"
 #include <string.h>
 #include <strings.h>
 #include <errno.h>
@@ -155,23 +156,15 @@ int32_t ChunkedEncoding::readChunkSize(char** pp) {
     return -1;
   }
   p = rp;
-
   // We ignore chunk-extension
   char* exsp = (char*)memchr(*pp, ';', strbufTail-*pp);
-  if(exsp == NULL) {
+  if(exsp == 0 || p < exsp) {
     exsp = p;
   }
-  // TODO check invalid characters in buffer
-  char* temp = new char[exsp-*pp+1];
-  memcpy(temp, *pp, exsp-*pp);
-  temp[exsp-*pp] = '\0';
-
-  chunkSize = strtol(temp, NULL, 16);
-  delete [] temp;
+  string temp(*pp, exsp);
+  chunkSize = Util::parseInt(temp, 16);
   if(chunkSize < 0) {
     throw new DlAbortEx(EX_INVALID_CHUNK_SIZE);
-  } else if(errno == ERANGE && (chunkSize == INT32_MAX || chunkSize == INT32_MIN)) {
-    throw new DlAbortEx(strerror(errno));
   }
   *pp = p+2;
   return 0;

src/CookieBoxFactory.cc

@@ -68,7 +68,7 @@ Cookie CookieBoxFactory::parseNsCookie(const string& nsCookieStr) const
   c.domain = vs[0];
   c.path = vs[2];
   c.secure = vs[3] == "TRUE" ? true : false;
-  c.expires = strtol(vs[4].c_str(), NULL, 10);
+  c.expires = Util::parseInt(vs[4]);
   c.name = vs[5];
   if(vs.size() >= 7) {
     c.value = vs[6];

src/HttpConnection.cc

@@ -115,7 +115,7 @@ HttpResponseHandle HttpConnection::receiveResponse()
 
   HttpResponseHandle httpResponse = new HttpResponse();
   httpResponse->setCuid(cuid);
-  httpResponse->setStatus(strtol(httpStatusHeader.first.c_str(), 0, 10));
+  httpResponse->setStatus(Util::parseInt(httpStatusHeader.first));
   httpResponse->setHttpHeader(httpStatusHeader.second);
   httpResponse->setHttpRequest(entry->getHttpRequest());
 

src/ParameterizedStringParser.cc

@@ -111,7 +111,7 @@ PStringDatumHandle ParameterizedStringParser::createLoop(const string& src,
   if(colonIndex != string::npos) {
     string stepStr = loopStr.substr(colonIndex+1);
     if(Util::isNumber(stepStr)) {
-      step = strtol(stepStr.c_str(), 0, 10);
+      step = Util::parseInt(stepStr);
     } else {
       throw new FatalException("A step count must be a number.");
     }
@@ -126,8 +126,8 @@ PStringDatumHandle ParameterizedStringParser::createLoop(const string& src,
   int32_t end;
   if(Util::isNumber(range.first) && Util::isNumber(range.second)) {
     nd = new FixedWidthNumberDecorator(range.first.size());
-    start = strtol(range.first.c_str(), 0, 10);
-    end = strtol(range.second.c_str(), 0, 10);
+    start = Util::parseInt(range.first);
+    end = Util::parseInt(range.second);
   } else if(Util::isLowercase(range.first) && Util::isLowercase(range.second)) {
     nd = new AlphaNumberDecorator(range.first.size());
     start = Util::alphaToNum(range.first);

src/Request.cc

@@ -36,6 +36,7 @@
 #include "Util.h"
 #include "FeatureConfig.h"
 #include "CookieBoxFactory.h"
+#include "RecoverableException.h"
 
 const string Request::METHOD_GET = "get";
 
@@ -113,8 +114,13 @@ bool Request::parseUrl(const string& url) {
   Util::split(hostAndPort, hostPart, ':');
   host = hostAndPort.first;
   if(hostAndPort.second != "") {
-    port = strtol(hostAndPort.second.c_str(), NULL, 10);
-    if(!(0 < port && port <= 65535)) {
+    try {
+      port = Util::parseInt(hostAndPort.second);
+      if(!(0 < port && port <= 65535)) {
+	return false;
+      }
+    } catch(RecoverableException* e) {
+      delete e;
       return false;
     }
   } else {

src/Util.cc

@@ -304,11 +304,7 @@ string Util::urldecode(const string& target) {
     if(*itr == '%') {
       if(itr+1 != target.end() && itr+2 != target.end() &&
 	 isxdigit(*(itr+1)) && isxdigit(*(itr+2))) {
-	char temp[3];
-	temp[0] = *(itr+1);
-	temp[1] = *(itr+2);
-	temp[2] = '\0';
-	result += strtol(temp, 0, 16);
+	result += Util::parseInt(string(itr+1, itr+3), 16);
 	itr += 2;
       } else {
 	result += *itr;
@@ -466,26 +462,38 @@ void Util::unfoldRange(const string& src, Integers& range) {
 
 int32_t Util::parseInt(const string& s, int32_t base)
 {
+  if(s.empty()) {
+    throw new DlAbortEx(MSG_STRING_INTEGER_CONVERSION_FAILURE,
+			"empty string");
+  }
   char* stop;
   errno = 0;
   long int v = strtol(s.c_str(), &stop, base);
   if(*stop != '\0') {
-    throw new DlAbortEx(MSG_ILLEGAL_CHARACTER, stop);
+    throw new DlAbortEx(MSG_STRING_INTEGER_CONVERSION_FAILURE,
+			"illegal character");
   } else if((v == LONG_MIN || v == LONG_MAX) && errno == ERANGE || v > INT32_MAX || v < INT32_MIN) {
-    throw new DlAbortEx(MSG_OVERFLOW_UNDERFLOW_DETECTED, s.c_str());
+    throw new DlAbortEx(MSG_STRING_INTEGER_CONVERSION_FAILURE,
+			"overflow/underflow");
   }
   return v;
 }
 
 int64_t Util::parseLLInt(const string& s, int32_t base)
 {
+  if(s.empty()) {
+    throw new DlAbortEx(MSG_STRING_INTEGER_CONVERSION_FAILURE,
+			"empty string");
+  }
   char* stop;
   errno = 0;
   int64_t v = strtoll(s.c_str(), &stop, base);
   if(*stop != '\0') {
-    throw new DlAbortEx(MSG_ILLEGAL_CHARACTER, stop);
+    throw new DlAbortEx(MSG_STRING_INTEGER_CONVERSION_FAILURE,
+			"illegal character");
   } else if((v == INT64_MIN || v == INT64_MAX) && errno == ERANGE) {
-    throw new DlAbortEx(MSG_OVERFLOW_UNDERFLOW_DETECTED, s.c_str());
+    throw new DlAbortEx(MSG_STRING_INTEGER_CONVERSION_FAILURE,
+			"overflow/underflow");
   }
   return v;
 }
@@ -646,7 +654,15 @@ int64_t Util::getRealSize(const string& sizeWithUnit)
     }
     size = sizeWithUnit.substr(0, p);
   }
-  return strtoll(size.c_str(), 0, 10)*mult;
+  int64_t v = Util::parseLLInt(size);
+
+  if(v < 0) {
+    throw new DlAbortEx("Negative value detected: %s", sizeWithUnit.c_str());
+  } else if(v*mult < 0) {
+    throw new DlAbortEx(MSG_STRING_INTEGER_CONVERSION_FAILURE,
+			"overflow/underflow");
+  }
+  return v*mult;
 }
 
 string Util::abbrevSize(int64_t size)

src/message.h

@@ -126,9 +126,8 @@
 #define MSG_DAEMON_FAILED _("daemon failed.")
 #define MSG_VERIFICATION_SUCCESSFUL _("Verification finished successfully. file=%s")
 #define MSG_VERIFICATION_FAILED _("Checksum error detected. file=%s")
-#define MSG_ILLEGAL_CHARACTER _("Illegal character detected: %s")
 #define MSG_INCOMPLETE_RANGE _("Incomplete range specified. %s")
-#define MSG_OVERFLOW_UNDERFLOW_DETECTED _("Overflow/underflow detected: %s")
+#define MSG_STRING_INTEGER_CONVERSION_FAILURE _("Failed to convert string into value: %s")
 
 #define EX_TIME_OUT _("Timeout.")
 #define EX_INVALID_CHUNK_SIZE _("Invalid chunk size.")

test/OptionHandlerTest.cc

@@ -151,12 +151,26 @@ void OptionHandlerTest::testUnitNumberOptionHandler()
   CPPUNIT_ASSERT_EQUAL(string("4294967296"), option.get("foo"));
   handler.parse(&option, "4096K");
   CPPUNIT_ASSERT_EQUAL(string("4194304"), option.get("foo"));
-  handler.parse(&option, "K");
-  CPPUNIT_ASSERT_EQUAL(string("0"), option.get("foo"));
-  handler.parse(&option, "M");
-  CPPUNIT_ASSERT_EQUAL(string("0"), option.get("foo"));
-  handler.parse(&option, "");
-  CPPUNIT_ASSERT_EQUAL(string("0"), option.get("foo"));
+  try {
+    handler.parse(&option, "K");
+    CPPUNIT_FAIL("exception must be thrown.");
+  } catch(Exception* e) {
+    cerr << *e;
+    delete e;
+  }
+  try {
+    handler.parse(&option, "M");
+  } catch(Exception* e) {
+    cerr << *e;
+    delete e;
+  }
+  try {
+    handler.parse(&option, "");
+    CPPUNIT_FAIL("exception must be thrown.");
+  } catch(Exception* e) {
+    cerr << *e;
+    delete e;
+  }
 }
 
 void OptionHandlerTest::testParameterOptionHandler_1argInit()

test/UtilTest.cc

@@ -305,8 +305,41 @@ void UtilTest::testGetRealSize()
 {
   CPPUNIT_ASSERT_EQUAL((int64_t)4294967296LL, Util::getRealSize("4096M"));
   CPPUNIT_ASSERT_EQUAL((int64_t)1024, Util::getRealSize("1K"));
-  CPPUNIT_ASSERT_EQUAL((int64_t)0, Util::getRealSize(""));
-  CPPUNIT_ASSERT_EQUAL((int64_t)0, Util::getRealSize("foo"));
+  try {
+    Util::getRealSize("");
+    CPPUNIT_FAIL("exception must be thrown.");
+  } catch(Exception* e) {
+    cerr << *e;
+    delete e;
+  }
+  try {
+    Util::getRealSize("foo");
+    CPPUNIT_FAIL("exception must be thrown.");
+  } catch(Exception* e) {
+    cerr << *e;
+    delete e;
+  }
+  try {
+    Util::getRealSize("-1");
+    CPPUNIT_FAIL("exception must be thrown.");
+  } catch(Exception* e) {
+    cerr << *e;
+    delete e;
+  }
+  try {
+    Util::getRealSize("9223372036854775807K");
+    CPPUNIT_FAIL("exception must be thrown.");
+  } catch(Exception* e) {
+    cerr << *e;
+    delete e;
+  }
+  try {
+    Util::getRealSize("9223372036854775807M");
+    CPPUNIT_FAIL("exception must be thrown.");
+  } catch(Exception* e) {
+    cerr << *e;
+    delete e;
+  }
 }
 
 void UtilTest::testAbbrevSize()
@@ -504,7 +537,13 @@ void UtilTest::testParseInt()
     cerr << *e;
     delete e;
   }
-
+  try {
+    Util::parseInt("");
+    CPPUNIT_FAIL("exception must be thrown.");
+  } catch(Exception* e) {
+    cerr << *e;
+    delete e;
+  }
 }
 
 void UtilTest::testParseLLInt()
@@ -533,5 +572,11 @@ void UtilTest::testParseLLInt()
     cerr << *e;
     delete e;
   }
-
+  try {
+    Util::parseLLInt("");
+    CPPUNIT_FAIL("exception must be thrown.");
+  } catch(Exception* e) {
+    cerr << *e;
+    delete e;
+  }
 }