Fix MarkingFunctionsFactory visibility race on static markingFunctions#3625
Open
t-h-i-n-k-er wants to merge 4 commits into
Open
Conversation
MarkingFunctionsFactory.createMarkingFunctions() is synchronized and
reads the static markingFunctions field under the class lock, but
postContruct() writes that same static field without synchronization
and without a volatile declaration on the field. Under the Java Memory
Model there is no happens-before edge from the write in postContruct()
to the read in createMarkingFunctions(), so a reader can observe a
stale null and fall through into the Spring-context-load branch
instead of returning the CDI-provided MarkingFunctions instance.
This commit adds MarkingFunctionsFactoryVisibilityTest which fails on
the current code:
- markingFunctionsFieldIsVolatile: asserts the static
markingFunctions field is declared volatile. Fails because the
field is currently declared without volatile.
- postContructIsSynchronized: asserts postContruct() is declared
synchronized to match createMarkingFunctions(). Fails because
postContruct() is currently declared without synchronized.
Both tests are structural reflection-based assertions that fail
deterministically on the unfixed code and pass after the fix.
Refs NationalSecurityAgency#3601
Declare the static markingFunctions field as volatile and mark postContruct() as synchronized so its write has a happens-before edge with respect to createMarkingFunctions(), which already synchronizes on the class monitor. Without these changes a MarkingFunctions reference published by postContruct() during CDI initialization is not guaranteed to be visible to a concurrent caller of createMarkingFunctions(); the reader can observe a stale null and fall through into the Spring-context-load branch, overwriting the CDI-provided instance with a freshly loaded one or returning null when no Spring context is available. The test added in the previous commit now passes: - markingFunctionsFieldIsVolatile: the field is now declared volatile. - postContructIsSynchronized: postContruct() is now declared synchronized. Refs NationalSecurityAgency#3601
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary:
MarkingFunctionsFactory.createMarkingFunctions() is synchronized and reads the static markingFunctions field under the class lock, but postContruct() writes that same static field without synchronization and without a volatile declaration on the field. Under the Java Memory Model there is no happens-before edge from the write in postContruct() to the read in createMarkingFunctions(), so a concurrent reader can observe a stale null and fall through into the Spring-context-load branch instead of returning the CDI-provided MarkingFunctions instance.
What does this PR do?
Two commits, in the order requested on #3602 — a test that demonstrates the failure condition, and then the commit to fix it:
1: Add failing test demonstrating MarkingFunctionsFactory visibility race — adds MarkingFunctionsFactoryVisibilityTest which fails on the current code:
markingFunctionsFieldIsVolatile | asserts the static markingFunctions field is declared volatile. Fails because the field is currently declared without volatile.
postContructIsSynchronized | asserts postContruct() is declared synchronized to match createMarkingFunctions(). Fails because postContruct() is currently declared without synchronized.
Both tests are structural reflection-based assertions that fail deterministically on the unfixed code and pass after the fix. Verified locally: 2 tests fail on unfixed code, 2 tests pass on fixed code.
2: Fix MarkingFunctionsFactory visibility race on static markingFunctions — declares the static markingFunctions field as volatile and marks postContruct() as synchronized so its write has a happens-before edge with respect to createMarkingFunctions(), which already synchronizes on the class monitor. All tests now pass.
This PR is intentionally narrow — only MarkingFunctionsFactory.java and one new test file. Split out from #3602 per the maintainer's request to keep changes focused. Refs #3601.
Why not just volatile:
volatile alone would make the write visible, but adding synchronized to postContruct() also matches the locking discipline already used on createMarkingFunctions() and prevents the write from racing with the read-check-write sequence inside createMarkingFunctions() (which could otherwise observe a non-null value, skip the Spring load, and then have postContruct() overwrite it with the CDI-provided instance — or vice versa). Using both is the minimal change that makes the contract internally consistent.