Resolution for issue #28746: changed order in which locks/flags are grabbed in MallocHooks to preve…

Resolution for issue #28746: changed order in which locks/flags are grabbed in MallocHooks to prevent race where a MallocHookScope tries to set the flag but MallocHookScope::TearDown deletes it before it has the opportunity to.

BUG=
R=zra@google.com

Committed: https://github.com/dart-lang/sdk/commit/b59cca17e7e1fb59667e172f32fd74ca9933f533

[bkonyi, 2017-02-17 18:34:49]

bkonyi@google.com changed reviewers:
+ zra@google.com

[zra, 2017-02-17 18:41:02]

https://codereview.chromium.org/2701013002/diff/1/runtime/vm/malloc_hooks.cc
File runtime/vm/malloc_hooks.cc (right):

https://codereview.chromium.org/2701013002/diff/1/runtime/vm/malloc_hooks.cc#...
runtime/vm/malloc_hooks.cc:275: MutexLocker
ml(MallocHooksState::malloc_hook_mutex());
I think taking the log and checking Active() needs to come even before
MallocHookScope::IsInHook() etc above, otherwise we could hit the assert in
MallocHookScope::IsInHook().

https://codereview.chromium.org/2701013002/diff/1/runtime/vm/malloc_hooks.cc#...
runtime/vm/malloc_hooks.cc:291: MutexLocker
ml(MallocHooksState::malloc_hook_mutex());
ditto

[bkonyi, 2017-02-17 21:15:11]

https://codereview.chromium.org/2701013002/diff/1/runtime/vm/malloc_hooks.cc
File runtime/vm/malloc_hooks.cc (right):

https://codereview.chromium.org/2701013002/diff/1/runtime/vm/malloc_hooks.cc#...
runtime/vm/malloc_hooks.cc:275: MutexLocker
ml(MallocHooksState::malloc_hook_mutex());
On 2017/02/17 18:41:02, zra wrote:
> I think taking the log and checking Active() needs to come even before
> MallocHookScope::IsInHook() etc above, otherwise we could hit the assert in
> MallocHookScope::IsInHook().

We can't take the lock before anything else, or else we'll end up in a deadlock.
See comments in newest patch set for my proposed solution.

https://codereview.chromium.org/2701013002/diff/1/runtime/vm/malloc_hooks.cc#...
runtime/vm/malloc_hooks.cc:291: MutexLocker
ml(MallocHooksState::malloc_hook_mutex());
On 2017/02/17 18:41:02, zra wrote:
> ditto

See above.

https://codereview.chromium.org/2701013002/diff/20001/runtime/vm/malloc_hooks.cc
File runtime/vm/malloc_hooks.cc (right):

https://codereview.chromium.org/2701013002/diff/20001/runtime/vm/malloc_hooks...
runtime/vm/malloc_hooks.cc:51: MutexLocker ml(malloc_hook_scope_mutex_);
I added a mutex to the MallocHookScope class so that the flag can't be destroyed
while we're checking it or trying to set it.

https://codereview.chromium.org/2701013002/diff/20001/runtime/vm/malloc_hooks...
runtime/vm/malloc_hooks.cc:66: return in_malloc_hook_flag_ !=
kUnsetThreadLocalKey;
I'm guessing this needs parens around the boolean expression, right?

https://codereview.chromium.org/2701013002/diff/20001/runtime/vm/malloc_hooks...
runtime/vm/malloc_hooks.cc:298: MallocHookScope::Active()) {
This extra condition, MallocHookScope::Active(), is just to ensure that the
MallocHookScope flag wasn't destroyed since the last time we checked it.

[zra, 2017-02-17 21:29:53]

https://codereview.chromium.org/2701013002/diff/20001/runtime/vm/malloc_hooks.cc
File runtime/vm/malloc_hooks.cc (right):

https://codereview.chromium.org/2701013002/diff/20001/runtime/vm/malloc_hooks...
runtime/vm/malloc_hooks.cc:237: MallocHooksState::TearDown();
 !MallocHooksState::Active() implies !MallocHookScope::Active(), so there is no
need to check both conditions below.

https://codereview.chromium.org/2701013002/diff/20001/runtime/vm/malloc_hooks...
runtime/vm/malloc_hooks.cc:298: MallocHookScope::Active()) {
On 2017/02/17 21:15:11, bkonyi wrote:
> This extra condition, MallocHookScope::Active(), is just to ensure that the
> MallocHookScope flag wasn't destroyed since the last time we checked it. 

I don't think it's possible that we could have MallocHooksState::Active() and
!MallocHookScope::Active(). See above.

https://codereview.chromium.org/2701013002/diff/20001/runtime/vm/malloc_hooks...
runtime/vm/malloc_hooks.cc:316: MallocHookScope::Active()) {
ditto

[zra, 2017-02-17 21:42:26]

lgtm

https://codereview.chromium.org/2701013002/diff/20001/runtime/vm/malloc_hooks.cc
File runtime/vm/malloc_hooks.cc (right):

https://codereview.chromium.org/2701013002/diff/20001/runtime/vm/malloc_hooks...
runtime/vm/malloc_hooks.cc:64: static bool Active() {
rm

[bkonyi, 2017-02-17 21:49:20]

Description was changed from

==========
Resolution for issue #28746: changed order in which locks/flags are grabbed in
MallocHooks to prevent race where a MallocHookScope tries to set the flag but
MallocHookScope::TearDown deletes it before it has the opportunity to.

BUG=
==========

to

==========
Resolution for issue #28746: changed order in which locks/flags are grabbed in
MallocHooks to prevent race where a MallocHookScope tries to set the flag but
MallocHookScope::TearDown deletes it before it has the opportunity to.

BUG=
R=zra@google.com

Committed:
https://github.com/dart-lang/sdk/commit/b59cca17e7e1fb59667e172f32fd74ca9933f533
==========

[bkonyi, 2017-02-17 21:49:21]

Committed patchset #3 (id:40001) manually as
b59cca17e7e1fb59667e172f32fd74ca9933f533 (presubmit successful).