[IOT-1561] Improve vs12_snprintf
authorDan Mihai <Daniel.Mihai@microsoft.com>
Tue, 15 Nov 2016 08:23:04 +0000 (00:23 -0800)
committerDan Mihai <Daniel.Mihai@microsoft.com>
Thu, 24 Nov 2016 00:20:02 +0000 (00:20 +0000)
Improve the snprintf surrogate used to build for Windows using Visual
Studio 2013 or older.

Also remove the dependency on cmake for building gtest for Windows, to
fix a build break for the new snprintf_test.cpp.

This is a cherry-pick of commit b2a398e from the 1.2-rel branch.

Change-Id: I70c9d0766a9a316a19cbd283671137e61ab88dbf
Signed-off-by: Dan Mihai <Daniel.Mihai@microsoft.com>
Reviewed-on: https://gerrit.iotivity.org/gerrit/14387
Reviewed-by: Kevin Kane <kkane@microsoft.com>
Tested-by: jenkins-iotivity <jenkins-iotivity@opendaylight.org>
Reviewed-by: Dave Thaler <dthaler@microsoft.com>
Reviewed-on: https://gerrit.iotivity.org/gerrit/14627

resource/c_common/windows/src/snprintf.c
resource/c_common/windows/test/SConscript [new file with mode: 0644]
resource/c_common/windows/test/snprintf_test.cpp [new file with mode: 0644]
resource/unit_tests.scons

index c9c7de5..1a65930 100644 (file)
 
 int vs12_snprintf(char *buffer, size_t count, const char *format, ...)
 {
-    va_list args;
+    // These three values are invalid when calling _vsnprintf
+    if ((buffer == NULL) || (count == 0) || (format == NULL))
+    {
+        return -1;
+    }
 
+    // _vsnprintf behaves similarly to snprintf, but:
+    //
+    // - It doesn't always add a zero terminator to the output buffer
+    // - When the output buffer is too small, it returns -1 instead of the
+    //   required buffer size
+    va_list args;
     va_start(args, format);
-
     int length = _vsnprintf(buffer, count, format, args);
-
     va_end(args);
 
-    if (length > count)
+    if ((length == -1) || (length == count))
     {
+        // Add the missing zero character terminator.
         buffer[count - 1] = '\0';
+
+        // When length == -1, it would be good to compute the required output
+        // buffer size, and return that computed value instead of -1. That would
+        // bring the behavior of vs12_snprintf closer to the C99 version of
+        // snprintf. However, IoTivity doesn't have an easy way to compute the
+        // required output buffer size. Also, there are no known callers of
+        // snprintf in IoTivity that are relying on a compliant return value,
+        // when that compliant return value would be larger than the 'count'
+        // input parameter.
+        //
+        // @todo Compute and return the output buffer size, instead of -1,
+        //       when the output buffer was too small.
     }
 
     return length;
 }
