Page MenuHomePhabricator

Fixed memory use-after-free problem.
ClosedPublic

Authored by AndreyChurbanov on Jun 20 2019, 6:25 AM.

Details

Summary

Bug reported in https://bugs.llvm.org/show_bug.cgi?id=42269.

Problem caused by recent attempt to fix memory leak on the same memory block.
Investigation showed that worker threads may leave contention group either before or after the master thread depending on the number of teams in the teams construct.
This patch makes freeing of the contention group structure conditional for master thread, and adds similar conditional freeing for worker threads. Thus both the memory leak and the use-after-free problems are fixed now.

Do not add test because the problem was only visible if library is instrumented by memory sanitizer.

Diff Detail

Repository
rL LLVM

Event Timeline

Hahnfeld added inline comments.
runtime/src/kmp_csupport.cpp
444 ↗(On Diff #205789)

Is this safe without atomics? I've only briefly looked over the code, most other usages of cg_nthreads are protected by __kmp_forkjoin_lock (at least according to comments). I don't see this here, but maybe I'm missing the relevant call.

AndreyChurbanov marked an inline comment as done.Jun 21 2019, 6:42 AM
AndreyChurbanov added inline comments.
runtime/src/kmp_csupport.cpp
444 ↗(On Diff #205789)

I cannot technically prove this unfortunately, but looks like there is no need in synchronization here. At least for now. The reason is - for a particular contention group (CG) when master thread touches the counter its workers either already been freed of wait in a hot team. In both cases no actions can be performed in parallel with the counter.

Trying to look in more details, I found couple more cases when library produces memory leak and use-after-free problems. I'll update the patch shortly

Though the code still has problems those should be addressed in other patches. Things like many consecutive target-teams regions executed on host, or target-teams on host with following parallel region are not correctly maintained currently. To be fixed later.

Covered more cases caused memory leak (when target-teams region is followed by parallel with bigger number of threads) and use-after-free problem (when nested hot teams requested via KMP_HOT_TEAMS_MAX_LEVEL=2 and num_teams is bigger than 1). The leak is fixed by freeing CG structure when worker threads inherit master's CG but had earlier non-NULL CG. Use-after-free fixed by making one more freeing of CG conditional (in __kmp_free_team routine).

This patch fixes the bug for our usecase.
Thanks!

The fixes look good overall, but I'd like approval from somebody more familiar with this part of the code.

For the decrement operations, you could actually use pre-decrement and then compare to zero; my 2 cents.

runtime/src/kmp_csupport.cpp
444 ↗(On Diff #205789)

Ok, sounds enough for now.

hbae accepted this revision.Jun 26 2019, 7:18 AM

LGTM.

This revision is now accepted and ready to land.Jun 26 2019, 7:18 AM
Closed by commit rL364456: Fixed memory use-after-free problem. (authored by achurbanov, committed by ). · Explain WhyJun 26 2019, 11:13 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 26 2019, 11:13 AM