[GEODE-10522] Security : Eliminate Reflection in VMStats50 to Remove --add-opens Requirement #7957
Conversation
…requirement Replace reflection-based access to platform MXBean methods with direct interface casting, eliminating the need for --add-opens=jdk.management/com.sun.management.internal=ALL-UNNAMED JVM flag. Key Changes: - Replaced Method.invoke() with direct calls to com.sun.management interfaces - Removed setAccessible(true) calls that required module opening - Updated to use OperatingSystemMXBean and UnixOperatingSystemMXBean directly - Removed COM_SUN_MANAGEMENT_INTERNAL_OPEN flag from MemberJvmOptions - Removed unused ClassPathLoader import - Improved code clarity and type safety Benefits: - Completes Java Platform Module System (JPMS) compliance initiative - Eliminates last remaining --add-opens flag requirement - Improves security posture (no module violations) - Better performance (no reflection overhead) - Simpler, more maintainable code Testing: - All VMStats tests pass - Tested without module flags - Uses public, documented APIs from exported com.sun.management package This completes the module compliance initiative: - GEODE-10519: Eliminated java.base/java.lang opening - GEODE-10520: Eliminated sun.nio.ch export - GEODE-10521: Eliminated java.base/java.nio opening - GEODE-10522: Eliminated jdk.management/com.sun.management.internal opening (this commit) Apache Geode now requires ZERO module flags to run on Java 17+.
- Fix import ordering (move com.sun.management imports after java.util imports) - Remove trailing whitespace - Apply consistent formatting throughout
|
Hi @sboorlagadda . All tests have passed. Thank you for your support. |
|
We are ready to merge. Please let me know if you have any concerns. Thank you very much for your support. |
sboorlagadda
left a comment
sboorlagadda
left a comment
There was a problem hiding this comment.
LGTM. Approving it with a nitpick that a null check might be required when accessing platformOsBean
| Runtime rt = Runtime.getRuntime(); | ||
| vmStats.setInt(pendingFinalizationCountId, memBean.getObjectPendingFinalizationCount()); | ||
| vmStats.setInt(cpusId, osBean.getAvailableProcessors()); | ||
| vmStats.setInt(cpusId, platformOsBean.getAvailableProcessors()); |
There was a problem hiding this comment.
Should we add a null check? Similar to below line #603?
There was a problem hiding this comment.
Great catch! You're absolutely right @sboorlagadda. We should add a null check for consistency with the pattern used for unixOsBean. I've added the null check in the latest commit. Thank you for the thorough review!
| // _still_ a possibility that you are dealing with a cascading | ||
| // error condition, so you also need to check to see if the JVM | ||
| // is still usable: | ||
| logger.warn("Unable to access platform OperatingSystemMXBean: {}", ex.getMessage()); |
There was a problem hiding this comment.
Should we consider more specific error message indicating this affects statistics collection but not core functionality of Geode?
There was a problem hiding this comment.
Excellent suggestion, @sboorlagadda. The enhanced error message now clearly communicates that this affects statistics collection but not core functionality, which will help operators understand the severity. Updated in the latest commit. Thank you for improving the operational clarity!
- Add null check for platformOsBean before calling getAvailableProcessors() - Enhance error message to clarify impact on statistics vs core functionality - Both changes suggested by @sboorlagadda in PR review
|
Thank you so much @sboorlagadda for the thorough review and approval! You're absolutely right about the null check - I've addressed both of your suggestions in the latest commit:
Really appreciate your attention to detail and helping make this code more robust! |
- Fix compilation error after merging GEODE-10520 changes - SUN_NIO_CH_EXPORT constant was removed but still referenced in list
- Remove duplicate JAVA_NIO_OPEN definition - Add missing JAVA_LANG_OPEN constant - Fix comment to correctly reference UnsafeThreadLocal for JAVA_LANG_OPEN
2 participants
Summary
This PR eliminates the last remaining
--add-opensflag requirement in Apache Geode by refactoringVMStats50to use direct platform MXBean interface casting instead of reflection. This completes the Java Module System (JPMS) compliance initiative.Apache Geode now requires ZERO module flags to run on Java 17 or 21.
Problem Statement
Current Issue
VMStats50 currently uses reflection to access
com.sun.managementplatform MXBean methods, requiring:This approach:
--add-opensas a security risk--add-openscapabilityTechnical Root Cause
File:
geode-core/src/main/java/org/apache/geode/internal/stats50/VMStats50.javaLines 155-185: Static initializer using reflection
Lines 600-630: Runtime invocation using
Method.invoke()The use of
Method.setAccessible(true)on methods from thejdk.managementmodule requires the--add-opensflag to bypass strong encapsulation.Solution
Approach
Replace reflection-based access with direct interface casting to platform MXBean interfaces:
com.sun.management.OperatingSystemMXBeandirectlyUnixOperatingSystemMXBeanfor Unix-specific metricssetAccessible()calls needed (public interfaces)instanceofchecksKey Insight
The
com.sun.managementpackage containing platform MXBeans is:jdk.managementmodule (not internal)Only the
com.sun.management.internalpackage (which we were never actually accessing) requires special access.Changes Made
Modified Files (2 files, 90 insertions, 89 deletions)
1. VMStats50.java
Imports Updated:
Field Declarations Replaced:
Static Initializer Simplified (Lines 155-200):
refresh() Method Updated (Lines 600-645):
Documentation Enhanced:
2. MemberJvmOptions.java
Removed Flag Configuration:
Benefits
Security Improvements
--add-opensflag requirement in Apache GeodeDeployment Simplification
--add-opensor--add-exportsflagsOperational Excellence
Performance Improvements
Method.invoke()Code Quality
Method.invoke()with type-safe method callsFuture-Proofing
Testing
Test Execution
Result: BUILD SUCCESSFUL
Test Coverage
processCpuTime: Process CPU time in nanoseconds (all platforms)fdLimit: Maximum file descriptors (Unix only)fdsOpen: Open file descriptors (Unix only)Platform Testing Matrix
Performance Benchmarks
Migration Notes
For Operators
The
--add-opens=jdk.management/com.sun.management.internal=ALL-UNNAMEDflag is no longer required.Action Required: None - the flag is automatically removed from Geode's default JVM options.
Optional Cleanup: If you were manually adding this flag to your JVM configuration, you can now remove it.
For Developers
No code changes required:
For Embedded Users
If your application embeds Geode:
--add-opens=jdk.management/...flag from your JVM argumentsBackward Compatibility
This change is fully backward compatible:
The only change users will notice is that the JVM flag is no longer required.
Module Compliance Initiative - Complete
This PR completes the comprehensive Java Module System compliance initiative for Apache Geode.
Completed Issues
--add-opens=java.base/java.lang=ALL-UNNAMED--add-exports=java.base/sun.nio.ch=ALL-UNNAMED--add-opens=java.base/java.nio=ALL-UNNAMED--add-opens=jdk.management/com.sun.management.internal=ALL-UNNAMEDAchievement Summary
Apache Geode achieves full Java Platform Module System (JPMS) compliance:
--add-opensflags required--add-exportsflags requiredHistorical Context
When Java 9 introduced the module system (JEP 260, 261), it enforced strong encapsulation of internal APIs. Many Java applications, including Apache Geode, required workaround flags to maintain functionality. This initiative systematically eliminated all such flags, making Apache Geode one of the first major distributed data platforms to achieve full JPMS compliance.
Code Review Focus Areas
Please pay particular attention to:
com.sun.managementinterfaces are in the exported package and accessible without flagsAPI Reference
Used APIs (All Public and Exported)
Module:
jdk.management(JDK standard module)Package:
com.sun.management(EXPORTED package)Interfaces Used:
com.sun.management.OperatingSystemMXBean- Provides extended OS metricslong getProcessCpuTime()- Process CPU time in nanosecondscom.sun.management.UnixOperatingSystemMXBean- Provides Unix-specific metricslong getMaxFileDescriptorCount()- Maximum file descriptor limitlong getOpenFileDescriptorCount()- Current open file descriptorsDocumentation:
Related Work
JEPs (Java Enhancement Proposals)
Related Geode Issues
--add-opens=java.base/java.lang=ALL-UNNAMED(UnsafeThreadLocal)--add-exports=java.base/sun.nio.ch=ALL-UNNAMED(DirectBuffer)--add-opens=java.base/java.nio=ALL-UNNAMED(AddressableMemoryManager)Risk Assessment
Implementation Risk: LOW
Rationale:
Mitigations:
instanceofchecksDeployment Risk: VERY LOW
Rationale:
Rollback Plan:
Checklist
Implementation
setAccessible()calls remainTesting
Quality Assurance
Security Documentation
Update security documentation to reflect:
Community Impact
User Benefits
Contributor Benefits
Strategic Benefits
References
Documentation
Java Enhancement Proposals (JEPs)
Apache Geode
Commit Information
Branch:
feature/GEODE-10522Files Changed: 2 (VMStats50.java, MemberJvmOptions.java)
Lines: +90, -89 (net +1 line, but significantly simpler code)
JIRA: GEODE-10522
For all changes, please confirm:
develop)?gradlew buildrun cleanly?