[3431] Address Coverity warning "buffer not null terminated" in logger.h

Functionally there is no change to the code by this change addresses
the "buffer not null terminated" warning in the logger constructor.
As well as removing the cause of the warning (a "strncpy" where
the length copied is equal to the destination buffer length), it
it removed two assert() calls: one was replaced by the throwing of
am exception, and the second was redundant.

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) {