Skip to content

scudo: fails to build on linux with error: returning variable 'QuarantineCache' by reference requires holding mutex 'Mutex' #67796

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
sylvestre opened this issue Sep 29, 2023 · 3 comments
Closed

scudo: fails to build on linux with error: returning variable 'QuarantineCache' by reference requires holding mutex 'Mutex' #67796

sylvestre opened this issue Sep 29, 2023 · 3 comments
Labels
build-problem compiler-rt:scudo Scudo Hardened Allocator

Comments

@sylvestre
Copy link
Collaborator

On trunk - 6dd96d6 on linux:

In file included from /build/source/compiler-rt/lib/scudo/standalone/combined.h:24:
/build/source/compiler-rt/lib/scudo/standalone/tsd.h:74:12: error: returning variable 'QuarantineCache' by reference requires holding mutex 'Mutex' exclusively [-Werror,-Wthread-safety-reference]
   74 |     return QuarantineCache;
      |            ^
/build/source/compiler-rt/lib/scudo/standalone/combined.h:248:28: note: in instantiation of member function 'scudo::TSD<scudo::Allocator<scudo::DefaultConfig, &malloc_postinit>>::getQuarantineCache' requested here
  248 |     Quarantine.drain(&TSD->getQuarantineCache(),
      |                            ^
/build/source/compiler-rt/lib/scudo/standalone/tsd.h:57:15: note: in instantiation of member function 'scudo::Allocator<scudo::DefaultConfig, &malloc_postinit>::commitBack' requested here
   57 |     Instance->commitBack(this);
      |               ^
/build/source/compiler-rt/lib/scudo/standalone/tsd_exclusive.h:172:27: note: in instantiation of member function 'scudo::TSD<scudo::Allocator<scudo::DefaultConfig, &malloc_postinit>>::commitBack' requested here
  172 |   TSDRegistryT::ThreadTSD.commitBack(Instance);
      |                           ^
/build/source/compiler-rt/lib/scudo/standalone/tsd_exclusive.h:33:46: note: in instantiation of function template specialization 'scudo::teardownThread<scudo::Allocator<scudo::DefaultConfig, &malloc_postinit>>' requested here
   33 |     CHECK_EQ(pthread_key_create(&PThreadKey, teardownThread<Allocator>), 0);
      |                                              ^
/build/source/compiler-rt/lib/scudo/standalone/tsd_exclusive.h:42:5: note: in instantiation of member function 'scudo::TSDRegistryExT<scudo::Allocator<scudo::DefaultConfig, &malloc_postinit>>::init' requested here
   42 |     init(Instance); // Sets Initialized.
      |     ^
/build/source/compiler-rt/lib/scudo/standalone/tsd_exclusive.h:130:5: note: in instantiation of member function 'scudo::TSDRegistryExT<scudo::Allocator<scudo::DefaultConfig, &malloc_postinit>>::initOnceMaybe' requested here
  130 |     initOnceMaybe(Instance);
      |     ^
/build/source/compiler-rt/lib/scudo/standalone/tsd_exclusive.h:74:5: note: in instantiation of member function 'scudo::TSDRegistryExT<scudo::Allocator<scudo::DefaultConfig, &malloc_postinit>>::initThread' requested here
   74 |     initThread(Instance, MinimalInit);
      |     ^
/build/source/compiler-rt/lib/scudo/standalone/combined.h:221:17: note: in instantiation of member function 'scudo::TSDRegistryExT<scudo::Allocator<scudo::DefaultConfig, &malloc_postinit>>::initThreadMaybe' requested here
  221 |     TSDRegistry.initThreadMaybe(this, MinimalInit);
      |                 ^
/build/source/compiler-rt/lib/scudo/standalone/combined.h:790:5: note: in instantiation of member function 'scudo::Allocator<scudo::DefaultConfig, &malloc_postinit>::initThreadMaybe' requested here
  790 |     initThreadMaybe();
      |     ^
/build/source/compiler-rt/lib/scudo/standalone/wrappers_c.inc:36:25: note: in instantiation of member function 'scudo::Allocator<scudo::DefaultConfig, &malloc_postinit>::canReturnNull' requested here
   36 |     if (SCUDO_ALLOCATOR.canReturnNull()) {
@sylvestre sylvestre added compiler-rt:scudo Scudo Hardened Allocator build-problem labels Sep 29, 2023
@sylvestre
Copy link
Collaborator Author

@ChiaHungDuan @fabio-d you touched these files recently, rings a bell?

@ChiaHungDuan
Copy link
Contributor

The thread-safety annotation was added at the beginning of this year 2023. And the code,

  typename Allocator::QuarantineCacheT &getQuarantineCache()                        
      ASSERT_CAPABILITY(Mutex) {                                                                                                                                                                                   
    return QuarantineCache;                                                         
  }

The function has been marked as ASSERT_CAPABILITY(Mutex) which I suppose that it already assumes the lock held. Does that patch change the behavior?

@ChiaHungDuan ChiaHungDuan mentioned this issue Oct 5, 2023
Merged
ChiaHungDuan added a commit that referenced this issue Oct 5, 2023
@ChiaHungDuan
[scudo] Fix the use of ASSERT_CAPABILITY in TSD (#68273)
ea2036e 
In getCache()/getQuarantineCache(), they return a reference to variable
guarded by a mutex. After #67776, thread-safey analysis checks if a
variable return by reference has the lock held. The ASSERT_CAPABILITY
only claims after calling that function, the lock will be held. But not
asserting that the lock is held *before* calling that function.

In the patch, we switch to use REQUIRES() and assertLocked() to mark the
code paths. Also remove the misused ASSERT_CAPABILITY.

Fixes #67795, #67796
@ChiaHungDuan
Copy link
Contributor

This has been fixed by #68273

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
build-problem compiler-rt:scudo Scudo Hardened Allocator
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@sylvestre @ChiaHungDuan