[3164] Add changes suggested by review

Change the connection timeout parameter from an "int" to an "unsigned
int". Update the checks to allow for lexical_cast not throwing an
exception when converting a string representing a negative number
to an unsigned int.

diff --git a/doc/guide/dhcp4-srv.xml b/doc/guide/dhcp4-srv.xml
index daae437afbf42d8c83836d145401154371c94b88..0489ff152814859c3880e6fde2dd7ad76b287487 100644 (file)
--- a/doc/guide/dhcp4-srv.xml
+++ b/doc/guide/dhcp4-srv.xml
@@ -463,6 +463,7 @@ be followed by a comma and another object definition.</para>
 "Dhcp4": { "lease-database": { <userinput>"connect-timeout" : <replaceable>timeout-in-seconds</replaceable></userinput>, ... }, ... }
 </screen>
 The default value of five seconds should be more than adequate for local connections.
+If a timeout is given though, it should be an integer greater than zero.
   </para>
   <para>Finally, the credentials of the account under which the server will
   access the database should be set:

diff --git a/doc/guide/dhcp6-srv.xml b/doc/guide/dhcp6-srv.xml
index d5a50397b12bbdddaaf3ff17a2eb8ed1bffec3a0..5eec3e79da059089673c782147795f5821145d17 100644 (file)
--- a/doc/guide/dhcp6-srv.xml
+++ b/doc/guide/dhcp6-srv.xml
@@ -462,6 +462,7 @@ be followed by a comma and another object definition.</para>
 "Dhcp6": { "lease-database": { <userinput>"connect-timeout" : <replaceable>timeout-in-seconds</replaceable></userinput>, ... }, ... }
 </screen>
 The default value of five seconds should be more than adequate for local connections.
+If a timeout is given though, it should be an integer greater than zero.
   </para>
   <para>Finally, the credentials of the account under which the server will
   access the database should be set:

diff --git a/src/lib/dhcpsrv/mysql_connection.cc b/src/lib/dhcpsrv/mysql_connection.cc
index b7163e798f96b8113e1ec278c8623f4362729738..e26c2332db04de3c8fd8816d9a267cbf30a0bf43 100755 (executable)
--- a/src/lib/dhcpsrv/mysql_connection.cc
+++ b/src/lib/dhcpsrv/mysql_connection.cc
@@ -15,6 +15,7 @@
 #include <iterator>
 #include <stdint.h>
 #include <string>
+#include <limits>
 
 using namespace isc;
 using namespace isc::dhcp;
@@ -73,7 +74,7 @@ MySqlConnection::openDatabase() {
         isc_throw(NoDatabaseName, "must specify a name for the database");
     }
 
-    int connect_timeout = MYSQL_DEFAULT_CONNECTION_TIMEOUT;
+    unsigned int connect_timeout = MYSQL_DEFAULT_CONNECTION_TIMEOUT;
     string stimeout;
     try {
         stimeout = getParameter("connect-timeout");
@@ -84,18 +85,29 @@ MySqlConnection::openDatabase() {
 
     if (stimeout.size() > 0) {
         // Timeout was given, so try to convert it to an integer.
+
         try {
-            connect_timeout = boost::lexical_cast<int>(stimeout);
+            connect_timeout = boost::lexical_cast<unsigned int>(stimeout);
         } catch (...) {
-            // Timeout given but invalid.
-            isc_throw(DbInvalidTimeout, "database connection timeout (" << stimeout <<
-                      ") must be numeric");
+            // Timeout given but could not be converted to an unsigned int. Set
+            // the connection timeout to an invalid value to trigger throwing
+            // of an exception.
+            connect_timeout = 0;
         }
 
-        // ... and check the range.
-        if (connect_timeout <= 0) {
-            isc_throw(DbInvalidTimeout, "database connection timeout (" << connect_timeout <<
-                      ") must be an integer greater than 0");
+        // The timeout is only valid if greater than zero, as depending on the
+        // database, a zero timeout might signify someting like "wait
+        // indefinitely".
+        //
+        // The check below also rejects a value greater than the maximum
+        // integer value.  The lexical_cast operation used to obtain a numeric
+        // value from a string can get confused if trying to convert a negative
+        // integer to an unsigned int: instead of throwing an exception, it may
+        // produce a large positive value.
+        if ((connect_timeout == 0) ||
+            (connect_timeout > numeric_limits<int>::max())) {
+            isc_throw(DbInvalidTimeout, "database connection timeout (" <<
+                      stimeout << ") must be an integer greater than 0");
         }
     }
 

diff --git a/src/lib/dhcpsrv/pgsql_lease_mgr.cc b/src/lib/dhcpsrv/pgsql_lease_mgr.cc
index 40546916f9a13fe2557d74b5fb27245a73fe92e5..08405fbc4cbe9c83a28b8860a53c547af54ac40e 100644 (file)
--- a/src/lib/dhcpsrv/pgsql_lease_mgr.cc
+++ b/src/lib/dhcpsrv/pgsql_lease_mgr.cc
@@ -1128,7 +1128,7 @@ PgSqlLeaseMgr::openDatabase() {
         isc_throw(NoDatabaseName, "must specify a name for the database");
     }
 
-    int connect_timeout = PGSQL_DEFAULT_CONNECTION_TIMEOUT;
+    unsigned int connect_timeout = PGSQL_DEFAULT_CONNECTION_TIMEOUT;
     string stimeout;
     try {
         stimeout = dbconn_.getParameter("connect-timeout");
@@ -1139,18 +1139,29 @@ PgSqlLeaseMgr::openDatabase() {
 
     if (stimeout.size() > 0) {
         // Timeout was given, so try to convert it to an integer.
+
         try {
-            connect_timeout = boost::lexical_cast<int>(stimeout);
+            connect_timeout = boost::lexical_cast<unsigned int>(stimeout);
         } catch (...) {
-            // Timeout given but invalid.
-            isc_throw(DbInvalidTimeout, "database connection timeout (" << stimeout <<
-                      ") must be numeric");
+            // Timeout given but could not be converted to an unsigned int. Set
+            // the connection timeout to an invalid value to trigger throwing
+            // of an exception.
+            connect_timeout = 0;
         }
 
-        // ... and check the range.
-        if (connect_timeout <= 0) {
-            isc_throw(DbInvalidTimeout, "database connection timeout (" << connect_timeout <<
-                      ") must be an integer greater than 0");
+        // The timeout is only valid if greater than zero, as depending on the
+        // database, a zero timeout might signify someting like "wait
+        // indefinitely".
+        //
+        // The check below also rejects a value greater than the maximum
+        // integer value.  The lexical_cast operation used to obtain a numeric
+        // value from a string can get confused if trying to convert a negative
+        // integer to an unsigned int: instead of throwing an exception, it may
+        // produce a large positive value.
+        if ((connect_timeout == 0) ||
+            (connect_timeout > numeric_limits<int>::max())) {
+            isc_throw(DbInvalidTimeout, "database connection timeout (" <<
+                      stimeout << ") must be an integer greater than 0");
         }
     }