From 2deed25da008a6e0a53231d2d00029a38686a328 Mon Sep 17 00:00:00 2001 From: David LeBlanc Date: Thu, 11 Apr 2019 15:54:12 -0700 Subject: [PATCH 1/8] Fix for issue 775 Fix possibly incorrect integer overflow check with an efficient correct check. Not fixing the issue where size == 0, unsure if this is by design, or what error to return if not. --- src/unpack.c | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/src/unpack.c b/src/unpack.c index 92f7c9132..e2644df25 100644 --- a/src/unpack.c +++ b/src/unpack.c @@ -190,19 +190,27 @@ static inline int template_callback_false(unpack_user* u, msgpack_object* o) static inline int template_callback_array(unpack_user* u, unsigned int n, msgpack_object* o) { unsigned int size; + unsigned long long tmp; + o->type = MSGPACK_OBJECT_ARRAY; o->via.array.size = 0; - size = n*sizeof(msgpack_object); - if (size / sizeof(msgpack_object) != n) { + tmp = (unsigned long long)n * sizeof(msgpack_object); + + if (tmp & 0xffffffff00000000) { // integer overflow return MSGPACK_UNPACK_NOMEM_ERROR; } + + size = (unsigned int)tmp; + if (*u->z == NULL) { *u->z = msgpack_zone_new(MSGPACK_ZONE_CHUNK_SIZE); if(*u->z == NULL) { return MSGPACK_UNPACK_NOMEM_ERROR; } } + + // Unsure whether size = 0 should be an error, and if so, what to return o->via.array.ptr = (msgpack_object*)msgpack_zone_malloc(*u->z, size); if(o->via.array.ptr == NULL) { return MSGPACK_UNPACK_NOMEM_ERROR; } return 0; @@ -223,19 +231,27 @@ static inline int template_callback_array_item(unpack_user* u, msgpack_object* c static inline int template_callback_map(unpack_user* u, unsigned int n, msgpack_object* o) { unsigned int size; + unsigned long long tmp; + o->type = MSGPACK_OBJECT_MAP; o->via.map.size = 0; - size = n*sizeof(msgpack_object_kv); - if (size / sizeof(msgpack_object_kv) != n) { + size = (unsigned long long)n * sizeof(msgpack_object_kv); + + if (tmp & 0xffffffff00000000) { // integer overflow return MSGPACK_UNPACK_NOMEM_ERROR; } + + size = (unsigned int)tmp; + if (*u->z == NULL) { *u->z = msgpack_zone_new(MSGPACK_ZONE_CHUNK_SIZE); if(*u->z == NULL) { return MSGPACK_UNPACK_NOMEM_ERROR; } } + + // Should size = 0 be an error? If so, what error to return? o->via.map.ptr = (msgpack_object_kv*)msgpack_zone_malloc(*u->z, size); if(o->via.map.ptr == NULL) { return MSGPACK_UNPACK_NOMEM_ERROR; } return 0; From ed30252bdc3f8f950b474ac3270b40864896c675 Mon Sep 17 00:00:00 2001 From: David LeBlanc Date: Thu, 11 Apr 2019 15:57:25 -0700 Subject: [PATCH 2/8] Spaces not tabs --- src/unpack.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/unpack.c b/src/unpack.c index e2644df25..1a2bd231e 100644 --- a/src/unpack.c +++ b/src/unpack.c @@ -190,7 +190,7 @@ static inline int template_callback_false(unpack_user* u, msgpack_object* o) static inline int template_callback_array(unpack_user* u, unsigned int n, msgpack_object* o) { unsigned int size; - unsigned long long tmp; + unsigned long long tmp; o->type = MSGPACK_OBJECT_ARRAY; o->via.array.size = 0; @@ -201,7 +201,7 @@ static inline int template_callback_array(unpack_user* u, unsigned int n, msgpac return MSGPACK_UNPACK_NOMEM_ERROR; } - size = (unsigned int)tmp; + size = (unsigned int)tmp; if (*u->z == NULL) { *u->z = msgpack_zone_new(MSGPACK_ZONE_CHUNK_SIZE); @@ -210,7 +210,7 @@ static inline int template_callback_array(unpack_user* u, unsigned int n, msgpac } } - // Unsure whether size = 0 should be an error, and if so, what to return + // Unsure whether size = 0 should be an error, and if so, what to return o->via.array.ptr = (msgpack_object*)msgpack_zone_malloc(*u->z, size); if(o->via.array.ptr == NULL) { return MSGPACK_UNPACK_NOMEM_ERROR; } return 0; @@ -231,7 +231,7 @@ static inline int template_callback_array_item(unpack_user* u, msgpack_object* c static inline int template_callback_map(unpack_user* u, unsigned int n, msgpack_object* o) { unsigned int size; - unsigned long long tmp; + unsigned long long tmp; o->type = MSGPACK_OBJECT_MAP; o->via.map.size = 0; From eff6f5a2fd48675ff45917268348915a244083b0 Mon Sep 17 00:00:00 2001 From: David LeBlanc Date: Thu, 11 Apr 2019 15:58:49 -0700 Subject: [PATCH 3/8] Two more tabs --- src/unpack.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/unpack.c b/src/unpack.c index 1a2bd231e..9216f451e 100644 --- a/src/unpack.c +++ b/src/unpack.c @@ -242,7 +242,7 @@ static inline int template_callback_map(unpack_user* u, unsigned int n, msgpack_ return MSGPACK_UNPACK_NOMEM_ERROR; } - size = (unsigned int)tmp; + size = (unsigned int)tmp; if (*u->z == NULL) { *u->z = msgpack_zone_new(MSGPACK_ZONE_CHUNK_SIZE); @@ -251,7 +251,7 @@ static inline int template_callback_map(unpack_user* u, unsigned int n, msgpack_ } } - // Should size = 0 be an error? If so, what error to return? + // Should size = 0 be an error? If so, what error to return? o->via.map.ptr = (msgpack_object_kv*)msgpack_zone_malloc(*u->z, size); if(o->via.map.ptr == NULL) { return MSGPACK_UNPACK_NOMEM_ERROR; } return 0; From fadc615f4ec21ff8f3fbd2cb2e4d43a58d77d0dc Mon Sep 17 00:00:00 2001 From: David LeBlanc Date: Fri, 19 Apr 2019 14:08:09 -0700 Subject: [PATCH 4/8] Fix typo --- src/unpack.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/unpack.c b/src/unpack.c index 9216f451e..97412a381 100644 --- a/src/unpack.c +++ b/src/unpack.c @@ -235,7 +235,7 @@ static inline int template_callback_map(unpack_user* u, unsigned int n, msgpack_ o->type = MSGPACK_OBJECT_MAP; o->via.map.size = 0; - size = (unsigned long long)n * sizeof(msgpack_object_kv); + tmp = (unsigned long long)n * sizeof(msgpack_object_kv); if (tmp & 0xffffffff00000000) { // integer overflow From 2d54c0e918f22c718c513646ae72fc6231f05231 Mon Sep 17 00:00:00 2001 From: David LeBlanc Date: Mon, 6 May 2019 17:29:40 -0700 Subject: [PATCH 5/8] Change integer overflow check to conform with spec --- src/unpack.c | 33 +++++++++++++++------------------ 1 file changed, 15 insertions(+), 18 deletions(-) diff --git a/src/unpack.c b/src/unpack.c index 97412a381..e4468c115 100644 --- a/src/unpack.c +++ b/src/unpack.c @@ -189,19 +189,17 @@ static inline int template_callback_false(unpack_user* u, msgpack_object* o) static inline int template_callback_array(unpack_user* u, unsigned int n, msgpack_object* o) { - unsigned int size; - unsigned long long tmp; + // Let's leverage the fact that sizeof(msgpack_object) is a compile time constant + // to check for int overflows. + // Note - while n is constrained to 32-bit, the product of n * sizeof(msgpack_object) + // might not be constrained to 4GB on 64-bit systems + if( n > SIZE_MAX/sizeof(msgpack_object)) + return MSGPACK_UNPACK_NOMEM_ERROR; o->type = MSGPACK_OBJECT_ARRAY; o->via.array.size = 0; - tmp = (unsigned long long)n * sizeof(msgpack_object); - if (tmp & 0xffffffff00000000) { - // integer overflow - return MSGPACK_UNPACK_NOMEM_ERROR; - } - - size = (unsigned int)tmp; + size_t size = n * sizeof(msgpack_object); if (*u->z == NULL) { *u->z = msgpack_zone_new(MSGPACK_ZONE_CHUNK_SIZE); @@ -230,19 +228,18 @@ static inline int template_callback_array_item(unpack_user* u, msgpack_object* c static inline int template_callback_map(unpack_user* u, unsigned int n, msgpack_object* o) { - unsigned int size; - unsigned long long tmp; + // Let's leverage the fact that sizeof(msgpack_object_kv) is a compile time constant + // to check for int overflows + // Note - while n is constrained to 32-bit, the product of n * sizeof(msgpack_object) + // might not be constrained to 4GB on 64-bit systems + + if(n > SIZE_MAX/sizeof(msgpack_object_kv)) + return MSGPACK_UNPACK_NOMEM_ERROR; o->type = MSGPACK_OBJECT_MAP; o->via.map.size = 0; - tmp = (unsigned long long)n * sizeof(msgpack_object_kv); - - if (tmp & 0xffffffff00000000) { - // integer overflow - return MSGPACK_UNPACK_NOMEM_ERROR; - } - size = (unsigned int)tmp; + size_t size = n * sizeof(msgpack_object_kv); if (*u->z == NULL) { *u->z = msgpack_zone_new(MSGPACK_ZONE_CHUNK_SIZE); From a2f36898650278c603b96cebb400cca48ff67f8c Mon Sep 17 00:00:00 2001 From: David LeBlanc Date: Mon, 6 May 2019 19:28:25 -0700 Subject: [PATCH 6/8] Older compilers don't allow declaring variables other than at the top of the block if it is a C file. --- src/unpack.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/unpack.c b/src/unpack.c index e4468c115..d89ac29c6 100644 --- a/src/unpack.c +++ b/src/unpack.c @@ -189,6 +189,7 @@ static inline int template_callback_false(unpack_user* u, msgpack_object* o) static inline int template_callback_array(unpack_user* u, unsigned int n, msgpack_object* o) { + size_t size; // Let's leverage the fact that sizeof(msgpack_object) is a compile time constant // to check for int overflows. // Note - while n is constrained to 32-bit, the product of n * sizeof(msgpack_object) @@ -199,7 +200,7 @@ static inline int template_callback_array(unpack_user* u, unsigned int n, msgpac o->type = MSGPACK_OBJECT_ARRAY; o->via.array.size = 0; - size_t size = n * sizeof(msgpack_object); + size = n * sizeof(msgpack_object); if (*u->z == NULL) { *u->z = msgpack_zone_new(MSGPACK_ZONE_CHUNK_SIZE); @@ -228,6 +229,7 @@ static inline int template_callback_array_item(unpack_user* u, msgpack_object* c static inline int template_callback_map(unpack_user* u, unsigned int n, msgpack_object* o) { + size_t size; // Let's leverage the fact that sizeof(msgpack_object_kv) is a compile time constant // to check for int overflows // Note - while n is constrained to 32-bit, the product of n * sizeof(msgpack_object) @@ -239,7 +241,7 @@ static inline int template_callback_map(unpack_user* u, unsigned int n, msgpack_ o->type = MSGPACK_OBJECT_MAP; o->via.map.size = 0; - size_t size = n * sizeof(msgpack_object_kv); + size = n * sizeof(msgpack_object_kv); if (*u->z == NULL) { *u->z = msgpack_zone_new(MSGPACK_ZONE_CHUNK_SIZE); From fcf89fe901a61b4406b42b76e5f127b000308579 Mon Sep 17 00:00:00 2001 From: David LeBlanc Date: Wed, 8 May 2019 17:59:10 -0700 Subject: [PATCH 7/8] Fix tabs, also attempt work-around for compile time constant conditional warning --- src/unpack.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/unpack.c b/src/unpack.c index d89ac29c6..fd5b5b2b1 100644 --- a/src/unpack.c +++ b/src/unpack.c @@ -189,12 +189,12 @@ static inline int template_callback_false(unpack_user* u, msgpack_object* o) static inline int template_callback_array(unpack_user* u, unsigned int n, msgpack_object* o) { - size_t size; + size_t size; // Let's leverage the fact that sizeof(msgpack_object) is a compile time constant // to check for int overflows. // Note - while n is constrained to 32-bit, the product of n * sizeof(msgpack_object) // might not be constrained to 4GB on 64-bit systems - if( n > SIZE_MAX/sizeof(msgpack_object)) + if( (size_t)n > SIZE_MAX/sizeof(msgpack_object)) return MSGPACK_UNPACK_NOMEM_ERROR; o->type = MSGPACK_OBJECT_ARRAY; @@ -229,13 +229,14 @@ static inline int template_callback_array_item(unpack_user* u, msgpack_object* c static inline int template_callback_map(unpack_user* u, unsigned int n, msgpack_object* o) { - size_t size; + size_t size; // Let's leverage the fact that sizeof(msgpack_object_kv) is a compile time constant // to check for int overflows // Note - while n is constrained to 32-bit, the product of n * sizeof(msgpack_object) // might not be constrained to 4GB on 64-bit systems - if(n > SIZE_MAX/sizeof(msgpack_object_kv)) + // Note - this will always be false on 64-bit systems + if((size_t)n > SIZE_MAX/sizeof(msgpack_object_kv)) return MSGPACK_UNPACK_NOMEM_ERROR; o->type = MSGPACK_OBJECT_MAP; From 7a70d749715e30a5b58a3700bedcfc9b62bd4fd3 Mon Sep 17 00:00:00 2001 From: David LeBlanc Date: Wed, 8 May 2019 18:49:55 -0700 Subject: [PATCH 8/8] Use a #define to only check for integer overflow when it is actually possible --- src/unpack.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/unpack.c b/src/unpack.c index fd5b5b2b1..9341cb08a 100644 --- a/src/unpack.c +++ b/src/unpack.c @@ -194,8 +194,10 @@ static inline int template_callback_array(unpack_user* u, unsigned int n, msgpac // to check for int overflows. // Note - while n is constrained to 32-bit, the product of n * sizeof(msgpack_object) // might not be constrained to 4GB on 64-bit systems - if( (size_t)n > SIZE_MAX/sizeof(msgpack_object)) +#if SIZE_MAX == UINT_MAX + if (n > SIZE_MAX/sizeof(msgpack_object)) return MSGPACK_UNPACK_NOMEM_ERROR; +#endif o->type = MSGPACK_OBJECT_ARRAY; o->via.array.size = 0; @@ -236,8 +238,10 @@ static inline int template_callback_map(unpack_user* u, unsigned int n, msgpack_ // might not be constrained to 4GB on 64-bit systems // Note - this will always be false on 64-bit systems - if((size_t)n > SIZE_MAX/sizeof(msgpack_object_kv)) +#if SIZE_MAX == UINT_MAX + if (n > SIZE_MAX/sizeof(msgpack_object_kv)) return MSGPACK_UNPACK_NOMEM_ERROR; +#endif o->type = MSGPACK_OBJECT_MAP; o->via.map.size = 0;