Skip to content

Commit b525480

Browse files
authored
i#3538: Fix flakiness in static_maps_mixup_novars (#7959)
The static_maps_mixup_novars is unable to create a fake issue in finding the DR library bounds on my local machine, and the execution just completes successfully, not generating the expected error message. Improves the copy_and_remap logic in static_maps_mixup.c which needs to break DR's logic for finding library bounds for the purpose of the static_maps_mixup_novars test, in a way that keeps the static_maps_mixup_yesvars version still working. We simply add the writable bit to the first segment mapping to achieve this. With this there's a hang in debug build. Adds a guard to get_dynamo_library_bounds to try getting the bounds only once and during init. This is to prevent a scenario where the 'expected elf header' ASSERT in memquery_library_bounds fails, which triggers d_r_internal_error -> report_dynamorio_problem -> privload_print_modules -> get_dynamorio_dll_start -> get_dynamo_library_bounds again. Ensures that a failure to find the ELF header is propagated properly in the release build. Also adjusts the expected failure message for the debug build, which actually fails at an ASSERT. Would be good to observe the test's flakiness over a few days before removing the "flaky" suffix. Issue: #3538
1 parent 3dfb2f9 commit b525480

5 files changed

Lines changed: 28 additions & 7 deletions

File tree

core/unix/memquery.c

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/* *******************************************************************************
2-
* Copyright (c) 2010-2025 Google, Inc. All rights reserved.
2+
* Copyright (c) 2010-2026 Google, Inc. All rights reserved.
33
* Copyright (c) 2011 Massachusetts Institute of Technology All rights reserved.
44
* Copyright (c) 2000-2010 VMware, Inc. All rights reserved.
55
* *******************************************************************************/
@@ -233,6 +233,11 @@ memquery_library_bounds_by_iterator_internal(
233233
}
234234
} else {
235235
ASSERT(false && "expected elf header");
236+
/* Return failure in release build. */
237+
count = 0;
238+
mod_start = NULL;
239+
cur_end = NULL;
240+
break;
236241
}
237242
}
238243
count++;
@@ -250,7 +255,7 @@ memquery_library_bounds_by_iterator_internal(
250255
* maps file), but not every library has one. We have to parse the ELF
251256
* header to know since we can't assume that a subsequent anonymous
252257
* region is .bss. */
253-
if (image_size != 0 && cur_end - mod_start < image_size) {
258+
if (image_size != 0 && cur_end - mod_start < image_size && cur_end != NULL) {
254259
if (iter.comment[0] != '\0') {
255260
/* There's something else in the text-data gap: xref i#2641. */
256261
} else {

core/unix/os.c

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9812,8 +9812,16 @@ extern int dynamorio_so_start, dynamorio_so_end;
98129812
static void
98139813
get_dynamo_library_bounds(void)
98149814
{
9815+
ASSERT(!dynamo_initialized);
9816+
static bool attempted_dynamo_library_bounds = false;
98159817
if (dynamorio_library_filepath[0] != '\0') /* Already cached. */
98169818
return;
9819+
/* Prevent infinite recursion if bounds recovery fails and triggers an assert,
9820+
* which in turn tries to print modules and query bounds again.
9821+
*/
9822+
if (attempted_dynamo_library_bounds)
9823+
return;
9824+
attempted_dynamo_library_bounds = true;
98179825
/* Note that we're not counting DYNAMORIO_PRELOAD_NAME as a DR area, to match
98189826
* Windows, so we should unload it like we do there. The other reason not to
98199827
* count it is so is_in_dynamo_dll() can be the only exception to the

suite/tests/CMakeLists.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3682,6 +3682,7 @@ if (NOT ANDROID AND NOT APPLE AND NOT RISCV64)
36823682
append_link_flags(api.static_maps_mixup_yesvars
36833683
"-Wl,--defsym,dynamorio_so_start=__executable_start -Wl,--defsym,dynamorio_so_end=end")
36843684

3685+
# TODO i#3538: Remove the _FLAKY suffix after ensuring no failures remaining.
36853686
set(api.static_maps_mixup_novars_FLAKY_expectbase
36863687
"static_maps_mixup_novars")
36873688
tobuild_api(api.static_maps_mixup_novars_FLAKY

suite/tests/api/static_maps_mixup.c

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/* **********************************************************
2-
* Copyright (c) 2019 Google, Inc. All rights reserved.
2+
* Copyright (c) 2019-2026 Google, Inc. All rights reserved.
33
* **********************************************************/
44

55
/*
@@ -90,7 +90,12 @@ find_exe_bounds(app_pc *base, app_pc *end)
9090
}
9191

9292
// Confusing the current logic in get_dynamo_library_bounds is as simple as
93-
// overwriting our first mapping with an anonymously-mapped version.
93+
// overwriting our first mapping (which contains the ELF header) with an
94+
// anonymously-mapped version with different protections.
95+
//
96+
// By making it anonymous, the segment loses its filename in /proc/self/maps.
97+
// DR's heuristic will still try to take a valid ELF header under certain
98+
// conditions; we add the writable bit to prevent that.
9499
static void
95100
copy_and_remap(void *base, size_t offs, size_t size)
96101
{
@@ -100,8 +105,6 @@ copy_and_remap(void *base, size_t offs, size_t size)
100105
assert(p != MAP_FAILED);
101106
void *dst = (byte *)base + offs;
102107
memcpy(p, dst, size);
103-
int res = mprotect(p, size, PROT_EXEC | PROT_READ);
104-
assert(res == 0);
105108
void *loc = mremap(p, size, size, MREMAP_MAYMOVE | MREMAP_FIXED, dst);
106109
assert(loc == dst);
107110
}

suite/tests/api/static_maps_mixup_novars.templatex

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,4 +2,8 @@ mix up maps
22
remap base=0x[0-9a-f]+, offs=0, sz=[1-9][0-9]*
33
pre-DR init
44
/* Depending on the environment, sometimes we hit an assert, sometimes a segfault */
5-
(<Application .*api.static_maps_mixup_novars(_FLAKY)? \([0-9]+\). Failed to find DynamoRIO library bounds.>)?
5+
#ifdef DEBUG
6+
.*check failure: .*expected elf header.*
7+
#else
8+
.*Failed to find DynamoRIO library bounds.*
9+
#endif

0 commit comments

Comments
 (0)