Thread problem in TEnum::GetEnum

[Dr15Jones]

Check duplicate issues.

• Checked for duplicates

Description

The following line must be within a R__WRITE_LOCKGUARD(ROOT::gCoreMutex);

root/core/meta/src/TEnum.cxx

Line 259 in e4292a8

TInterpreter::SuspendAutoParsing autoParseRaii(gInterpreter, suspendAutoParse);

Multiple threads could be within a read-lock and ask for the value which is being modified without any synchronization.

Reproducer

Found by visual inspection.

ROOT version

master branch

Installation method

source

Operating system

all

Additional context

This was found while investigating cms-sw/cmssw#48106

[Dr15Jones]

@makortel

[pcanal]

That line is inside a lambda function (searchEnum) which is called only after the lock was taken on line 302 ... However, this is only the read lock .... (pondering).

[pcanal]

However, this is only the read lock .... (pondering).

And that is wrong ... The idea was that the Cling/Clang state (thanks to the disabling of the autoloading) would not change (unless some inner routine asked for the write lock) but the flaw here is that 2 threads getting to that GetEnum function around the same time would 'mess up' the state of the autoparsing flag.

Failure example:

1. thread1 call GetEnum
2. thread1 record autoparsing state as true
3. thread1 set autoparsing state as false
4. thread2 call GetEnum
5. thread2 recording autoparsing state as false
6. thread1 !! restore autoparsing state to true --> Cause problem for thread2
7. thread2 (let's assume its does fail/crash)
8. thread2 !! restore autoparsing state to false -> Cause problem for the rest of the program.

[pcanal]

See #18793

[hahnjo]

Yes, I found the same place earlier this evening. I took the more radical approach

diff --git a/core/meta/src/TEnum.cxx b/core/meta/src/TEnum.cxx
index f3cb8a8f714..8bd2e0e5c99 100644
--- a/core/meta/src/TEnum.cxx
+++ b/core/meta/src/TEnum.cxx
@@ -299,7 +299,7 @@ TEnum *TEnum::GetEnum(const char *enumName, ESearchAction sa)
    // Keep the state consistent.  In particular prevent change in the state of
    // AutoLoading and AutoParsing allowance and gROOT->GetListOfClasses()
    // and the later update/modification to the autoparsing state.
-   R__READ_LOCKGUARD(ROOT::gCoreMutex);
+   R__WRITE_LOCKGUARD(ROOT::gCoreMutex);

    if (lastPos != enumName) {
       // We have a scope

and can confirm this passed 100 iterations of the reproducer in cms-sw/cmssw#48106. I will test again with the more narrowly scoped lock in #18793. Though I do wonder if all the repeated lock upgrading is actually better in the end...

[pcanal]

The intent of the narrowing is too not have to go through the lock upgrading.

[hahnjo]

We already have one write lock that we take unconditionally just to call TClassEdit::GetNormalizedName. Then we take the read lock, and for example TCling::AutoLoad or auto parsing will upgrade it to a write lock again. I'm not sure I see how narrowing helps for that, I argue that a single write lock in the beginning might be more efficient overall in case of heavy contention. And certainly much easier to reason about...