[OpenMP]Fix race condition in omp_init_lock

[adam-azarchs]

Summary

This is a partial fix for bug 34050.

This prevents callers of omp_set_lock (which does not hold __kmp_global_lock) from
ever seeing an uninitialized version of __kmp_i_lock_table.table.

It does not solve a use-after-free race condition if omp_set_lock obtains a pointer to
__kmp_i_lock_table.table before it is updated and then attempts to dereference
afterwards. That race is far less likely and can be handled in a separate patch.

The unit test usually segfaults on the current trunk revision. It passes with the patch.

[omalyshe]

runtime/test/lock/omp_init_lock.c

19

Should be omp_destroy_lock(), right?
With this change I don't see the crash. The test successfully finishes with REPETITIONS=10000

[AndreyChurbanov]

Failure to reproduce the bug does not mean there is no bug:). The library code change looks good, but please fix the test.

testsuite/omp_testsuite.h

49

I am not an expert in testsuite test system, but I would guess that this change could break it. Because you added test references without adding the test itself.

Couple more notes:

• this header file is auto generated, according to the comment at the beginning of the file;
• this folder looks outdated in general, as all new tests related activity and LIT testing happens in the runtime/test location, so I doubt you need to touch this file.

[Hahnfeld]

testsuite/omp_testsuite.h

49

That was also my impression - should we get rid of the testsuite folder completely?

[adam-azarchs]

Thanks everyone for the switch response and comments.

This fixes the test to properly destroy the locks when done.
REPITITIONS was previously not doing anything for this test - the
test is (indirectly) exercising the global lock pool, which does not
get re-initialized with each repetition. This restructuring of the
test makes more sense from that perspective. I've again
confirmed that the test fails without the patch to kmp_lock.cpp
and passes with the patch.

I've also removed the edit to testsuite, as suggested.

[AndreyChurbanov]

LGTM

[adam-azarchs]

Thank you! I do not have commit access so someone else will have to do that.

[AndreyChurbanov]

OK, I will commit the patch.

Thanks.

runtime/src/kmp_lock.cpp

if (__kmp_indirect_lock_pool[tag] != NULL) {
     KA_TRACE(20, ("__kmp_allocate_indirect_lock: reusing an existing lock %p\n",
                   lck));
   } else {
     idx = __kmp_i_lock_table.next;
     // Check capacity and double the size if it is full
     if (idx == __kmp_i_lock_table.size) {
       // Double up the space for block pointers
       int row = __kmp_i_lock_table.size / KMP_I_LOCK_CHUNK;
-      kmp_indirect_lock_t **old_table = __kmp_i_lock_table.table;
-      __kmp_i_lock_table.table = (kmp_indirect_lock_t **)__kmp_allocate(
+      kmp_indirect_lock_t **new_table = (kmp_indirect_lock_t **)__kmp_allocate(
           2 * row * sizeof(kmp_indirect_lock_t *));
-      KMP_MEMCPY(__kmp_i_lock_table.table, old_table,
+      KMP_MEMCPY(new_table, __kmp_i_lock_table.table,
                  row * sizeof(kmp_indirect_lock_t *));
+      kmp_indirect_lock_t **old_table = __kmp_i_lock_table.table;
+      __kmp_i_lock_table.table = new_table;
       __kmp_free(old_table);
       // Allocate new objects in the new blocks
       for (int i = row; i < 2 * row; ++i)
         *(__kmp_i_lock_table.table + i) = (kmp_indirect_lock_t *)__kmp_allocate(
             KMP_I_LOCK_CHUNK * sizeof(kmp_indirect_lock_t));
       __kmp_i_lock_table.size = 2 * idx;
     }
     __kmp_i_lock_table.next++;

runtime/test/lock/omp_init_lock.c

+// RUN: %libomp-compile-and-run
+#include "omp_testsuite.h"
+#include <stdio.h>
+
+int test_omp_init_lock() {
+  int i;
+#pragma omp parallel for schedule(static) num_threads(NUM_TASKS)
+  for (i = 0; i < LOOPCOUNT; i++) {
+    int j;
+    omp_lock_t lcks[1000];
+    for (j = 0; j < 1000; j++) {
+      omp_init_lock(&lcks[j]);
+    }
+    for (j = 0; j < 1000 * 100; j++) {
+      omp_set_lock(&lcks[j / 100]);
+      omp_unset_lock(&lcks[j / 100]);
+    }
+    for (j = 0; j < 1000; j++) {
+      omp_init_lock(&lcks[j]);
+    }
+  }
+
+  return 1;
+}
+
+int main() {
+  int i;
+  int num_failed = 0;
+
+  for (i = 0; i < REPETITIONS; i++) {
+    if (!test_omp_init_lock()) {
+      num_failed++;
+    }
+  }
+  return num_failed;
+}

testsuite/omp_testsuite.h

 int test_omp_get_num_threads(FILE * logfile);  /* Test for omp_get_num_threads */
 int crosstest_omp_get_num_threads(FILE * logfile);  /* Crosstest for omp_get_num_threads */
 int test_omp_section_private(FILE * logfile);  /* Test for omp section private */
 int crosstest_omp_section_private(FILE * logfile);  /* Crosstest for omp section private */
 int test_omp_parallel_if(FILE * logfile);  /* Test for omp parallel if */
 int crosstest_omp_parallel_if(FILE * logfile);  /* Crosstest for omp parallel if */
 int test_omp_lock(FILE * logfile);  /* Test for omp_lock */
 int crosstest_omp_lock(FILE * logfile);  /* Crosstest for omp_lock */
+int test_omp_init_lock(FILE * logfile);  /* Test for omp_init_lock */
+int crosstest_omp_init_lock(FILE * logfile);  /* Crosstest for omp_init_lock */
 int test_omp_parallel_shared(FILE * logfile);  /* Test for omp parallel shared */
 int crosstest_omp_parallel_shared(FILE * logfile);  /* Crosstest for omp parallel shared */
 int test_omp_task_imp_shared(FILE * logfile);  /* Test for omp task */
 int crosstest_omp_task_imp_shared(FILE * logfile);  /* Crosstest for omp task */
 int test_omp_task_private(FILE * logfile);  /* Test for omp task private */
 int crosstest_omp_task_private(FILE * logfile);  /* Crosstest for omp task private */
 int test_omp_section_lastprivate(FILE * logfile);  /* Test for omp section lastprivate */
 int crosstest_omp_section_lastprivate(FILE * logfile);  /* Crosstest for omp section lastprivate */