Fixed memory use-after-free problem.

[AndreyChurbanov]

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.

[Hahnfeld]

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]

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.

[AndreyChurbanov]

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).

[protze.joachim]

This patch fixes the bug for our usecase.
Thanks!

[Hahnfeld]

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]

LGTM.

openmp/trunk/runtime/src/kmp_csupport.cpp

#endif
 
   // Pop current CG root off list
   KMP_DEBUG_ASSERT(this_thr->th.th_cg_roots);
   kmp_cg_root_t *tmp = this_thr->th.th_cg_roots;
   this_thr->th.th_cg_roots = tmp->up;
   KA_TRACE(100, ("__kmpc_fork_teams: Thread %p popping node %p and moving up"
                  " to node %p. cg_nthreads was %d\n",
                  this_thr, tmp, this_thr->th.th_cg_roots, tmp->cg_nthreads));
+  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)
     __kmp_free(tmp);
+  }
   // Restore current task's thread_limit from CG root
   KMP_DEBUG_ASSERT(this_thr->th.th_cg_roots);
   this_thr->th.th_current_task->td_icvs.thread_limit =
       this_thr->th.th_cg_roots->cg_thread_limit;
 
   this_thr->th.th_teams_microtask = NULL;
   this_thr->th.th_teams_level = 0;
   *(kmp_int64 *)(&this_thr->th.th_teams_size) = 0L;

openmp/trunk/runtime/src/kmp_runtime.cpp

if (!this_thr->th.th_pri_common) {
     }
     this_thr->th.th_pri_head = NULL;
   }
 
   if (this_thr != master && // Master's CG root is initialized elsewhere
       this_thr->th.th_cg_roots != master->th.th_cg_roots) { // CG root not set
     // Make new thread's CG root same as master's
     KMP_DEBUG_ASSERT(master->th.th_cg_roots);
+    kmp_cg_root_t *tmp = this_thr->th.th_cg_roots;
+    if (tmp) {
+      // worker changes CG, need to check if old CG should be freed
+      int i = tmp->cg_nthreads--;
+      KA_TRACE(100, ("__kmp_initialize_info: Thread %p decrement cg_nthreads"
+                     " on node %p of thread %p to %d\n",
+                     this_thr, tmp, tmp->cg_root, tmp->cg_nthreads));
+      if (i == 1) {
+        __kmp_free(tmp); // last thread left CG --> free it
+      }
+    }
     this_thr->th.th_cg_roots = master->th.th_cg_roots;
     // Increment new thread's CG root's counter to add the new thread
     this_thr->th.th_cg_roots->cg_nthreads++;
     KA_TRACE(100, ("__kmp_initialize_info: Thread %p increment cg_nthreads on"
                    " node %p of thread %p to %d\n",
                    this_thr, this_thr->th.th_cg_roots,
                    this_thr->th.th_cg_roots->cg_root,
                    this_thr->th.th_cg_roots->cg_nthreads));
if (team->t.t_threads[1]->th.th_cg_roots->cg_root == team->t.t_threads[1]) {
         KMP_DEBUG_ASSERT(thr && thr->th.th_cg_roots &&
                          thr->th.th_cg_roots->cg_root == thr);
         // Pop current CG root off list
         kmp_cg_root_t *tmp = thr->th.th_cg_roots;
         thr->th.th_cg_roots = tmp->up;
         KA_TRACE(100, ("__kmp_free_team: Thread %p popping node %p and moving"
                        " up to node %p. cg_nthreads was %d\n",
                        thr, tmp, thr->th.th_cg_roots, tmp->cg_nthreads));
-        __kmp_free(tmp);
+        int i = tmp->cg_nthreads--;
+        if (i == 1) {
+          __kmp_free(tmp); // free CG if we are the last thread in it
+        }
         // Restore current task's thread_limit from CG root
         if (thr->th.th_cg_roots)
           thr->th.th_current_task->td_icvs.thread_limit =
               thr->th.th_cg_roots->cg_thread_limit;
       }
     }
   }
 
while (this_th->th.th_cg_roots) {
     kmp_cg_root_t *tmp = this_th->th.th_cg_roots;
     if (tmp->cg_root == this_th) { // Thread is a cg_root
       KMP_DEBUG_ASSERT(tmp->cg_nthreads == 0);
       KA_TRACE(
           5, ("__kmp_free_thread: Thread %p freeing node %p\n", this_th, tmp));
       this_th->th.th_cg_roots = tmp->up;
       __kmp_free(tmp);
     } else { // Worker thread
+      if (tmp->cg_nthreads == 0) { // last thread leaves contention group
+        __kmp_free(tmp);
+      }
       this_th->th.th_cg_roots = NULL;
       break;
     }
   }
 
   /* If the implicit task assigned to this thread can be used by other threads
    * -> multiple threads can share the data and try to free the task at
    * __kmp_reap_thread at exit. This duplicate use of the task data can happen
void __kmp_teams_master(int gtid) {
 
   // This thread is a new CG root.  Set up the proper variables.
   kmp_cg_root_t *tmp = (kmp_cg_root_t *)__kmp_allocate(sizeof(kmp_cg_root_t));
   tmp->cg_root = thr; // Make thr the CG root
   // Init to thread limit that was stored when league masters were forked
   tmp->cg_thread_limit = thr->th.th_current_task->td_icvs.thread_limit;
   tmp->cg_nthreads = 1; // Init counter to one active thread, this one
   KA_TRACE(100, ("__kmp_teams_master: Thread %p created node %p and init"
-                 " cg_threads to 1\n",
+                 " cg_nthreads to 1\n",
                  thr, tmp));
   tmp->up = thr->th.th_cg_roots;
   thr->th.th_cg_roots = tmp;
 
 // Launch league of teams now, but not let workers execute
 // (they hang on fork barrier until next parallel)
 #if INCLUDE_SSC_MARKS
   SSC_MARK_FORKING();