-
diff --git a/resource/c_common/windows/test/SConscript b/resource/c_common/windows/test/SConscript
new file mode 100644 (file)
index 0000000..fbffdfb
--- /dev/null
@@ -0,0 +1,53 @@
+#******************************************************************
+#
+# Copyright 2016 Microsoft
+#
+#-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+#-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
+
+import os.path
+
+######################################################################
+# Build the gtest framework
+######################################################################
+gtest_env = SConscript('#extlibs/gtest/SConscript')
+
+c_common_windows_test_env = gtest_env.Clone()
+
+######################################################################
+# Build flags
+######################################################################
+c_common_windows_test_env.PrependUnique(CPPPATH = ['../include'])
+
+c_common_windows_test_env.AppendUnique(LIBPATH = [os.path.join(c_common_windows_test_env.get('BUILD_DIR'), 'resource', 'c_common')])
+c_common_windows_test_env.PrependUnique(LIBS = ['win_helper'])
+
+if c_common_windows_test_env.get('LOGGING'):
+    c_common_windows_test_env.AppendUnique(CPPDEFINES = ['TB_LOG'])
+
+######################################################################
+# Source files and Targets
+######################################################################
+c_common_windows_tests = c_common_windows_test_env.Program('c_common_windows_tests', ['snprintf_test.cpp'])
+
+Alias("test", [c_common_windows_tests])
+
+c_common_windows_test_env.AppendTarget('test')
+
+from tools.scons.RunTest import *
+run_test(c_common_windows_test_env,
+         'resource_c_common_windows_test.memcheck',
+         'resource/c_common/windows/test/c_common_windows_tests')
diff --git a/resource/c_common/windows/test/snprintf_test.cpp b/resource/c_common/windows/test/snprintf_test.cpp
new file mode 100644 (file)
index 0000000..cce6276
--- /dev/null
@@ -0,0 +1,122 @@
+/* *****************************************************************
+ *
+ * Copyright 2016 Microsoft
+ *
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ * *****************************************************************/
+
+#include <windows.h>
+#include <gtest/gtest.h>
+#include "vs12_snprintf.h"
+
+void OutputBufferLargerThanNeeded(size_t outputBufferSize, size_t inputBufferSize, int callerLineNumber)
+{
+    ASSERT_GT(outputBufferSize, inputBufferSize);
+    ASSERT_GT(inputBufferSize, 1);
+
+    char *inputBuffer = new char[inputBufferSize];
+    memset(inputBuffer, 'i', inputBufferSize - 1);
+    inputBuffer[inputBufferSize - 1] = '\0';
+
+    char *outputBuffer = new char[outputBufferSize];
+    memset(outputBuffer, 'o', outputBufferSize);
+
+    EXPECT_EQ((inputBufferSize - 1),
+              vs12_snprintf(outputBuffer, outputBufferSize, "%s", inputBuffer))
+              << "caller line number: " << callerLineNumber;
+    EXPECT_STREQ(inputBuffer, outputBuffer) << "caller line number: " << callerLineNumber;
+
+    delete[] inputBuffer;
+    delete[] outputBuffer;
+}
+
+void OutputBufferSmallerThanNeeded(size_t outputBufferSize, size_t inputBufferSize, int callerLineNumber)
+{
+    ASSERT_GT(inputBufferSize, outputBufferSize);
+    ASSERT_GT(outputBufferSize, 1);
+
+    char *inputBuffer = new char[inputBufferSize];
+    memset(inputBuffer, 'i', inputBufferSize - 1);
+    inputBuffer[inputBufferSize - 1] = '\0';
+
+    char *outputBuffer = new char[outputBufferSize];
+    memset(outputBuffer, 'o', outputBufferSize);
+
+    if (outputBufferSize == (inputBufferSize - 1))
+    {
+        // When the output buffer size is just off by 1, the return value of
+        // vs12_snprintf is C99-compliant.
+        EXPECT_EQ((inputBufferSize - 1),
+                   vs12_snprintf(outputBuffer, outputBufferSize, "%s", inputBuffer))
+                   << "caller line number: " << callerLineNumber;
+    }
+    else
+    {
+        // vs12_snprintf returns -1 in this case (a non C99-compliant value).
+        EXPECT_EQ(-1,
+                  vs12_snprintf(outputBuffer, outputBufferSize, "%s", inputBuffer))
+                  << "caller line number: " << callerLineNumber;
+    }
+
+    // snprintf output should be zero-terminated, even when the output buffer was too small.
+    EXPECT_EQ('\0', outputBuffer[outputBufferSize - 1]) << "caller line number: " << callerLineNumber;
+
+    // Truncate the input buffer, then compare it with the output buffer.
+    inputBuffer[outputBufferSize - 1] = '\0';
+    EXPECT_STREQ(inputBuffer, outputBuffer) << "caller line number: " << callerLineNumber;
+
+    delete[] inputBuffer;
+    delete[] outputBuffer;
+}
+
+TEST(vs12_snprintfTest, OutputBufferLargerThanNeeded1)
+{
+    OutputBufferLargerThanNeeded(10, 10 - 1, __LINE__);
+}
+
+TEST(vs12_snprintfTest, OutputBufferLargerThanNeeded2)
+{
+    OutputBufferLargerThanNeeded(123, 123 - 2, __LINE__);
+}
+
+TEST(vs12_snprintfTest, OutputBufferAsLargeAsNeeded)
+{
+    char outputBuffer[54];
+    char inputBuffer[sizeof(outputBuffer)];
+
+    memset(inputBuffer, 'i', sizeof(inputBuffer) - 1);
+    inputBuffer[sizeof(inputBuffer) - 1] = '\0';
+
+    memset(outputBuffer, 'o', sizeof(outputBuffer));
+
+    EXPECT_EQ((sizeof(inputBuffer) - 1),
+               vs12_snprintf(outputBuffer, sizeof(outputBuffer), "%s", inputBuffer));
+    EXPECT_STREQ(inputBuffer, outputBuffer);
+}
+
+TEST(vs12_snprintfTest, OutputBufferSmallerThanNeeded1)
+{
+    OutputBufferSmallerThanNeeded(100, 100 + 1, __LINE__);
+}
+
+TEST(vs12_snprintfTest, OutputBufferSmallerThanNeeded2)
+{
+    OutputBufferSmallerThanNeeded(123, 123 + 2, __LINE__);
+}
+
+TEST(vs12_snprintfTest, OutputBufferSmallerThanNeeded3)
+{
+    OutputBufferSmallerThanNeeded(432, 2 * 432, __LINE__);
+}
index c018e28..ed516f9 100644 (file)
@@ -38,11 +38,13 @@ if target_os in ['linux', 'windows']:
        # get it and install it
        SConscript('#extlibs/hippomocks.scons')
 
-    # Build Common unit tests
+       # Build Common unit tests
        SConscript('c_common/oic_string/test/SConscript')
        SConscript('c_common/oic_malloc/test/SConscript')
        SConscript('c_common/oic_time/test/SConscript')
        SConscript('c_common/ocrandom/test/SConscript')
+       if target_os == 'windows':
+               SConscript('c_common/windows/test/SConscript')
 
        # Build C unit tests
        SConscript('csdk/stack/test/SConscript')