Skip to content

Commit c3068ce

Browse files
abmussetargos
authored andcommitted
deps: fix V8 race condition for AIX
aix: simplify OS::DecommitPages implementation Replace complex mmap/munmap retry logic with mprotect + madvise approach. This fixes a race condition that was causing test failures in Node.js. Node.js stress test was run with this fix and testing shows 0 failures out of 1000 runs of wpt/test-wasm-jsapi with this patch compared to 224 failures without it. Refs: #62647 Refs: https://chromium-review.googlesource.com/c/v8/v8/+/7780464 PR-URL: #61898 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Filip Skokan <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>
1 parent e43581e commit c3068ce

2 files changed

Lines changed: 19 additions & 27 deletions

File tree

common.gypi

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@
4040

4141
# Reset this number to 0 on major V8 upgrades.
4242
# Increment by one for each non-official patch applied to deps/v8.
43-
'v8_embedder_string': '-node.12',
43+
'v8_embedder_string': '-node.13',
4444

4545
##### V8 defaults for Node.js #####
4646

deps/v8/src/base/platform/platform-aix.cc

Lines changed: 18 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -171,34 +171,26 @@ bool OS::DecommitPages(void* address, size_t size) {
171171
// with MAP_FIXED will fail and return -1 unless the application has requested
172172
// SPEC1170 compliant behaviour:
173173
// https://www.ibm.com/docs/en/aix/7.3?topic=m-mmap-mmap64-subroutine
174-
// Therefore in case if failure we need to unmap the address before trying to
175-
// map it again. The downside is another thread could place another mapping at
176-
// the same address after the munmap but before the mmap, therefore a CHECK is
177-
// also added to assure the address is mapped successfully. Refer to the
178-
// comments under https://crrev.com/c/3010195 for more details.
179-
#define MMAP() \
180-
mmap(address, size, PROT_NONE, MAP_FIXED | MAP_ANONYMOUS | MAP_PRIVATE, -1, 0)
174+
// As a workaround we use `mprotect` to make the page inaccessible and
175+
// `madvise` to hint the OS to release the memory.
176+
//
177+
// NOTE: On AIX, madvise() is a no-op and does not release physical
178+
// memory. The pages remain protected but the physical memory may only be
179+
// reclaimed by the OS under memory pressure. This trade-off is acceptable
180+
// because:
181+
// 1. It fixes critical race conditions (Refs:
182+
// https://github.com/nodejs/node/issues/62647)
183+
// 2. AIX's disclaim64() requires writable pages, incompatible with PROT_NONE
184+
// protection
185+
// 3. The alternative munmap/mmap approach causes "Check failed: ptr ==
186+
// address" errors
187+
// 4. Pages are inaccessible, preventing memory corruption
188+
181189
DCHECK_EQ(0, reinterpret_cast<uintptr_t>(address) % CommitPageSize());
182190
DCHECK_EQ(0, size % CommitPageSize());
183-
void* ptr;
184-
// Try without mapping first.
185-
ptr = MMAP();
186-
if (ptr != address) {
187-
DCHECK_EQ(ptr, MAP_FAILED);
188-
// Returns 0 when successful.
189-
if (munmap(address, size)) {
190-
return false;
191-
}
192-
// Try again after unmap.
193-
ptr = MMAP();
194-
// If this check fails it's most likely due to a racing condition where
195-
// another thread has mapped the same address right before we do.
196-
// Since this could cause hard-to-debug issues, potentially with security
197-
// impact, and we can't recover from this, the best we can do is abort the
198-
// process.
199-
CHECK_EQ(ptr, address);
200-
}
201-
#undef MMAP
191+
if (mprotect(address, size, PROT_NONE) != 0) return false;
192+
if (madvise(static_cast<caddr_t>(address), size, MADV_DONTNEED) != 0)
193+
return false;
202194
return true;
203195
}
204196

0 commit comments

Comments
 (0)