Skip to content
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.

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

Clarify support for exclusive_trylock_function annotation on constructors #92408

Open
apasel422 opened this issue May 16, 2024 · 1 comment
Open

Comments

@apasel422
Copy link

The mutex demo in the Thread Safety Analysis documentation demonstrates a number of MutexLocker constructors that assume various properties about the given mutex, but it notably omits a constructor that performs TryLock, instead providing that as a method on MutexLocker itself.

In attempting to address a bug related to a similar scoped lockable that tries to lock a given mutex we considered the possibility of annotating the constructor with exclusive_trylock_function(true) and adding an implicit operator bool() that returns whether locking succeeded.

I'm not sure whether the analyzer should support something like this, but is it ever appropriate today to mark a constructor with exclusive_trylock_function, given that constructors don't return anything? If not, could we make it an error to help prevent misuse, similar to the way it is prohibited on anything other than a function?

aarongable pushed a commit to chromium/chromium that referenced this issue May 16, 2024
Prior to this CL, base::AutoTryLock's constructor was annotated with
EXCLUSIVE_LOCK_FUNCTION, a Thread Safety Analysis [1] annotation that
indicates that the function infallibly acquires the lock. In reality, it
just *tries* to acquire the lock, so potentially-unguarded access was
flying under the radar.

It appears it's structurally impossible to annotate a constructor as a
fallible locking operation. The EXCLUSIVE_TRYLOCK_FUNCTION(...)
annotation exists, but it requires a boolean parameter corresponding to
the function's return type. Constructors have no return type, so it
seems this annotation simply cannot be applied to constructors. This
problem is described in a newly-filed LLVM issue [2].

As a workaround, this CL annotates `bool AutoLock::is_acquired()` with
EXCLUSIVE_TRYLOCK_FUNCTION(true) instead of the constructor. The
accessor does not really acquire the lock, but that's an implementation
detail from the caller's perspective. The effect is that the analysis
now requires callers to check is_acquired() before using guarded data.

[1]: https://clang.llvm.org/docs/ThreadSafetyAnalysis.html
[2]: llvm/llvm-project#92408

Bug: 340196356
Change-Id: I98a37e9c9260eee417250190b770a2e2d8cee720
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5542379
Commit-Queue: Dan McArdle <dmcardle@chromium.org>
Reviewed-by: Hongchan Choi <hongchan@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1301979}
@pkasting
Copy link
Member

What we ultimately landed here has its own downsides: it suggests that the "is the lock acquired?" oracle function is EXCLUSIVE_TRYLOCK_FUNCTION, which is misleading (but understandable), but worse, the destructor is marked as UNLOCK_FUNCTION, which is strictly false if the lock was not acquired, and in theory could cause bogus warnings (though the comments on ~MutexLocker() in the example suggest it won't).

Ultimately, IMO, the right solution is something like a new SCOPED_TRYLOCKABLE annotation to go with SCOPED_LOCKABLE, which would take an argument that is an expression which should evaluate to true iff the lock is acquired.

github-actions bot pushed a commit to kaidokert/chrome_base_mirror that referenced this issue May 17, 2024
Prior to this CL, base::AutoTryLock's constructor was annotated with
EXCLUSIVE_LOCK_FUNCTION, a Thread Safety Analysis [1] annotation that
indicates that the function infallibly acquires the lock. In reality, it
just *tries* to acquire the lock, so potentially-unguarded access was
flying under the radar.

It appears it's structurally impossible to annotate a constructor as a
fallible locking operation. The EXCLUSIVE_TRYLOCK_FUNCTION(...)
annotation exists, but it requires a boolean parameter corresponding to
the function's return type. Constructors have no return type, so it
seems this annotation simply cannot be applied to constructors. This
problem is described in a newly-filed LLVM issue [2].

As a workaround, this CL annotates `bool AutoLock::is_acquired()` with
EXCLUSIVE_TRYLOCK_FUNCTION(true) instead of the constructor. The
accessor does not really acquire the lock, but that's an implementation
detail from the caller's perspective. The effect is that the analysis
now requires callers to check is_acquired() before using guarded data.

[1]: https://clang.llvm.org/docs/ThreadSafetyAnalysis.html
[2]: llvm/llvm-project#92408

Bug: 340196356
Change-Id: I98a37e9c9260eee417250190b770a2e2d8cee720
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5542379
Commit-Queue: Dan McArdle <dmcardle@chromium.org>
Reviewed-by: Hongchan Choi <hongchan@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1301979}
NOKEYCHECK=True
GitOrigin-RevId: f97ab60f89600edf9068e5a7a87c6413009de5ed
jrguzman-ms pushed a commit to msft-mirror-aosp/platform.external.libchrome that referenced this issue May 18, 2024
Prior to this CL, base::AutoTryLock's constructor was annotated with
EXCLUSIVE_LOCK_FUNCTION, a Thread Safety Analysis [1] annotation that
indicates that the function infallibly acquires the lock. In reality, it
just *tries* to acquire the lock, so potentially-unguarded access was
flying under the radar.

It appears it's structurally impossible to annotate a constructor as a
fallible locking operation. The EXCLUSIVE_TRYLOCK_FUNCTION(...)
annotation exists, but it requires a boolean parameter corresponding to
the function's return type. Constructors have no return type, so it
seems this annotation simply cannot be applied to constructors. This
problem is described in a newly-filed LLVM issue [2].

As a workaround, this CL annotates `bool AutoLock::is_acquired()` with
EXCLUSIVE_TRYLOCK_FUNCTION(true) instead of the constructor. The
accessor does not really acquire the lock, but that's an implementation
detail from the caller's perspective. The effect is that the analysis
now requires callers to check is_acquired() before using guarded data.

[1]: https://clang.llvm.org/docs/ThreadSafetyAnalysis.html
[2]: llvm/llvm-project#92408

Bug: 340196356
Change-Id: I98a37e9c9260eee417250190b770a2e2d8cee720
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5542379
Commit-Queue: Dan McArdle <dmcardle@chromium.org>
Reviewed-by: Hongchan Choi <hongchan@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1301979}


CrOS-Libchrome-Original-Commit: f97ab60f89600edf9068e5a7a87c6413009de5ed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants