Enable /W3 and fix /W3 warnings for Windows

Replace most functions deemed unsafe by the compiler with the more secure CRT functions: - gmtime() becomes gmtime_s(), or gmtime_r() for Linux builds - getenv() becomes getenv_s() - sscanf() becomes sscanf_s() - sprintf() becomes sprintf_s() - fopen() becomes fopen_s() - strcpy() becomes strcpy_s() - strcat() becomes strcat_s() - strncat() becomes strncat_s() Also use more explicit conversions to prevent type and pointer mismatches. Move most #ifdef branches to the following files: - include/openenclave/host.h - include/openenclave/internal/datetime.h - tests/crypto/readfile.h Fix part of openenclave#2075. Signed-off-by: Ryan Hsu <[email protected]>
BRMcLaren · Mar 28, 2020 · d87466e · d87466e
1 parent 2536d0c
commit d87466e
Show file tree

Hide file tree

Showing 42 changed files with 336 additions and 160 deletions.
diff --git a/cmake/compiler_settings.cmake b/cmake/compiler_settings.cmake
@@ -114,7 +114,7 @@ elseif (MSVC)
 
   # Add Flags we want to use for both C and CXX
   add_compile_options(/WX)
-  add_compile_options(/W2)
+  add_compile_options(/W3)
 
   # Ignore compiler warnings:
   # * unicode character not supported

