microsoft / microsoft/mimalloc

mem_region_s::commit is out of sync with VirtualAlloc on Windows

Open
#336 4 comments 0 reactions 0 assignees View on GitHub
Dominant language
C
Stars
13.4k
Forks
1.2k
Avg merge
4d 45m
Merged PRs (30d)
13

Description

We found another issue with out of memory recovering, unfortunately we cannot provide reproducible example outside of our environment again, but after investigation we found out that mem_region_s::commit is out of sync with VirtualAlloc in out of memory state, and a non allocated/committed segment from mi_region_try_alloc (1, true, ...) is being returned.
Debug build crashes on
```
mimalloc/src/region.c(330): if (*commit) { ((uint8_t*)p)[0] = 0; }
```
Release build crashes in user code when returned pointer is dereferenced.
It could be fixed with this:
```
diff --git a/src/region.c b/src/region.c
index b6d0da3..0f24478 100644
--- a/src/region.c
+++ b/src/region.c
@@ -295,11 +295,12 @@ static void* mi_region_try_alloc(size_t blocks, bool* commit, bool* is_large, bo
// ensure commit
bool any_uncommitted;
mi_bitmap_claim(®ion->commit, 1, blocks, bit_idx, &any_uncommitted);
- if (any_uncommitted) {
+ if (any_uncommitted || (1 == blocks)) {
mi_assert_internal(!info.x.is_large);
bool commit_zero = false;
if (!_mi_mem_commit(p, blocks * MI_SEGMENT_SIZE, &commit_zero, tld)) {
```
but this fix negatively affected performance, so we ended up with another fix:
```
diff --git a/src/options.c b/src/options.c
index 9da3a9b..5ec9eef 100644
--- a/src/options.c
+++ b/src/options.c
@@ -66,7 +66,7 @@ static mi_option_desc_t options[_mi_option_last] =
// the following options are experimental and not all combinations make sense.
{ 1, UNINIT, MI_OPTION(eager_commit) }, // commit per segment directly (4MiB) (but see also `eager_commit_delay`)
- #if defined(_WIN32) || (MI_INTPTR_SIZE <= 4) // and other OS's without overcommit?
+ #if (MI_INTPTR_SIZE <= 4) // and other OS's without overcommit?
{ 0, UNINIT, MI_OPTION(eager_region_commit) },
{ 1, UNINIT, MI_OPTION(reset_decommits) }, // reset decommits memory
#else
```
where we got slightly improved performance and no crash.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start in src/region.c at mi_region_try_alloc and the commit check around line 330, then compare its behavior with VirtualAlloc during out-of-memory conditions. Review src/options.c and the eager_region_commit setting; done means no unallocated or uncommitted segment is returned on Windows while avoiding the reported performance regression.

Written by the indexing model from the issue text.

Assessment

Tech stack
c
Domain
operating-systems, performance
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
42/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.