[master] Merge branch 'trac3431'

src/lib/log/logger.h

@@ -1,4 +1,4 @@
-// Copyright (C) 2011  Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2011, 2014  Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -20,6 +20,8 @@
 #include <string>
 #include <cstring>
 
+#include <boost/static_assert.hpp>
+
 #include <exceptions/exceptions.h>
 #include <log/logger_level.h>
 #include <log/message_types.h>
@@ -27,6 +29,7 @@
 
 namespace isc {
 namespace log {
+
 namespace interprocess {
 // Forward declaration to hide implementation details from normal
 // applications.
@@ -91,24 +94,44 @@ class InterprocessSync;
 
 class LoggerImpl;   // Forward declaration of the implementation class
 
-/// \brief Logging Not Initialized
+/// \brief Bad Interprocess Sync
 ///
-/// Exception thrown if an attempt is made to access a logging function
-/// if the logging system has not been initialized.
-class LoggingNotInitialized : public isc::Exception {
+/// Exception thrown if a bad InterprocessSync object (such as NULL) is
+/// used.
+class BadInterprocessSync : public isc::Exception {
 public:
-    LoggingNotInitialized(const char* file, size_t line, const char* what) :
+    BadInterprocessSync(const char* file, size_t line, const char* what) :
         isc::Exception(file, line, what)
     {}
 };
 
-/// \brief Bad Interprocess Sync
+/// \brief Logger Name Error
 ///
-/// Exception thrown if a bad InterprocessSync object (such as NULL) is
-/// used.
-class BadInterprocessSync : public isc::Exception {
+/// Exception thrown if a logger name is too short or too long.
+class LoggerNameError : public isc::Exception {
 public:
-    BadInterprocessSync(const char* file, size_t line, const char* what) :
+    LoggerNameError(const char* file, size_t line, const char* what) :
+        isc::Exception(file, line, what)
+    {}
+};
+
+/// \brief Logger Name is Null
+///
+/// Exception thrown if a logger name is null
+class LoggerNameNull : public isc::Exception {
+public:
+    LoggerNameNull(const char* file, size_t line, const char* what) :
+        isc::Exception(file, line, what)
+    {}
+};
+
+/// \brief Logging Not Initialized
+///
+/// Exception thrown if an attempt is made to access a logging function
+/// if the logging system has not been initialized.
+class LoggingNotInitialized : public isc::Exception {
+public:
+    LoggingNotInitialized(const char* file, size_t line, const char* what) :
         isc::Exception(file, line, what)
     {}
 };
@@ -139,23 +162,39 @@ public:
     /// child of the root logger.
     ///
     /// \note The name of the logger may be no longer than MAX_LOGGER_NAME_SIZE
-    /// else the program will halt with an assertion failure.  This restriction
-    /// allows loggers to be declared statically: the name is stored in a
-    /// fixed-size array to avoid the need to allocate heap storage during
-    /// program initialization (which causes problems on some operating
-    /// systems).
+    /// else the program will throw an exception.  This restriction allows
+    /// loggers to be declared statically: the name is stored in a fixed-size
+    /// array to avoid the need to allocate heap storage during program
+    /// initialization (which causes problems on some operating systems).
     ///
     /// \note Note also that there is no constructor taking a std::string. This
     /// minimises the possibility of initializing a static logger with a
     /// string, so leading to problems mentioned above.
     Logger(const char* name) : loggerptr_(NULL) {
-        assert(std::strlen(name) < sizeof(name_));
-        // Do the copy.  Note that the assertion above has checked that the
-        // contents of "name" and a trailing null will fit within the space
-        // allocated for name_, so we could use strcpy here and be safe.
-        // However, a bit of extra paranoia doesn't hurt.
-        std::strncpy(name_, name, sizeof(name_));
-        assert(name_[sizeof(name_) - 1] == '\0');
+
+        // Validate the name of the logger.
+        if (name == NULL) {
+            isc_throw(LoggerNameNull, "logger names may not be null");
+
+        } else {
+            // Name not null, is it too short or too long?
+            size_t namelen = std::strlen(name);
+            if ((namelen == 0) || (namelen > MAX_LOGGER_NAME_SIZE)) {
+                isc_throw(LoggerNameError, "'" << name << "' is not a valid "
+                          << "name for a logger: valid names must be between 1 "
+                          << "and " << MAX_LOGGER_NAME_SIZE << " characters in "
+                          << "length");
+            }
+        }
+
+        // The checks above and the assertion below ensure that the contents of
+        // "name" plus a trailing null will fit into the space allocated for
+        // "name_".
+        BOOST_STATIC_ASSERT(MAX_LOGGER_NAME_SIZE < sizeof(name_));
+
+        // Do the copy, ensuring a trailing NULL in all cases.
+        std::strncpy(name_, name, MAX_LOGGER_NAME_SIZE);
+        name_[MAX_LOGGER_NAME_SIZE] = '\0';
     }
 
     /// \brief Destructor

src/lib/log/tests/logger_unittest.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2011  Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2011, 2014  Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -11,6 +11,7 @@
 // LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE
 // OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
 // PERFORMANCE OF THIS SOFTWARE.
+
 #include <gtest/gtest.h>
 
 #include <util/unittests/resource.h>
@@ -353,34 +354,43 @@ TEST_F(LoggerTest, IsDebugEnabledLevel) {
     EXPECT_TRUE(logger.isDebugEnabled(MAX_DEBUG_LEVEL));
 }
 
-// Check that if a logger name is too long, it triggers the appropriate
-// assertion.
+// Check that loggers with invalid names give an error.
+
 TEST_F(LoggerTest, LoggerNameLength) {
-    // The following statements should just declare a logger and nothing
-    // should happen.
-    string ok1(Logger::MAX_LOGGER_NAME_SIZE - 1, 'x');
-    Logger l1(ok1.c_str());
-    EXPECT_EQ(getRootLoggerName() + "." + ok1, l1.getName());
-
-    string ok2(Logger::MAX_LOGGER_NAME_SIZE, 'x');
-    Logger l2(ok2.c_str());
-    EXPECT_EQ(getRootLoggerName() + "." + ok2, l2.getName());
-
-    // Note: Not all systems have EXPECT_DEATH.  As it is a macro we can just
-    // test for its presence and bypass the test if not available.
-#ifdef EXPECT_DEATH
-    // Too long a logger name should trigger an assertion failure.
-    // Note that we just check that it dies - we don't check what message is
-    // output.
-    if (!isc::util::unittests::runningOnValgrind()) {
-        EXPECT_DEATH({
-            isc::util::unittests::dontCreateCoreDumps();
-
-            string ok3(Logger::MAX_LOGGER_NAME_SIZE + 1, 'x');
-            Logger l3(ok3.c_str());
-        }, ".*");
+    // Null name
+    EXPECT_THROW(Logger(NULL), LoggerNameNull);
+
+    // Declare space for the logger name.  The length of names checked
+    // will range from 0 through MAX_LOGGER_NAME_SIZE + 1: to allow for
+    // the trailing null, at least one more byte than the longest name size
+    // must be reserved.
+    char name[Logger::MAX_LOGGER_NAME_SIZE + 2];
+
+    // Zero-length name should throw an exception
+    name[0] = '\0';
+    EXPECT_THROW({
+            Logger dummy(name);
+            }, LoggerNameError);
+
+    // Work through all valid names.
+    for (size_t i = 0; i < Logger::MAX_LOGGER_NAME_SIZE; ++i) {
+
+        // Append a character to the name and check that a logger with that
+        // name can be created without throwing an exception.
+        name[i] = 'X';
+        name[i + 1] = '\0';
+        EXPECT_NO_THROW({
+                Logger dummy(name);
+                }) << "Size of logger name is " << (i + 1);
     }
-#endif
+
+    // ... and check that an overly long name throws an exception.
+    name[Logger::MAX_LOGGER_NAME_SIZE] = 'X';
+    name[Logger::MAX_LOGGER_NAME_SIZE + 1] = '\0';
+    EXPECT_THROW({
+            Logger dummy(name);
+            }, LoggerNameError);
+
 }
 
 TEST_F(LoggerTest, setInterprocessSync) {