Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check for integer overflows when allocating memory #3549

Merged
merged 1 commit into from
Aug 27, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions src/monodroid/jni/android-system.cc
Original file line number Diff line number Diff line change
Expand Up @@ -148,8 +148,9 @@ AndroidSystem::add_system_property (const char *name, const char *value)
return;
}

size_t name_len = strlen (name);
p = reinterpret_cast<BundledProperty*> (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<BundledProperty*> (malloc (alloc_size));
if (p == nullptr)
return;

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -262,7 +264,8 @@ AndroidSystem::monodroid_get_system_property (const char *name, char **value)
}

if (len >= 0 && value) {
*value = new char [static_cast<size_t>(len) + 1];
size_t alloc_size = ADD_WITH_OVERFLOW_CHECK (size_t, static_cast<size_t>(len), 1);
*value = new char [alloc_size];
if (*value == nullptr)
return -len;
if (len > 0)
Expand All @@ -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<size_t>(fileStat.st_size) + 1;
r = ADD_WITH_OVERFLOW_CHECK (size_t, static_cast<size_t>(fileStat.st_size), 1);
if (value && (*value = new char[r])) {
fread (*value, 1, static_cast<size_t>(fileStat.st_size), fp);
}
Expand Down Expand Up @@ -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;
Expand Down
10 changes: 7 additions & 3 deletions src/monodroid/jni/embedded-assemblies.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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') {
Expand Down Expand Up @@ -457,7 +459,8 @@ EmbeddedAssemblies::gather_bundled_assemblies_from_apk (const char* apk, monodro
if (entry_is_overridden)
continue;

bundled_assemblies = reinterpret_cast<MonoBundledAssembly**> (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<MonoBundledAssembly**> (utils.xrealloc (bundled_assemblies, alloc_size));
cur = bundled_assemblies [bundled_assemblies_count] = reinterpret_cast<MonoBundledAssembly*> (utils.xcalloc (1, sizeof (MonoBundledAssembly)));
++bundled_assemblies_count;

Expand Down Expand Up @@ -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 <MonoBundledAssembly**> (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 <MonoBundledAssembly**> (utils.xrealloc (bundled_assemblies, alloc_size));
bundled_assemblies [bundled_assemblies_count] = nullptr;
}

Expand Down
2 changes: 1 addition & 1 deletion src/monodroid/jni/logger.cc
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ init_logging_categories ()
}
}

monodroid_strfreev (args);
utils.monodroid_strfreev (args);

#if DEBUG
if ((log_categories & LOG_GC) != 0)
Expand Down
11 changes: 7 additions & 4 deletions src/monodroid/jni/monodroid-glue.cc
Original file line number Diff line number Diff line change
Expand Up @@ -670,8 +670,9 @@ parse_runtime_args (char *runtime_args, RuntimeOptions *options)
sep = strchr (arg, ':');
if (sep != nullptr) {
size_t arg_len = static_cast<size_t>(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;

Expand Down Expand Up @@ -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<size_t>(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 {
Expand Down Expand Up @@ -1573,7 +1575,8 @@ set_profile_options (JNIEnv *env)
extension = utils.strdup_new ("aotprofile");
else {
size_t len = col != nullptr ? static_cast<size_t>(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';
}
Expand Down
4 changes: 3 additions & 1 deletion src/monodroid/jni/monodroid-networkinfo.cc
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
#include "monodroid.h"
#include "monodroid-glue.h"
#include "util.h"
#include "globals.h"

using namespace xamarin::android;

Expand Down Expand Up @@ -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<size_t>(count));
size_t alloc_size = MULTIPLY_WITH_OVERFLOW_CHECK (size_t, sizeof (char*), static_cast<size_t>(count));
char **ret = (char**)malloc (alloc_size);
char **p = ret;
for (int i = 0; i < 8; i++) {
if (!dns_servers [i])
Expand Down
14 changes: 9 additions & 5 deletions src/monodroid/jni/util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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<size_t> (__FILE__, __LINE__, strlen (path1), strlen (path2) + 2);
char *ret = new char [len];
*ret = '\0';

Expand All @@ -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<char**>(xmalloc (size * sizeof(*vector)));
} else {
size_t alloc_size = multiply_with_overflow_check<size_t> (__FILE__, __LINE__, sizeof(*vector), size + 1);
*vector = static_cast<char**>(xrealloc (*vector, alloc_size));
}

(*vector)[size - 1] = token;
}
Expand Down Expand Up @@ -165,7 +168,8 @@ Util::monodroid_strsplit (const char *str, const char *delimiter, size_t max_tok

if (*str) {
size_t toklen = static_cast<size_t>((str - c));
token = new char [toklen + 1];
size_t alloc_size = add_with_overflow_check<size_t> (__FILE__, __LINE__, toklen, 1);
token = new char [alloc_size];
strncpy (token, c, toklen);
token [toklen] = '\0';

Expand Down
41 changes: 41 additions & 0 deletions src/monodroid/jni/util.h
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -227,6 +230,44 @@ namespace xamarin { namespace android
return (log_categories & category) != 0;
}

template<typename Ret, typename P1, typename P2>
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<Ret>(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<typename Ret>
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<Ret>(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);
Expand Down
6 changes: 4 additions & 2 deletions src/monodroid/jni/xamarin_getifaddrs.cc
Original file line number Diff line number Diff line change
Expand Up @@ -532,7 +532,8 @@ parse_netlink_reply (netlink_session *session, struct _monodroid_ifaddrs **ifadd
size_t buf_size = static_cast<size_t>(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;
Expand Down Expand Up @@ -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;
}
Expand Down