This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP]Fix race condition in omp_init_lock
ClosedPublic

Authored by adam-azarchs on Oct 30 2017, 4:48 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

adam-azarchs created this revision.Oct 30 2017, 4:48 PM
omalyshe added inline comments.
runtime/test/lock/omp_init_lock.c
19 ↗(On Diff #120911)

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

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 ↗(On Diff #120911)

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 added inline comments.
testsuite/omp_testsuite.h
49 ↗(On Diff #120911)

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

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.

adam-azarchs marked an inline comment as done.Oct 31 2017, 3:08 PM
This revision is now accepted and ready to land.Nov 1 2017, 2:24 AM

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

OK, I will commit the patch.

Thanks.

This revision was automatically updated to reflect the committed changes.