From 8892d3337afa31fee644eed5ea9d9b057a077f7b Mon Sep 17 00:00:00 2001 From: Marek Habersack Date: Mon, 26 Aug 2019 12:53:40 +0200 Subject: [PATCH] Check for integer overflows when allocating memory Context: https://github.com/android-ndk/ndk/issues/294 Context: https://github.com/android-ndk/ndk/issues/295 Context: https://bugs.llvm.org/show_bug.cgi?id=16404 Add a couple of functions to `Utils` taking advantage of the compiler builtin functions to perform integer addition and multiplication while checking for overflows. If an overflow is detected, the application is terminated. Due to a bug in Android NDK's clang, the multiplication is not performed using "open" types but rather, currently, only `size_t` for the multiplication operands. Using a template for them would result in a link error as the compiler would generate code to use 128-bit integers to perform the operation, attempting to call the `__muloti4` intrinsic function which is usually defined in `libgcc`, `libcompiler_rt` or `libcompiler_rt-extras` libraries. In the NDK case the 64-bit targets do not contain implementation of the function in neither of the libraries mentioned above. --- src/monodroid/jni/android-system.cc | 16 +++++---- src/monodroid/jni/embedded-assemblies.cc | 10 ++++-- src/monodroid/jni/logger.cc | 2 +- src/monodroid/jni/monodroid-glue.cc | 11 +++--- src/monodroid/jni/monodroid-networkinfo.cc | 4 ++- src/monodroid/jni/util.cc | 14 +++++--- src/monodroid/jni/util.h | 41 ++++++++++++++++++++++ src/monodroid/jni/xamarin_getifaddrs.cc | 6 ++-- 8 files changed, 82 insertions(+), 22 deletions(-) diff --git a/src/monodroid/jni/android-system.cc b/src/monodroid/jni/android-system.cc index b53bf6b5fe2..aa18e802ed7 100644 --- a/src/monodroid/jni/android-system.cc +++ b/src/monodroid/jni/android-system.cc @@ -148,8 +148,9 @@ AndroidSystem::add_system_property (const char *name, const char *value) return; } - size_t name_len = strlen (name); - p = reinterpret_cast (malloc (sizeof ( BundledProperty) + name_len + 1)); + size_t name_len = strlen (name) + 1; + size_t alloc_size = ADD_WITH_OVERFLOW_CHECK (size_t, sizeof (BundledProperty), name_len); + p = reinterpret_cast (malloc (alloc_size)); if (p == nullptr) return; @@ -227,8 +228,9 @@ AndroidSystem::_monodroid__system_property_get (const char *name, char *sp_value char *buf = nullptr; if (sp_value_len < PROP_VALUE_MAX + 1) { + size_t alloc_size = ADD_WITH_OVERFLOW_CHECK (size_t, PROP_VALUE_MAX, 1); log_warn (LOG_DEFAULT, "Buffer to store system property may be too small, will copy only %u bytes", sp_value_len); - buf = new char [PROP_VALUE_MAX + 1]; + buf = new char [alloc_size]; } int len = __system_property_get (name, buf ? buf : sp_value); @@ -262,7 +264,8 @@ AndroidSystem::monodroid_get_system_property (const char *name, char **value) } if (len >= 0 && value) { - *value = new char [static_cast(len) + 1]; + size_t alloc_size = ADD_WITH_OVERFLOW_CHECK (size_t, static_cast(len), 1); + *value = new char [alloc_size]; if (*value == nullptr) return -len; if (len > 0) @@ -283,7 +286,7 @@ AndroidSystem::monodroid_read_file_into_memory (const char *path, char **value) if (fp != nullptr) { struct stat fileStat; if (fstat (fileno (fp), &fileStat) == 0) { - r = static_cast(fileStat.st_size) + 1; + r = ADD_WITH_OVERFLOW_CHECK (size_t, static_cast(fileStat.st_size), 1); if (value && (*value = new char[r])) { fread (*value, 1, static_cast(fileStat.st_size), fp); } @@ -316,7 +319,8 @@ AndroidSystem::_monodroid_get_system_property_from_file (const char *path, char return file_size + 1; } - *value = new char[file_size + 1]; + size_t alloc_size = ADD_WITH_OVERFLOW_CHECK (size_t, file_size, 1); + *value = new char[alloc_size]; if (!(*value)) { fclose (fp); return file_size + 1; diff --git a/src/monodroid/jni/embedded-assemblies.cc b/src/monodroid/jni/embedded-assemblies.cc index 40b1ac19cc2..4718a2713c5 100644 --- a/src/monodroid/jni/embedded-assemblies.cc +++ b/src/monodroid/jni/embedded-assemblies.cc @@ -70,7 +70,9 @@ EmbeddedAssemblies::open_from_bundles (MonoAssemblyName* aname, bool ref_only) size_t name_len = culture == nullptr ? 0 : strlen (culture) + 1; name_len += sizeof (".exe"); name_len += strlen (asmname); - char *name = new char [name_len + 1]; + + size_t alloc_size = ADD_WITH_OVERFLOW_CHECK (size_t, name_len, 1); + char *name = new char [alloc_size]; name [0] = '\0'; if (culture != nullptr && *culture != '\0') { @@ -457,7 +459,8 @@ EmbeddedAssemblies::gather_bundled_assemblies_from_apk (const char* apk, monodro if (entry_is_overridden) continue; - bundled_assemblies = reinterpret_cast (utils.xrealloc (bundled_assemblies, sizeof(void*) * (bundled_assemblies_count + 1))); + size_t alloc_size = MULTIPLY_WITH_OVERFLOW_CHECK (size_t, sizeof(void*), bundled_assemblies_count + 1); + bundled_assemblies = reinterpret_cast (utils.xrealloc (bundled_assemblies, alloc_size)); cur = bundled_assemblies [bundled_assemblies_count] = reinterpret_cast (utils.xcalloc (1, sizeof (MonoBundledAssembly))); ++bundled_assemblies_count; @@ -550,7 +553,8 @@ EmbeddedAssemblies::register_from (const char *apk_file, monodroid_should_regist log_info (LOG_ASSEMBLY, "Package '%s' contains %i assemblies", apk_file, bundled_assemblies_count - prev); if (bundled_assemblies) { - bundled_assemblies = reinterpret_cast (utils.xrealloc (bundled_assemblies, sizeof(void*)*(bundled_assemblies_count + 1))); + size_t alloc_size = MULTIPLY_WITH_OVERFLOW_CHECK (size_t, sizeof(void*), bundled_assemblies_count + 1); + bundled_assemblies = reinterpret_cast (utils.xrealloc (bundled_assemblies, alloc_size)); bundled_assemblies [bundled_assemblies_count] = nullptr; } diff --git a/src/monodroid/jni/logger.cc b/src/monodroid/jni/logger.cc index 6bc0b71cb32..d5605d283de 100644 --- a/src/monodroid/jni/logger.cc +++ b/src/monodroid/jni/logger.cc @@ -182,7 +182,7 @@ init_logging_categories () } } - monodroid_strfreev (args); + utils.monodroid_strfreev (args); #if DEBUG if ((log_categories & LOG_GC) != 0) diff --git a/src/monodroid/jni/monodroid-glue.cc b/src/monodroid/jni/monodroid-glue.cc index a3e019181cf..beea67ec4c3 100644 --- a/src/monodroid/jni/monodroid-glue.cc +++ b/src/monodroid/jni/monodroid-glue.cc @@ -670,8 +670,9 @@ parse_runtime_args (char *runtime_args, RuntimeOptions *options) sep = strchr (arg, ':'); if (sep != nullptr) { size_t arg_len = static_cast(sep - arg); - host = new char [arg_len + 1]; - memset (host, 0x00, arg_len + 1); + size_t alloc_size = ADD_WITH_OVERFLOW_CHECK (size_t, arg_len, 1); + host = new char [alloc_size]; + memset (host, 0x00, alloc_size); strncpy (host, arg, arg_len); arg = sep+1; @@ -1503,7 +1504,8 @@ monodroid_profiler_load (const char *libmono_path, const char *desc, const char if (col != nullptr) { size_t name_len = static_cast(col - desc); - mname = new char [name_len + 1]; + size_t alloc_size = ADD_WITH_OVERFLOW_CHECK (size_t, name_len, 1); + mname = new char [alloc_size]; strncpy (mname, desc, name_len); mname [name_len] = 0; } else { @@ -1573,7 +1575,8 @@ set_profile_options (JNIEnv *env) extension = utils.strdup_new ("aotprofile"); else { size_t len = col != nullptr ? static_cast(col - value) : strlen (value); - extension = new char [len + 1]; + size_t alloc_size = ADD_WITH_OVERFLOW_CHECK (size_t, len, 1); + extension = new char [alloc_size]; strncpy (extension, value, len); extension [len] = '\0'; } diff --git a/src/monodroid/jni/monodroid-networkinfo.cc b/src/monodroid/jni/monodroid-networkinfo.cc index 13129987e04..25d9edb2749 100644 --- a/src/monodroid/jni/monodroid-networkinfo.cc +++ b/src/monodroid/jni/monodroid-networkinfo.cc @@ -32,6 +32,7 @@ #include "monodroid.h" #include "monodroid-glue.h" #include "util.h" +#include "globals.h" using namespace xamarin::android; @@ -167,7 +168,8 @@ _monodroid_get_dns_servers (void **dns_servers_array) if (count <= 0) return 0; - char **ret = (char**)malloc (sizeof (char*) * static_cast(count)); + size_t alloc_size = MULTIPLY_WITH_OVERFLOW_CHECK (size_t, sizeof (char*), static_cast(count)); + char **ret = (char**)malloc (alloc_size); char **p = ret; for (int i = 0; i < 8; i++) { if (!dns_servers [i]) diff --git a/src/monodroid/jni/util.cc b/src/monodroid/jni/util.cc index 2cdee8f66cc..a297c520a1e 100644 --- a/src/monodroid/jni/util.cc +++ b/src/monodroid/jni/util.cc @@ -103,7 +103,7 @@ Util::path_combine (const char *path1, const char *path2) if (path2 == nullptr) return strdup_new (path1); - size_t len = strlen (path1) + strlen (path2) + 2; + size_t len = add_with_overflow_check (__FILE__, __LINE__, strlen (path1), strlen (path2) + 2); char *ret = new char [len]; *ret = '\0'; @@ -117,9 +117,12 @@ Util::path_combine (const char *path1, const char *path2) void Util::add_to_vector (char ***vector, size_t size, char *token) { - *vector = *vector == nullptr ? - (char **)xmalloc(2 * sizeof(*vector)) : - (char **)xrealloc(*vector, (size + 1) * sizeof(*vector)); + if (*vector == nullptr) { + *vector = (char **)static_cast(xmalloc (size * sizeof(*vector))); + } else { + size_t alloc_size = multiply_with_overflow_check (__FILE__, __LINE__, sizeof(*vector), size + 1); + *vector = static_cast(xrealloc (*vector, alloc_size)); + } (*vector)[size - 1] = token; } @@ -165,7 +168,8 @@ Util::monodroid_strsplit (const char *str, const char *delimiter, size_t max_tok if (*str) { size_t toklen = static_cast((str - c)); - token = new char [toklen + 1]; + size_t alloc_size = add_with_overflow_check (__FILE__, __LINE__, toklen, 1); + token = new char [alloc_size]; strncpy (token, c, toklen); token [toklen] = '\0'; diff --git a/src/monodroid/jni/util.h b/src/monodroid/jni/util.h index bd3ebe72d68..5e048c9641a 100644 --- a/src/monodroid/jni/util.h +++ b/src/monodroid/jni/util.h @@ -124,6 +124,9 @@ namespace xamarin { namespace android timing_diff (const timing_period &period); }; +#define ADD_WITH_OVERFLOW_CHECK(__ret_type__, __a__, __b__) utils.add_with_overflow_check<__ret_type__>(__FILE__, __LINE__, (__a__), (__b__)) +#define MULTIPLY_WITH_OVERFLOW_CHECK(__ret_type__, __a__, __b__) utils.multiply_with_overflow_check<__ret_type__>(__FILE__, __LINE__, (__a__), (__b__)) + class Util { #if defined (ANDROID) || defined (LINUX) @@ -227,6 +230,44 @@ namespace xamarin { namespace android return (log_categories & category) != 0; } + template + inline Ret add_with_overflow_check (const char *file, uint32_t line, P1 a, P2 b) const + { + Ret ret; + + if (XA_UNLIKELY (__builtin_add_overflow (a, b, &ret))) { + log_fatal (LOG_DEFAULT, "Integer overflow on addition at %s:%u", file, line); + exit (FATAL_EXIT_OUT_OF_MEMORY); + return static_cast(0); + } + + return ret; + } + + // Can't use templates as above with add_with_oveflow because of a bug in the clang compiler + // shipped with the NDK: + // + // https://github.com/android-ndk/ndk/issues/294 + // https://github.com/android-ndk/ndk/issues/295 + // https://bugs.llvm.org/show_bug.cgi?id=16404 + // + // Using templated parameter types for `a` and `b` would make clang generate that tries to + // use 128-bit integers and thus output code that calls `__muloti4` and so linking would + // fail + // + template + inline Ret multiply_with_overflow_check (const char *file, uint32_t line, size_t a, size_t b) const + { + Ret ret; + + if (XA_UNLIKELY (__builtin_mul_overflow (a, b, &ret))) { + log_fatal (LOG_DEFAULT, "Integer overflow on multiplication at %s:%u", file, line); + exit (FATAL_EXIT_OUT_OF_MEMORY); + return static_cast(0); + } + + return ret; + } private: //char *monodroid_strdup_printf (const char *format, va_list vargs); void add_to_vector (char ***vector, size_t size, char *token); diff --git a/src/monodroid/jni/xamarin_getifaddrs.cc b/src/monodroid/jni/xamarin_getifaddrs.cc index 31b7e804295..84a7c898b95 100644 --- a/src/monodroid/jni/xamarin_getifaddrs.cc +++ b/src/monodroid/jni/xamarin_getifaddrs.cc @@ -532,7 +532,8 @@ parse_netlink_reply (netlink_session *session, struct _monodroid_ifaddrs **ifadd size_t buf_size = static_cast(getpagesize ()); log_debug (LOG_NETLINK, "receive buffer size == %d", buf_size); - response = (unsigned char*)malloc (sizeof (*response) * buf_size); + size_t alloc_size = MULTIPLY_WITH_OVERFLOW_CHECK (size_t, sizeof(*response), buf_size); + response = (unsigned char*)malloc (alloc_size); ssize_t length = 0; if (!response) { goto cleanup; @@ -869,7 +870,8 @@ get_link_address (const struct nlmsghdr *message, struct _monodroid_ifaddrs **if } if (payload_size > 0) { - ifa->ifa_name = (char*)malloc (payload_size + room_for_trailing_null); + size_t alloc_size = ADD_WITH_OVERFLOW_CHECK (size_t, payload_size, room_for_trailing_null); + ifa->ifa_name = (char*)malloc (alloc_size); if (!ifa->ifa_name) { goto error; }