Added confidential checker JDK annotations#224
Open
iywang2016 wants to merge 50 commits into
Open
Conversation
# Conflicts: # src/java.base/share/classes/java/lang/Object.java # src/java.base/share/classes/org/checkerframework/checker/confidential/qual/BottomConfidential.java # src/java.base/share/classes/org/checkerframework/checker/confidential/qual/Confidential.java # src/java.base/share/classes/org/checkerframework/checker/confidential/qual/NonConfidential.java # src/java.base/share/classes/org/checkerframework/checker/confidential/qual/PolyConfidential.java # src/java.base/share/classes/org/checkerframework/checker/confidential/qual/UnknownConfidential.java # src/java.base/share/classes/org/checkerframework/checker/sqlquotes/qual/SqlEvenQuotes.java # src/java.base/share/classes/org/checkerframework/checker/sqlquotes/qual/SqlOddQuotes.java # src/java.base/share/classes/org/checkerframework/checker/sqlquotes/qual/SqlQuotesBottom.java # src/java.base/share/classes/org/checkerframework/checker/sqlquotes/qual/SqlQuotesUnknown.java
📝 WalkthroughWalkthroughThe change adds confidentiality Checker Framework annotations and applies them in the changed ranges: java.lang.String (imports, Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/java.base/share/classes/java/lang/Exception.java`:
- Line 28: Subclasses of java.lang.Exception (notably IOException and
ReflectiveOperationException) must mirror the new `@PolyConfidential` constructor
signatures and module annotations: update each subclass's constructors that
accept message/cause (e.g., IOException(String message), IOException(String
message, Throwable cause), ReflectiveOperationException(...) and similar in
other Exception subclasses) to annotate the message and cause parameters with
`@PolyConfidential`, and add "confidential" to their `@AnnotatedFor` declarations so
they satisfy the Exception contract; ensure every super(...) call remains
compatible with the annotated types.
In `@src/java.base/share/classes/java/util/Map.java`:
- Around line 274-275: The implementations of Map.get() are missing the
`@PolyConfidential` annotation on their receiver which violates the interface
contract declared by Map.get(`@PolyConfidential` ... this) and breaks type
checking. For each implementing class (HashMap, LinkedHashMap, AbstractMap,
IdentityHashMap, WeakHashMap, TreeMap, EnumMap, ConcurrentHashMap) update the
get(...) method signature to include the `@PolyConfidential` annotation on the
receiver (matching Map.get's receiver annotation style, e.g., add
`@PolyConfidential` to the this parameter alongside existing
`@GuardSatisfied/`@UnknownSignedness annotations) and add any required imports so
the method signatures match the interface contract.
In `@src/java.logging/share/classes/java/util/logging/Handler.java`:
- Line 29: Add the `@NonConfidential` annotation to each override of the
publish(LogRecord record) method in the listed Handler subclasses so their
signatures match the annotated parent: update FileHandler.publish(LogRecord
record), SocketHandler.publish(LogRecord record),
StreamHandler.publish(LogRecord record), MemoryHandler.publish(LogRecord
record), ConsoleHandler.publish(LogRecord record), and
sun.rmi.runtime.Log.publish(LogRecord record) to include `@NonConfidential` on the
method declaration.
In `@src/java.logging/share/classes/java/util/logging/Logger.java`:
- Line 28: Remove the duplicate import of
org.checkerframework.checker.confidential.qual.NonConfidential in Logger.java:
locate the import declarations at the top of the file (related to class Logger)
and delete the redundant second occurrence so only a single import of
`@NonConfidential` remains.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 88f7bb0d-c5ca-4cdc-928f-4a2404844bb2
📒 Files selected for processing (18)
src/java.base/share/classes/java/io/PrintStream.javasrc/java.base/share/classes/java/lang/Exception.javasrc/java.base/share/classes/java/lang/Object.javasrc/java.base/share/classes/java/lang/RuntimeException.javasrc/java.base/share/classes/java/lang/String.javasrc/java.base/share/classes/java/lang/Throwable.javasrc/java.base/share/classes/java/util/Map.javasrc/java.base/share/classes/java/util/Optional.javasrc/java.base/share/classes/java/util/stream/Stream.javasrc/java.base/share/classes/module-info.javasrc/java.base/share/classes/org/checkerframework/checker/confidential/qual/BottomConfidential.javasrc/java.base/share/classes/org/checkerframework/checker/confidential/qual/Confidential.javasrc/java.base/share/classes/org/checkerframework/checker/confidential/qual/NonConfidential.javasrc/java.base/share/classes/org/checkerframework/checker/confidential/qual/PolyConfidential.javasrc/java.base/share/classes/org/checkerframework/checker/confidential/qual/UnknownConfidential.javasrc/java.logging/share/classes/java/util/logging/Formatter.javasrc/java.logging/share/classes/java/util/logging/Handler.javasrc/java.logging/share/classes/java/util/logging/Logger.java
👮 Files not reviewed due to content moderation or server errors (5)
- src/java.logging/share/classes/java/util/logging/Formatter.java
- src/java.base/share/classes/org/checkerframework/checker/confidential/qual/BottomConfidential.java
- src/java.base/share/classes/java/lang/Object.java
- src/java.base/share/classes/java/lang/String.java
- src/java.base/share/classes/java/io/PrintStream.java
|
|
||
| package java.lang; | ||
|
|
||
| import org.checkerframework.checker.confidential.qual.PolyConfidential; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find Exception subclasses with constructors taking String message that lack `@PolyConfidential`
rg -l "extends Exception" --type=java | xargs -I{} sh -c 'echo "=== {} ===" && rg -n "public.*\(.*String message" {} | head -5'Repository: typetools/jdk
Length of output: 36660
🏁 Script executed:
# Check Exception.java content
cat -n src/java.base/share/classes/java/lang/Exception.java | head -150Repository: typetools/jdk
Length of output: 6726
🏁 Script executed:
# Check IOException to see if it extends Exception and its constructor signatures
cat -n src/java.base/share/classes/java/io/IOException.java | head -100Repository: typetools/jdk
Length of output: 4428
🏁 Script executed:
# Check ReflectiveOperationException as well
cat -n src/java.base/share/classes/java/lang/ReflectiveOperationException.javaRepository: typetools/jdk
Length of output: 4257
LGTM! @PolyConfidential annotations properly added to Exception constructors.
The annotations enable confidentiality tracking through the exception hierarchy. The @PolyConfidential qualifier on message and cause parameters allows the checker to track whether exceptions contain confidential data.
However, IOException (which extends Exception) and other subclasses lack corresponding @PolyConfidential annotations on their constructors. When IOException calls super(message), it passes a plain String where Exception expects @PolyConfidential String, breaking the type contract. Update IOException, ReflectiveOperationException, and other Exception subclasses to include @PolyConfidential annotations and add "confidential" to their @AnnotatedFor declarations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/java.base/share/classes/java/lang/Exception.java` at line 28, Subclasses
of java.lang.Exception (notably IOException and ReflectiveOperationException)
must mirror the new `@PolyConfidential` constructor signatures and module
annotations: update each subclass's constructors that accept message/cause
(e.g., IOException(String message), IOException(String message, Throwable
cause), ReflectiveOperationException(...) and similar in other Exception
subclasses) to annotate the message and cause parameters with `@PolyConfidential`,
and add "confidential" to their `@AnnotatedFor` declarations so they satisfy the
Exception contract; ensure every super(...) call remains compatible with the
annotated types.
Comment on lines
274
to
+275
| @Pure | ||
| @Nullable V get(@GuardSatisfied Map<K, V> this, @UnknownSignedness @GuardSatisfied Object key); | ||
| @Nullable V get(@GuardSatisfied @PolyConfidential Map<K, V> this, @UnknownSignedness @GuardSatisfied Object key); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if Map implementations have `@PolyConfidential` on their get() receivers
# Expected: Find whether implementations need to be updated
# Search for get() method signatures in Map implementations
rg -n --type=java -B2 -A2 'public.*\bV\b\s+get\s*\(' | rg -A4 -B4 'HashMap|ConcurrentHashMap|AbstractMap|LinkedHashMap|TreeMap|EnumMap|IdentityHashMap|WeakHashMap'Repository: typetools/jdk
Length of output: 13632
Add @PolyConfidential annotation to the receiver of get() methods in all Map implementations.
The Map.get() interface declares @PolyConfidential on its receiver, but all implementing classes (HashMap, LinkedHashMap, AbstractMap, IdentityHashMap, WeakHashMap, TreeMap, EnumMap, ConcurrentHashMap) omit this annotation on their receivers. This inconsistency will cause type-checking failures since implementations do not satisfy the receiver type annotation contract established by the interface.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/java.base/share/classes/java/util/Map.java` around lines 274 - 275, The
implementations of Map.get() are missing the `@PolyConfidential` annotation on
their receiver which violates the interface contract declared by
Map.get(`@PolyConfidential` ... this) and breaks type checking. For each
implementing class (HashMap, LinkedHashMap, AbstractMap, IdentityHashMap,
WeakHashMap, TreeMap, EnumMap, ConcurrentHashMap) update the get(...) method
signature to include the `@PolyConfidential` annotation on the receiver (matching
Map.get's receiver annotation style, e.g., add `@PolyConfidential` to the this
parameter alongside existing `@GuardSatisfied/`@UnknownSignedness annotations) and
add any required imports so the method signatures match the interface contract.
|
|
||
| package java.util.logging; | ||
|
|
||
| import org.checkerframework.checker.confidential.qual.NonConfidential; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all Handler subclasses with publish methods that might need `@NonConfidential`
rg -n "public void publish\(LogRecord" --type=java -C2 | grep -v "@NonConfidential"Repository: typetools/jdk
Length of output: 23801
Handler subclasses must also include @NonConfidential annotation
The Handler.java changes are correct. The @NonConfidential annotation on Handler.publish(LogRecord record) properly enforces that log records must not contain confidential data.
However, the following Handler subclasses override publish() without this annotation and will trigger type-checker warnings:
- FileHandler.java:778
- SocketHandler.java:192
- StreamHandler.java:221
- MemoryHandler.java:181
- ConsoleHandler.java:94
- sun.rmi.runtime.Log.java:287
These subclass implementations must include @NonConfidential to match the parent signature.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/java.logging/share/classes/java/util/logging/Handler.java` at line 29,
Add the `@NonConfidential` annotation to each override of the publish(LogRecord
record) method in the listed Handler subclasses so their signatures match the
annotated parent: update FileHandler.publish(LogRecord record),
SocketHandler.publish(LogRecord record), StreamHandler.publish(LogRecord
record), MemoryHandler.publish(LogRecord record),
ConsoleHandler.publish(LogRecord record), and
sun.rmi.runtime.Log.publish(LogRecord record) to include `@NonConfidential` on the
method declaration.
|
|
||
| package java.util.logging; | ||
|
|
||
| import org.checkerframework.checker.confidential.qual.NonConfidential; |
There was a problem hiding this comment.
Remove duplicate import of @NonConfidential.
The import org.checkerframework.checker.confidential.qual.NonConfidential appears twice:
- Line 28
- Line 38
This is redundant and will cause a compiler warning.
🔧 Proposed fix
import org.checkerframework.checker.confidential.qual.NonConfidential;
import org.checkerframework.checker.interning.qual.Interned;
import org.checkerframework.checker.interning.qual.UsesObjectEquals;
import org.checkerframework.checker.lock.qual.GuardSatisfied;
import org.checkerframework.checker.nullness.qual.Nullable;
import org.checkerframework.checker.signature.qual.BinaryName;
import org.checkerframework.dataflow.qual.Pure;
import org.checkerframework.dataflow.qual.SideEffectFree;
import org.checkerframework.framework.qual.AnnotatedFor;
import org.checkerframework.framework.qual.CFComment;
-import org.checkerframework.checker.confidential.qual.NonConfidential;Also applies to: 38-38
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/java.logging/share/classes/java/util/logging/Logger.java` at line 28,
Remove the duplicate import of
org.checkerframework.checker.confidential.qual.NonConfidential in Logger.java:
locate the import declarations at the top of the file (related to class Logger)
and delete the redundant second occurrence so only a single import of
`@NonConfidential` remains.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/java.base/share/classes/java/lang/String.java (1)
4586-4653:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPropagate
@PolyConfidentialconsistently acrossStringformatting entry points.
String.format(String, Object...)is partially qualified (@PolyConfidentialreturn + varargs element type) but theformatparameter is unannotated (line 4586), whileString.format(Locale, String, Object...)andString.formatted(Object...)are entirely unannotated (lines 4630, 4652), creating confidentiality-drop paths.Suggested fix
- public static `@PolyConfidential` String format(String format, `@PolyConfidential` `@GuardSatisfied` `@Nullable` Object `@GuardSatisfied` ... args) { + public static `@PolyConfidential` String format( + `@PolyConfidential` String format, + `@PolyConfidential` `@GuardSatisfied` `@Nullable` Object `@GuardSatisfied` ... args) { return new Formatter().format(format, args).toString(); } - public static String format(`@GuardSatisfied` `@Nullable` Locale l, String format, `@GuardSatisfied` `@Nullable` Object `@GuardSatisfied` ... args) { + public static `@PolyConfidential` String format( + `@GuardSatisfied` `@Nullable` Locale l, + `@PolyConfidential` String format, + `@PolyConfidential` `@GuardSatisfied` `@Nullable` Object `@GuardSatisfied` ... args) { return new Formatter(l).format(format, args).toString(); } - public String formatted(Object... args) { + public `@PolyConfidential` String formatted( + `@PolyConfidential` Object... args) { return new Formatter().format(this, args).toString(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/java.base/share/classes/java/lang/String.java` around lines 4586 - 4653, The three formatting entry points create confidentiality-drop paths; update their signatures to consistently propagate `@PolyConfidential`: annotate the return type, the format String parameter, and the varargs element type with `@PolyConfidential`. Concretely, change String.format(String, Object...) to keep the `@PolyConfidential` on the return and add `@PolyConfidential` to the format parameter and the Object varargs element; update String.format(Locale, String, Object...) to annotate its return, the format parameter, and the varargs element with `@PolyConfidential`; and update String.formatted(Object...) to annotate its return and the Object varargs element with `@PolyConfidential` so all three methods (format(String,...), format(Locale, String,...), and formatted(...)) consistently propagate confidentiality.src/java.base/share/classes/java/lang/Throwable.java (2)
671-687:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove
@SideEffectFreefromThrowable#printStackTraceoverloads (observable I/O + locking)
Throwable’sprintStackTraceoverloads are annotated@SideEffectFreebut they produce observable output (vias.println(...)inlockedPrintStackTrace) and acquire locks (Object lock = s.lock()+locker.lock()/synchronized (lock)). This applies toprintStackTrace()/printStackTrace(PrintStream)/ thePrintStreamOrWriterhelper andprintStackTrace(PrintWriter).Suggested fix
- `@SideEffectFree` public void printStackTrace() { printStackTrace(System.err); } - `@SideEffectFree` public void printStackTrace(PrintStream s) { printStackTrace(new WrappedPrintStream(s)); } - `@SideEffectFree` private void printStackTrace(PrintStreamOrWriter s) { Object lock = s.lock(); if (lock instanceof InternalLock locker) { locker.lock(); try { lockedPrintStackTrace(s); } finally { locker.unlock(); } } else synchronized (lock) { lockedPrintStackTrace(s); } } - `@SideEffectFree` public void printStackTrace(PrintWriter s) { printStackTrace(new WrappedPrintWriter(s)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/java.base/share/classes/java/lang/Throwable.java` around lines 671 - 687, Remove the incorrect `@SideEffectFree` annotations from Throwable#printStackTrace(), Throwable#printStackTrace(PrintStream), Throwable#printStackTrace(PrintWriter) and the private helper printStackTrace(PrintStreamOrWriter) because these methods perform observable I/O and locking; locate the annotations on the methods named printStackTrace (no matter the signature) and delete the `@SideEffectFree` marker lines so the methods no longer claim to be side-effect free.
400-402:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix broken confidentiality polymorphism in
Throwable.getMessage()
Throwable.getMessage(@PolyConfidential... this)returns@Nullable@NonConfidentialString, which blocks confidentiality flow from the polymorphic receiver and contradicts the adjacent TODO that the type should be@PolyConfidential.Suggested fix
- // TODO: The type should be `@Poly`. Change it later. - public `@Nullable` `@NonConfidential` String getMessage(`@PolyConfidential` `@GuardSatisfied` Throwable this) { + public `@Nullable` `@PolyConfidential` String getMessage(`@PolyConfidential` `@GuardSatisfied` Throwable this) { return detailMessage; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/java.base/share/classes/java/lang/Throwable.java` around lines 400 - 402, Throwable.getMessage currently declares the return type as `@Nullable` `@NonConfidential` String while the receiver is annotated `@PolyConfidential`, preventing confidentiality polymorphism and contradicting the TODO; change the method signature so the return type is polymorphic with the receiver (e.g., `@PolyConfidential` `@Nullable` String) so the confidentiality flows from the `@PolyConfidential` receiver through getMessage; ensure the implementation still returns detailMessage and update any import/annotation usage if needed to use the `@PolyConfidential/`@Poly annotations consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/java.base/share/classes/java/lang/String.java`:
- Around line 4586-4653: The three formatting entry points create
confidentiality-drop paths; update their signatures to consistently propagate
`@PolyConfidential`: annotate the return type, the format String parameter, and
the varargs element type with `@PolyConfidential`. Concretely, change
String.format(String, Object...) to keep the `@PolyConfidential` on the return and
add `@PolyConfidential` to the format parameter and the Object varargs element;
update String.format(Locale, String, Object...) to annotate its return, the
format parameter, and the varargs element with `@PolyConfidential`; and update
String.formatted(Object...) to annotate its return and the Object varargs
element with `@PolyConfidential` so all three methods (format(String,...),
format(Locale, String,...), and formatted(...)) consistently propagate
confidentiality.
In `@src/java.base/share/classes/java/lang/Throwable.java`:
- Around line 671-687: Remove the incorrect `@SideEffectFree` annotations from
Throwable#printStackTrace(), Throwable#printStackTrace(PrintStream),
Throwable#printStackTrace(PrintWriter) and the private helper
printStackTrace(PrintStreamOrWriter) because these methods perform observable
I/O and locking; locate the annotations on the methods named printStackTrace (no
matter the signature) and delete the `@SideEffectFree` marker lines so the methods
no longer claim to be side-effect free.
- Around line 400-402: Throwable.getMessage currently declares the return type
as `@Nullable` `@NonConfidential` String while the receiver is annotated
`@PolyConfidential`, preventing confidentiality polymorphism and contradicting the
TODO; change the method signature so the return type is polymorphic with the
receiver (e.g., `@PolyConfidential` `@Nullable` String) so the confidentiality flows
from the `@PolyConfidential` receiver through getMessage; ensure the
implementation still returns detailMessage and update any import/annotation
usage if needed to use the `@PolyConfidential/`@Poly annotations consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 75410cfb-d31d-465b-9481-c6d798c629c1
📒 Files selected for processing (4)
src/java.base/share/classes/java/lang/String.javasrc/java.base/share/classes/java/lang/Throwable.javasrc/java.base/share/classes/java/util/Map.javasrc/java.base/share/classes/java/util/stream/Stream.java
💤 Files with no reviewable changes (1)
- src/java.base/share/classes/java/util/stream/Stream.java
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
2 participants
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.
Associated with checker framework pull request #6981