[Openmp-commits] [PATCH] D63599: Fixed memory use-after-free problem.

Fri Jun 21 09:09:13 PDT 2019

Hahnfeld added a comment.

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.

================
Comment at: runtime/src/kmp_csupport.cpp:444
+  KMP_DEBUG_ASSERT(tmp->cg_nthreads);
+  int i = tmp->cg_nthreads--;
+  if (i == 1) { // check is we are the last thread in CG (not always the case)
----------------
AndreyChurbanov wrote:
> Hahnfeld wrote:
> > 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.
> 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.
Ok, sounds enough for now.

Repository:
  rOMP OpenMP

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63599/new/

https://reviews.llvm.org/D63599