From 3c4d0d387f1aeffd73f5faef286ddd12039c4be6 Mon Sep 17 00:00:00 2001 From: Derek Bruening Date: Tue, 15 Jan 2019 19:34:30 -0500 Subject: [PATCH] i#3346 trace exit: avoid deadlock in check_in_last_thread_vm_area Removes the lock acquisition from check_in_last_thread_vm_area to avoid deadlock. The scenario we're trying to catch will read and match with no extra lock due to the bb building lock. It would be nice to have a test but it is not easy to write due to the race that needs to be arranged. Issue: #3346 --- core/vmareas.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/core/vmareas.c b/core/vmareas.c index b492847cf46..f76be72f7f0 100644 --- a/core/vmareas.c +++ b/core/vmareas.c @@ -1,5 +1,5 @@ /* ********************************************************** - * Copyright (c) 2010-2018 Google, Inc. All rights reserved. + * Copyright (c) 2010-2019 Google, Inc. All rights reserved. * Copyright (c) 2002-2010 VMware, Inc. All rights reserved. * **********************************************************/ @@ -8237,17 +8237,21 @@ check_in_last_thread_vm_area(dcontext_t *dcontext, app_pc pc) } /* last decoded app pc may be in last shared area instead */ if (!in_last && DYNAMO_OPTION(shared_bbs)) { - /* FIXME: bad to grab on failure path... - * can we assume only grabbed then, and not synch? + /* We avoid the high-ranked shared_vm_areas lock which can easily cause + * rank order violations (i#3346). We're trying to catch the scenario + * where a shared bb is being built and we fault decoding it. There, + * the bb building lock will prevent another thread from changing the + * shared last_area, so we should hit when reading w/o the lock. + * The risk of falsely matching with a half-updated or mismatched + * racily read last_area bounds seems lower than the risk of problems + * if we grab the lock. */ - SHARED_VECTOR_RWLOCK(&shared_data->areas, read, lock); if (is_readable_without_exception((app_pc)&shared_data->last_area->end, 4) && is_readable_without_exception((app_pc)&shared_data->last_area->start, 4)) { /* we can walk off to the next page */ in_last = (pc < shared_data->last_area->end + MAX_INSTR_LENGTH && shared_data->last_area->start <= pc); } - SHARED_VECTOR_RWLOCK(&shared_data->areas, read, unlock); } /* the last decoded app pc may be in the last decoded page or the page after * if the instr crosses a page boundary. This can help us more gracefully