Verify that the requested allocation size does not overflow. #109

malsbat · 2018-07-23T16:33:16Z

Signed-off-by: Todd Malsbary todd.malsbary@intel.com

Signed-off-by: Todd Malsbary <todd.malsbary@intel.com>

wjwwood

The change to string map might be fine, I had one question about it. @dhood might have more context on the change to the logging.

wjwwood · 2018-07-24T05:01:41Z

src/logging.c

@@ -417,9 +417,17 @@ void rcutils_log(
 ) \
  size_t old_output_buffer_len = strlen(output_buffer); \
  size_t required_output_buffer_size = old_output_buffer_len + n + 1; \
+  if (required_output_buffer_size <= old_output_buffer_len) { \


Is this the overflow case? If so a comment would be good to indicate this.

Yes, this is the overflow case. I can add a comment.

wjwwood · 2018-07-24T05:25:35Z

src/string_map.c

@@ -166,6 +167,12 @@ rcutils_string_map_reserve(rcutils_string_map_t * string_map, size_t capacity)
    // note that realloc when the pointer is NULL is the same as malloc
    // note also that realloc will shrink the space if needed

+    // ensure that reallocate won't overflow capacity
+    if (capacity > (SIZE_MAX / sizeof(char *))) {


Can you explain in what cases this might occur?

I did not trace down all the callers of rcutils_string_map_reserve so it may not occur in practice today. However, it may occur if the requested capacity is greater than SIZE_MAX / 4 or SIZE_MAX / 8, depending on the pointer size.

I'll also note that glibc provides an explicit reallocarray() function that is the same as realloc() but includes the overflow check as done here.

We can consider adding a wrapper function in rcutils/allocator.h to preform this check if you like, I already created a reallocf function along these lines:

rcutils/include/rcutils/allocator.h

Lines 129 to 137 in 244fa98

/// Emulate the behavior of [reallocf](https://linux.die.net/man/3/reallocf).

/**

* This function will return `NULL` if the allocator is `NULL` or has `NULL` for

* function pointer fields.

*/

RCUTILS_PUBLIC

RCUTILS_WARN_UNUSED

void *

rcutils_reallocf(void * pointer, size_t size, rcutils_allocator_t * allocator);

rcutils/src/allocator.c

Lines 100 to 117 in 244fa98

void *

rcutils_reallocf(void * pointer, size_t size, rcutils_allocator_t * allocator)

{

if (!rcutils_allocator_is_valid(allocator)) {

// cannot deallocate pointer, so print message to stderr and return NULL

#if RCUTILS_REPORT_ERROR_HANDLING_ERRORS

RCUTILS_SAFE_FWRITE_TO_STDERR(

"[rcutils|allocator.c:" RCUTILS_STRINGIFY(__LINE__) "] rcutils_reallocf(): "

"invalid allocator or allocator function pointers, memory leaked\n");

#endif

return NULL;

}

void * new_pointer = allocator->reallocate(pointer, size, allocator->state);

if (NULL == new_pointer) {

allocator->deallocate(pointer, allocator->state);

}

return new_pointer;

}

Otherwise, this check here seems fine.

malsbat

Thanks William for taking a look.

malsbat · 2018-07-24T17:12:32Z

src/logging.c

@@ -417,9 +417,17 @@ void rcutils_log(
 ) \
  size_t old_output_buffer_len = strlen(output_buffer); \
  size_t required_output_buffer_size = old_output_buffer_len + n + 1; \
+  if (required_output_buffer_size <= old_output_buffer_len) { \


Yes, this is the overflow case. I can add a comment.

malsbat · 2018-07-24T17:20:28Z

src/string_map.c

@@ -166,6 +167,12 @@ rcutils_string_map_reserve(rcutils_string_map_t * string_map, size_t capacity)
    // note that realloc when the pointer is NULL is the same as malloc
    // note also that realloc will shrink the space if needed

+    // ensure that reallocate won't overflow capacity
+    if (capacity > (SIZE_MAX / sizeof(char *))) {


I did not trace down all the callers of rcutils_string_map_reserve so it may not occur in practice today. However, it may occur if the requested capacity is greater than SIZE_MAX / 4 or SIZE_MAX / 8, depending on the pointer size.

I'll also note that glibc provides an explicit reallocarray() function that is the same as realloc() but includes the overflow check as done here.

wjwwood

lgtm with the comment and ideally with @dhood's feedback, but once you make the comment I'll start some CI for you

wjwwood · 2018-07-24T18:55:27Z

src/string_map.c

@@ -166,6 +167,12 @@ rcutils_string_map_reserve(rcutils_string_map_t * string_map, size_t capacity)
    // note that realloc when the pointer is NULL is the same as malloc
    // note also that realloc will shrink the space if needed

+    // ensure that reallocate won't overflow capacity
+    if (capacity > (SIZE_MAX / sizeof(char *))) {


We can consider adding a wrapper function in rcutils/allocator.h to preform this check if you like, I already created a reallocf function along these lines:

rcutils/include/rcutils/allocator.h

Lines 129 to 137 in 244fa98

/// Emulate the behavior of [reallocf](https://linux.die.net/man/3/reallocf).

/**

* This function will return `NULL` if the allocator is `NULL` or has `NULL` for

* function pointer fields.

*/

RCUTILS_PUBLIC

RCUTILS_WARN_UNUSED

void *

rcutils_reallocf(void * pointer, size_t size, rcutils_allocator_t * allocator);

rcutils/src/allocator.c

Lines 100 to 117 in 244fa98

void *

rcutils_reallocf(void * pointer, size_t size, rcutils_allocator_t * allocator)

{

if (!rcutils_allocator_is_valid(allocator)) {

// cannot deallocate pointer, so print message to stderr and return NULL

#if RCUTILS_REPORT_ERROR_HANDLING_ERRORS

RCUTILS_SAFE_FWRITE_TO_STDERR(

"[rcutils|allocator.c:" RCUTILS_STRINGIFY(__LINE__) "] rcutils_reallocf(): "

"invalid allocator or allocator function pointers, memory leaked\n");

#endif

return NULL;

}

void * new_pointer = allocator->reallocate(pointer, size, allocator->state);

if (NULL == new_pointer) {

allocator->deallocate(pointer, allocator->state);

}

return new_pointer;

}

Otherwise, this check here seems fine.

Signed-off-by: Todd Malsbary <todd.malsbary@intel.com>

wjwwood · 2018-07-24T22:09:28Z

Thanks here's some CI:

Linux
Linux-aarch64
macOS
Windows

wjwwood · 2018-07-25T23:03:33Z

Ok, I'm going to go ahead and merge this. @dhood if you have any retroactive comments we can address them in a follow up pr.

dhood · 2018-07-25T23:08:03Z

no comment, looks good!

Verify that the requested allocation size does not overflow.

fe1255e

Signed-off-by: Todd Malsbary <todd.malsbary@intel.com>

dirk-thomas added the in review Waiting for review (Kanban column) label Jul 23, 2018

wjwwood reviewed Jul 24, 2018

View reviewed changes

malsbat commented Jul 24, 2018

View reviewed changes

wjwwood approved these changes Jul 24, 2018

View reviewed changes

Add comment to clarify overflow calculation.

488e398

Signed-off-by: Todd Malsbary <todd.malsbary@intel.com>

wjwwood merged commit d757928 into ros2:master Jul 25, 2018

wjwwood removed the in review Waiting for review (Kanban column) label Jul 25, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Verify that the requested allocation size does not overflow. #109

Verify that the requested allocation size does not overflow. #109

malsbat commented Jul 23, 2018

wjwwood left a comment

wjwwood Jul 24, 2018

malsbat Jul 24, 2018

wjwwood Jul 24, 2018

malsbat Jul 24, 2018

wjwwood Jul 24, 2018

malsbat left a comment

malsbat Jul 24, 2018

malsbat Jul 24, 2018

wjwwood left a comment

wjwwood Jul 24, 2018

wjwwood commented Jul 24, 2018

wjwwood commented Jul 25, 2018

dhood commented Jul 25, 2018

	/// Emulate the behavior of [reallocf](https://linux.die.net/man/3/reallocf).
	/**
	* This function will return `NULL` if the allocator is `NULL` or has `NULL` for
	* function pointer fields.
	*/
	RCUTILS_PUBLIC
	RCUTILS_WARN_UNUSED
	void *
	rcutils_reallocf(void * pointer, size_t size, rcutils_allocator_t * allocator);

	void *
	rcutils_reallocf(void * pointer, size_t size, rcutils_allocator_t * allocator)
	{
	if (!rcutils_allocator_is_valid(allocator)) {
	// cannot deallocate pointer, so print message to stderr and return NULL
	#if RCUTILS_REPORT_ERROR_HANDLING_ERRORS
	RCUTILS_SAFE_FWRITE_TO_STDERR(
	"[rcutils\|allocator.c:" RCUTILS_STRINGIFY(__LINE__) "] rcutils_reallocf(): "
	"invalid allocator or allocator function pointers, memory leaked\n");
	#endif
	return NULL;
	}
	void * new_pointer = allocator->reallocate(pointer, size, allocator->state);
	if (NULL == new_pointer) {
	allocator->deallocate(pointer, allocator->state);
	}
	return new_pointer;
	}

Verify that the requested allocation size does not overflow. #109

Verify that the requested allocation size does not overflow. #109

Conversation

malsbat commented Jul 23, 2018

wjwwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malsbat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wjwwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wjwwood commented Jul 24, 2018

wjwwood commented Jul 25, 2018

dhood commented Jul 25, 2018