diff --git a/common/datetime.c b/common/datetime.c
@@ -229,20 +229,20 @@ oe_result_t oe_datetime_now(oe_datetime_t* value)
 {
     oe_result_t result = OE_UNEXPECTED;
     time_t now;
-    struct tm* timeinfo;
+    struct tm timeinfo;
 
     if (value == NULL)
         OE_RAISE(OE_INVALID_PARAMETER);
 
     time(&now);
-    timeinfo = gmtime(&now);
-
-    value->year = (uint32_t)timeinfo->tm_year + 1900;
-    value->month = (uint32_t)timeinfo->tm_mon + 1;
-    value->day = (uint32_t)timeinfo->tm_mday;
-    value->hours = (uint32_t)timeinfo->tm_hour;
-    value->minutes = (uint32_t)timeinfo->tm_min;
-    value->seconds = (uint32_t)timeinfo->tm_sec;
+    gmtime_r(&now, &timeinfo);
+
+    value->year = (uint32_t)timeinfo.tm_year + 1900;
+    value->month = (uint32_t)timeinfo.tm_mon + 1;
+    value->day = (uint32_t)timeinfo.tm_mday;
+    value->hours = (uint32_t)timeinfo.tm_hour;
+    value->minutes = (uint32_t)timeinfo.tm_min;
+    value->seconds = (uint32_t)timeinfo.tm_sec;
 
     result = OE_OK;
 done:

diff --git a/common/oe_host_socket.h b/common/oe_host_socket.h
@@ -104,7 +104,7 @@ int _getaddrinfo_read(
         *ai_family = p->ai_family;
         *ai_socktype = p->ai_socktype;
         *ai_protocol = p->ai_protocol;
-        *ai_addrlen = p->ai_addrlen;
+        *ai_addrlen = (oe_socklen_t)p->ai_addrlen;
 
         if (p->ai_canonname)
             *ai_canonnamelen = strlen(p->ai_canonname) + 1;

diff --git a/common/oe_host_string.h b/common/oe_host_string.h
@@ -38,10 +38,21 @@ int oe_strncmp(const char* s1, const char* s2, size_t n)
 
 /* host already has an oe_strlcat implementation */
 
+/*
+ * There are separate OE functions for strerror and strerror_s.
+ * Therefore we suppress the warning about not using more secure CRT functions.
+ */
 OE_INLINE
 char* oe_strerror(int errnum)
 {
+#if defined(_MSC_VER)
+#pragma warning(push)
+#pragma warning(disable : 4996)
+#endif
     return strerror(errnum);
+#if defined(_MSC_VER)
+#pragma warning(pop)
+#endif
 }
 
 OE_INLINE

diff --git a/common/safecrt.c b/common/safecrt.c
@@ -82,7 +82,7 @@ oe_result_t oe_memset_s(void* dst, size_t dst_size, int value, size_t num_bytes)
 
     /* memset_s cannot be optimized away by the compiler */
     while (num_bytes--)
-        *p++ = (volatile unsigned char)value;
+        *p++ = (unsigned char)value;
 
 done:
     return result;

diff --git a/debugger/debugrt/host/host.c b/debugger/debugrt/host/host.c
@@ -47,13 +47,26 @@ static bool raise_debugger_events()
         {
             // If specified, override oe_debugger_contract_version from the
             // environment.
-            char* version = getenv("OE_DEBUGGER_CONTRACT_VERSION");
-            if (version != NULL)
+            char* version;
+            size_t getenv_length;
+            getenv_s(&getenv_length, NULL, 0, "OE_DEBUGGER_CONTRACT_VERSION");
+
+            if (getenv_length > 0)
             {
-                int v = 0;
-                if (sscanf(version, "%d", &v) == 1)
+                version = (char*)malloc(sizeof(char) * getenv_length);
+                getenv_s(
+                    &getenv_length,
+                    version,
+                    sizeof(char) * getenv_length,
+                    "OE_DEBUGGER_CONTRACT_VERSION");
+
+                if (version != NULL)
                 {
-                    oe_debugger_contract_version = (uint32_t)v;
+                    int v = 0;
+                    if (sscanf_s(version, "%d", &v) == 1)
+                    {
+                        oe_debugger_contract_version = (uint32_t)v;
+                    }
                 }
             }
 

diff --git a/host/sgx/windows/exception.c b/host/sgx/windows/exception.c
@@ -4,6 +4,7 @@
 #include "exception.h"
 #include <openenclave/host.h>
 #include <openenclave/internal/calls.h>
+#include <openenclave/internal/registers.h>
 #include <stdio.h>
 #include <windows.h>
 #include "../enclave.h"
@@ -44,7 +45,7 @@ static LONG WINAPI _handle_simulation_mode_exception(
                 if (context->SegFs != enclave_fsbase)
                 {
                     // Update the FS register and continue execution.
-                    oe_set_fs_register_base(enclave_fsbase);
+                    oe_set_fs_register_base((void*)enclave_fsbase);
                     return OE_EXCEPTION_CONTINUE_EXECUTION;
                 }
             }

diff --git a/host/traceh.c b/host/traceh.c
@@ -3,6 +3,7 @@
 
 #include <openenclave/corelibc/limits.h>
 #include <openenclave/internal/calls.h>
+#include <openenclave/internal/datetime.h>
 #include <openenclave/internal/raise.h>
 #include <openenclave/internal/safecrt.h>
 #include <openenclave/internal/trace.h>
@@ -20,6 +21,11 @@
 
 #define LOGGING_FORMAT_STRING "%s.%06ldZ [(%s)%s] tid(0x%llx) | %s"
 
+#if defined(__linux__)
+#define sprintf_s(buffer, size, format, argument) \
+    sprintf(buffer, format, argument)
+#endif
+
 static char* _log_level_strings[OE_LOG_LEVEL_MAX] =
     {"NONE", "FATAL", "ERROR", "WARN", "INFO", "VERBOSE"};
 static oe_mutex _log_lock = OE_H_MUTEX_INITIALIZER;
@@ -201,8 +207,11 @@ static bool _escape_characters(
                         log_msg_escaped[idx] = '\0';
                         return false;
                     }
-                    sprintf(
-                        (char*)&log_msg_escaped[idx], "u%04hhx", log_msg[i]);
+                    sprintf_s(
+                        (char*)&log_msg_escaped[idx],
+                        msg_size - idx,
+                        "u%04hhx",
+                        log_msg[i]);
                     // idx is also incremented after switch case
                     idx += MAX_ESCAPED_CHAR_LEN - 1;
                     break;
@@ -310,23 +319,14 @@ oe_result_t oe_log(oe_log_level_t level, const char* fmt, ...)
 void oe_log_message(bool is_enclave, oe_log_level_t level, const char* message)
 {
     // get timestamp for log
-#if defined(__linux__)
-    struct timeval time_now;
-    gettimeofday(&time_now, NULL);
-    struct tm* t = gmtime(&time_now.tv_sec);
-#else
+    struct tm t;
     time_t lt = time(NULL);
-    struct tm* t = gmtime(&lt);
-#endif
+    gmtime_r(&lt, &t);
 
     char time[20];
-    strftime(time, sizeof(time), "%Y-%m-%dT%H:%M:%S", t);
-
-#if defined(__linux__)
-    long int usecs = time_now.tv_usec;
-#else
+    strftime(time, sizeof(time), "%Y-%m-%dT%H:%M:%S", &t);
     long int usecs = 0;
-#endif
+
     if (!_initialized)
     {
         initialize_log_config();

diff --git a/host/windows/syscall.c b/host/windows/syscall.c
@@ -17,12 +17,14 @@
 #include <io.h>
 #include <stdint.h>
 #include <sys/stat.h>
+#include <sys/types.h>
 
 // clang-format off
 
 #include <winsock2.h>
 #include <windows.h>
 #include <Ws2def.h>
+#include <Ws2tcpip.h>
 #include <VersionHelpers.h>
 // clang-format on
 
@@ -934,15 +936,21 @@ ssize_t oe_syscall_send_ocall(
     size_t len,
     int flags)
 {
-    ssize_t ret;
+    ssize_t ret = OE_EINVAL;
     _set_errno(0);
 
-    ret = send(_get_socket(sockfd), buf, len, flags);
+    if ((len & 0xFFFFFFFF) != len)
+    {
+        _set_errno(OE_EINVAL);
+        goto done;
+    }
+
+    ret = send(_get_socket(sockfd), buf, (int)len, flags);
     if (ret == SOCKET_ERROR)
     {
         _set_errno(_winsockerr_to_errno(WSAGetLastError()));
     }
-
+done:
     return ret;
 }
 
@@ -957,18 +965,24 @@ ssize_t oe_syscall_sendto_ocall(
     ssize_t ret;
     _set_errno(0);
 
+    if ((len & 0xFFFFFFFF) != len)
+    {
+        _set_errno(OE_EINVAL);
+        goto done;
+    }
+
     ret = sendto(
         _get_socket(sockfd),
         buf,
-        len,
+        (int)len,
         flags,
         (struct sockaddr*)src_addr,
         addrlen);
     if (ret == SOCKET_ERROR)
     {
         _set_errno(_winsockerr_to_errno(WSAGetLastError()));
     }
-
+done:
     return ret;
 }
 
@@ -1506,13 +1520,13 @@ int oe_syscall_uname_ocall(struct oe_utsname* buf)
     // OE SDK is supported only on WindowsServer and Win10
     if (IsWindowsServer())
     {
-        sprintf(buf->sysname, "WindowsServer");
-        sprintf(buf->version, "2016OrAbove");
+        sprintf_s(buf->sysname, sizeof(buf->sysname), "WindowsServer");
+        sprintf_s(buf->version, sizeof(buf->version), "2016OrAbove");
     }
     else if (IsWindows10OrGreater())
     {
-        sprintf(buf->sysname, "Windows10OrGreater");
-        sprintf(buf->version, "10OrAbove");
+        sprintf_s(buf->sysname, sizeof(buf->sysname), "Windows10OrGreater");
+        sprintf_s(buf->version, sizeof(buf->version), "10OrAbove");
     }
 
     ret = 0;

diff --git a/include/openenclave/host.h b/include/openenclave/host.h
@@ -29,6 +29,24 @@
 
 OE_EXTERNC_BEGIN
 
+#ifndef _WIN32
+#define _getpid getpid
+#define sscanf_s sscanf
+#define sprintf_s(buffer, size, format, argument) \
+    sprintf(buffer, format, argument)
+#define strcat_s(destination, destination_size, source) \
+    strcat(destination, source)
+#define strcpy_s(destination, destination_size, source) \
+    ;                                                   \
+    {                                                   \
+        (void)(destination_size);                       \
+        strcpy(destination, source);                    \
+    }
+#define _strdup strdup
+#define strncat_s(destination, destination_size, source, source_size) \
+    strncat(destination, source, source_size)
+#endif
+
 /**
  *  Flag passed into oe_create_enclave to run the enclave in debug mode.
  *  The flag allows the enclave to be created without the enclave binary

diff --git a/include/openenclave/internal/datetime.h b/include/openenclave/internal/datetime.h
@@ -13,6 +13,10 @@ OE_EXTERNC_BEGIN
 // ISO 8601 format: YYYY-MM-DDThh:mm:ssZ
 #define OE_DATETIME_FORMAT ("YYYY-MM-DDThh:mm:ssZ")
 
+#ifdef _WIN32
+#define gmtime_r(now, timeinfo) gmtime_s(timeinfo, now)
+#endif
+
 /**
  * Check whether the given issue date is a valid date time.
  */

diff --git a/tests/attestation_cert_apis/host/host.cpp b/tests/attestation_cert_apis/host/host.cpp
@@ -20,6 +20,8 @@
 #define TEST_RSA_KEY 1
 #define SKIP_RETURN_CODE 2
 
+#define FILENAME_LENGTH 80
+
 // This is the identity validation callback. An TLS connecting party (client or
 // server) can verify the passed in "identity" information to decide whether to
 // accept an connection reqest
@@ -92,16 +94,21 @@ void run_test(oe_enclave_t* enclave, int test_type)
 
     {
         // for testing purpose, output the whole cer in DER format
-        char filename[80];
+        char filename[FILENAME_LENGTH];
         FILE* file = NULL;
 
-        sprintf(
+        sprintf_s(
             filename,
+            sizeof(filename),
             "./cert_%s.der",
             test_type == TEST_RSA_KEY ? "rsa" : "ec");
         OE_TRACE_INFO(
             "Host: Log quote embedded certificate to file: [%s]\n", filename);
+#ifdef _WIN32
+        fopen_s(&file, filename, "wb");
+#else
         file = fopen(filename, "wb");
+#endif
         fwrite(cert, 1, cert_size, file);
         fclose(file);
     }

diff --git a/tests/crypto/enclave/host/host.c b/tests/crypto/enclave/host/host.c
@@ -38,7 +38,10 @@ int f_open(char* path, int flags, int mode)
      * also processed as binary (as they would be equivalently on Linux).
      */
     flags |= _O_BINARY;
+#pragma warning(push)
+#pragma warning(disable : 4996)
     return _open(path, flags, mode);
+#pragma warning(pop)
 #else
     return open(path, flags, mode);
 #endif
@@ -56,7 +59,7 @@ int f_openat(int dirfd, char* path, int flags, int mode)
 int f_read(int fd, char* ptr, size_t len)
 {
 #if defined(_WIN32)
-    return (int)_read(fd, ptr, len);
+    return (int)_read(fd, ptr, (int)len);
 #else
     return (int)read(fd, ptr, len);
 #endif