Java: Fix FP in "Time-of-check time-of-use race condition" (java/toctou-race-condition)

[owen-mc]

Fixed a false positive in "Time-of-check time-of-use race condition" (java/toctou-race-condition) where a field of a non-static class was not considered always-locked if it was accessed in a constructor.

For an example, see the added tests.

[Copilot]

Pull Request Overview

This PR fixes a false positive in the "Time-of-check time-of-use race condition" query by adjusting the handling of non-static field accesses in constructors.

• Added tests in FieldAlwaysLocked.java and FieldNotAlwaysLocked.java to verify correct locking behavior.
• Updated Test.java with alert markers and added documentation in the change-notes file.

Reviewed Changes

Copilot reviewed 4 out of 7 changed files in this pull request and generated no comments.

File	Description
java/ql/test/query-tests/security/CWE-367/semmle/tests/FieldAlwaysLocked.java	New test verifying that field initialization in constructors is treated as always-locked
java/ql/test/query-tests/security/CWE-367/semmle/tests/FieldNotAlwaysLocked.java	New test demonstrating a scenario where the field is not always locked
java/ql/src/change-notes/2025-03-13-fix-toctou-false-positive.md	Added change note documenting the fix
java/ql/test/query-tests/security/CWE-367/semmle/tests/Test.java	Updated test cases with alert markers to trigger the query

Files not reviewed (3)

• java/ql/src/Security/CWE/CWE-367/TOCTOURace.ql: Language not supported
• java/ql/test/query-tests/security/CWE-367/semmle/tests/TOCTOURace.expected: Language not supported
• java/ql/test/query-tests/security/CWE-367/semmle/tests/TOCTOURace.qlref: Language not supported

Tip: If you use Visual Studio Code, you can request a review from Copilot before you push from the "Source Control" tab. Learn more

[owen-mc]

Hmm, maybe we need to add a restriction that Field.getDeclaringType() = thisType. It's probably always true because it would be rare for a field to not be accessed at all in its declaring class.

We are trying to encode the logic that "f is only accessed in synchronized methods on its declaring class (or the equivalent with synchronized statements)". We have already excluded initializers.

My reasoning is:

• The constructor will be run by one thread.
• Other threads won't normally get access to the object until after the constructor has finished
  • (In theory you could leak the this reference from the constructor, but I think we can assume that doesn't happen.)
• Therefore, other threads won't be able to call methods that reference f until the constructor has finished.

So, my conclusion is: the code relating to this probably assumed that f is a field of thisType, and that's probably almost always implied by the code; as it stands, I think the code is okay even if f is not a field of thisType; but if we make the change above to exclude constructors then we probably do want to add the restriction that f is a field of thisType to keep correctness.

[aschackmull]

maybe we need to add a restriction that Field.getDeclaringType() = thisType

No, that should not be necessary - it's not completely inconceivable to synchronize on a this from some other class. As long as that's done consistently, then it's fine.

What we might need instead, though, is to check that the Constructor belongs to the same class as the field. And in a similar vein, we probably ought to check the same for the InitializerMethod.
Also, this extension from InitializerMethods to Constructors also applies to the two other disjuncts.
So at this point we should probably factor out a helper predicate identifying field accesses that aren't in constructors or initializers of the declaring type of the field, and use that as the common limitation in all three forexs.

[aschackmull]

LGTM

[owen-mc]

I did variant analysis before and after on the top 1000 java repos. Before we had 400 results and now we have 388. I spot-checked some of the lost alerts and they looked like FPs of the kind we were trying to get rid of.