Skip to content

Issue #876: Fix integer overflow on slice_count #877

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

Closed
wants to merge 1 commit into from

Conversation

vstinner
Copy link

@vstinner vstinner commented Apr 15, 2024

If the slice count doesn't fit into uint32_t, consider that the memory allocation failed.

On Linux s390x, allocating around 8,589,934,592 GiB with mmap() works thanks to overcommit on a machine with 8 GiB of memory:

mmap(NULL,
     0x8000000000400000,
     PROT_READ | PROT_WRITE,
     MAP_PRIVATE | MAP_ANONYMOUS | MAP_NORESERVE,
     -1, 0)

If the slice count doesn't fit into uint32_t, consider that the
memory allocation failed.

On Linux s390x, allocating around 8,589,934,592 GiB with mmap() works
thanks to overcommit on a machine with 8 GiB of memory:

    mmap(NULL,
         0x8000000000400000,
         PROT_READ | PROT_WRITE,
         MAP_PRIVATE | MAP_ANONYMOUS | MAP_NORESERVE,
         -1, 0)
@vstinner
Copy link
Author

microsoft.mimalloc Failing after 3m — Build #20240415.2 failed

Hum, it doesn't seem to be related to my change:

1: =================================================================
1: ==1864==ERROR: LeakSanitizer: detected memory leaks
1: 
1: Direct leak of 4097 byte(s) in 1 object(s) allocated from:
1:     #0 0x56446f10728e in __interceptor_malloc (/home/vsts/work/1/s/debug-asan-clang/mimalloc-test-api+0xa228e) (BuildId: f63f94b5f9da6b5b7cc6c667411035f8024a4297)
1:     #1 0x56446f0bb0f6 in __interceptor_realpath (/home/vsts/work/1/s/debug-asan-clang/mimalloc-test-api+0x560f6) (BuildId: f63f94b5f9da6b5b7cc6c667411035f8024a4297)
1:     #2 0x7f8fd7c0f4c1 in mi_heap_realpath /home/vsts/work/1/s/src/alloc.c:396:19
1:     #3 0x7f8fd7c0f544 in mi_realpath /home/vsts/work/1/s/src/alloc.c:420:10
1:     #4 0x56446f1458f9 in main /home/vsts/work/1/s/test/test-api.c:299:15
1:     #5 0x7f8fd7829d8f  (/lib/x86_64-linux-gnu/libc.so.6+0x29d8f) (BuildId: c289da5071a3399de893d2af81d6a30c62646e1e)
1: 
1: SUMMARY: AddressSanitizer: 4097 byte(s) leaked in 1 allocation(s).

daanx added a commit that referenced this pull request Apr 19, 2024
@daanx
Copy link
Collaborator

daanx commented Apr 19, 2024

Thanks so much for spotting this! I fixed it differently by defining a MI_MAX_ALLOC_SIZE and limiting it to at most MI_SEGMENT_SLICE_SIZE * (UINT32_MAX-1) (and adding assertions). If you can, can you verify this works for your tests?

@vstinner
Copy link
Author

I fixed it differently by defining a MI_MAX_ALLOC_SIZE and limiting it to at most MI_SEGMENT_SLICE_SIZE * (UINT32_MAX-1) (and adding assertions).

Do you have a link to the fix? 5050b63 only defines MI_MAX_ALLOC_SIZE as PTRDIFF_MAX. Is it the same fix?

@daanx
Copy link
Collaborator

daanx commented Apr 20, 2024

Ah, for dev PTRDIFF_MAX is still the limit as it has no "slices". (dev = v1.x),
but for the dev-slice branch the new limit is UINT32_MAX as in the commit 78418b3. (dev-slice == v2.x).

@vstinner
Copy link
Author

If you can, can you verify this works for your tests?

Sorry, but it's quite complicated to set up the test :-/ Thanks for the fix anyway!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants