Project

General

Profile

Bug #3114

Using malloc(size_max) gives strange results

Added by ddegroot 8 months ago. Updated 8 months ago.

Status:
Closed
Priority:
Normal
Assignee:
-
Category:
Userland
Target version:
Start date:
12/18/2017
Due date:
% Done:

0%

Estimated time:

Description

While porting d-lang dmd/druntime/phobos to DragonFlyBSD, it became apparent that using 'malloc(size_t)' to deduce malloc and alignment rules, gave some unexpected results.

Example:
malloc size:9223372036854775807, malloc failed, ptr == NULL, errno:12 // expected result (INTPTR_MAX)
malloc size:72036854775808, ptr == 0x800800000 // this is fine
malloc size:18446744073709551613, ptr == 0x800455000 // unexpected UINTPTR_MAX / SIZE_MAX

Related dlang:druntime PR: https://github.com/dlang/druntime/pull/1999

test_malloc.c (1.01 KB) test_malloc.c malloc test c file ddegroot, 12/18/2017 09:25 AM
test_malloc_results.txt (1.97 KB) test_malloc_results.txt results from malloc test ddegroot, 12/18/2017 09:25 AM
nmalloc.c.diff (1.81 KB) nmalloc.c.diff diff against /lib/libc/stdlib/nmalloc.c ddegroot, 12/30/2017 04:21 PM
test_malloc.c (1.11 KB) test_malloc.c ddegroot, 12/31/2017 04:14 AM

History

#1 Updated by ddegroot 8 months ago

I have had a look at the libc/stdlib/malloc.c code, and came to the conclusion that i would not be able to create a patch that would definitively fix this type of malloc/calloc/realloc issue.

An interesting link, regarding other allocators and this exact same issue: http://kqueue.org/blog/2012/03/05/memory-allocator-security-revisited/. This might provide some inspiration and potential test cases.

#2 Updated by ddegroot 8 months ago

This seems to resolve the issue. But i am not quite sure if this is the right way.

Notes:
- i am doing the check twice at this moment, only the slaballoc one should be required.
- in _slaballoc size is rewritten and it might be better to use a separate local variable instead. This would help with bug tracing. Changing the size that was requested makes it harder to track what is happening, and prevents later checks if so required. (BTW: there are multiple (other) locations where the requested size is rewritten as well).
- in _slaballoc size is not checked for overflow (i think), before it is rewritten (line 922).

With this patch in place the results resemble the Linux Results exactly.

```
diff --git a/lib/libc/stdlib/nmalloc.c b/lib/libc/stdlib/nmalloc.c
index b39aaf301..d9bc90fb8 100644
--- a/lib/libc/stdlib/nmalloc.c
+++ b/lib/libc/stdlib/nmalloc.c
@@ -753,6 +753,8 @@ zoneindex(size_t *bytes, size_t *chunking)
return(0);
}

+#define MUL_NO_OVERFLOW (1UL << (sizeof(size_t) * 4))
+
/*
* malloc() - call internal slab allocator
*/
@@ -761,6 +763,11 @@ __malloc(size_t size)
{
void *ptr;

+ if ((size >= MUL_NO_OVERFLOW ) || (SIZE_MAX < size)) {
+ errno = ENOMEM;
+ return(NULL);
+ }
+
ptr = _slaballoc(size, 0);
if (ptr == NULL)
errno = ENOMEM;
@@ -769,8 +776,6 @@ __malloc(size_t size)
return(ptr);
}

-#define MUL_NO_OVERFLOW (1UL << (sizeof(size_t) * 4))
-
/*
* calloc() - call internal slab allocator
*/
@@ -982,6 +987,9 @@ _slaballoc(size_t size, int flags)
bigalloc_t big;
bigalloc_t *bigp;

+ if ((size >= MUL_NO_OVERFLOW ) || (SIZE_MAX < size) ) {
+ return(NULL);
+ }
/*
* Page-align and cache-color in case of virtually indexed
* physically tagged L1 caches (aka SandyBridge). No sweat
@@ -989,7 +997,8 @@ _slaballoc(size_t size, int flags)
*
* (don't count as excess).
*/
- size = (size + PAGE_MASK) & ~(size_t)PAGE_MASK;
+ size = (size + PAGE_MASK) & ~(size_t)PAGE_MASK; /* Note: Changing size, without checking overflow.
+ also might be better to use a different variable instead of the original request size */

/*
* If we have overflown above when rounding to the page
```

#3 Updated by ddegroot 8 months ago

Update test_malloc.c

#4 Updated by ddegroot 8 months ago

  • Status changed from New to Closed

Overflow issue was already fixed by zrj in master.
Results do not match 100% with what linux does, but that should be ok.

Also available in: Atom PDF