[
https://issues.apache.org/jira/browse/GEODE-10520?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Jinwoo Hwang updated GEODE-10520:
---------------------------------
Description:
h2. Summary
Apache Geode currently accesses the internal {{sun.nio.ch.DirectBuffer}}
interface to track buffer slice-to-original mappings in {{{}BufferPool{}}}.
This requires the JVM export flag
{{{}--add-exports=java.base/sun.nio.ch=ALL-UNNAMED{}}}, creating security
vulnerabilities, operational complexity, and maintenance risks. This access
violates Java Platform Module System (JPMS) encapsulation and depends on
non-public APIs that could change or be removed in future JDK releases.
h2. Problem Description
h3. Current Implementation Dependencies
The {{BufferPool}} class in
{{geode-core/src/main/java/org/apache/geode/internal/tcp/Connection.java}}
manages ByteBuffer pooling for network communication. When creating buffer
slices, it needs to track the relationship between slices and their original
buffers to support proper buffer pooling and reuse.
*Current approach:*
* Uses {{sun.nio.ch.DirectBuffer}} internal interface
* Calls {{DirectBuffer.attachment()}} method to retrieve the original buffer
from a slice
* Requires a wrapper class in {{geode-unsafe}} module to access this internal
API
* Necessitates JVM export flag:
{{--add-exports=java.base/sun.nio.ch=ALL-UNNAMED}}
*Code location:*
{{geode-core/src/main/java/org/apache/geode/internal/tcp/Connection.java}}
{code:java}
// Current implementation accessing internal API
import org.apache.geode.unsafe.internal.sun.nio.ch.DirectBuffer;
// In BufferPool class
DirectBuffer directBuffer = (DirectBuffer) buffer;
ByteBuffer attachment = (ByteBuffer) directBuffer.attachment();
{code}
h3. Required JVM Configuration
To enable this internal API access, Geode requires the following JVM export
flag (defined in {{{}MemberJvmOptions.java{}}}):
{code:java}
/**
* export needed by DirectBuffer wrapper
*/
private static final String SUN_NIO_CH_EXPORT =
"--add-exports=java.base/sun.nio.ch=ALL-UNNAMED";
{code}
This flag must be configured on all Geode server JVMs, adding to the
operational burden alongside other required flags for {{java.lang}} and
{{java.nio}} access.
h3. Wrapper Module Requirements
The {{geode-unsafe}} module contains a wrapper class to access the internal
DirectBuffer interface:
{{geode-unsafe/src/main/java/org/apache/geode/unsafe/internal/sun/nio/ch/DirectBuffer.java}}
This wrapper:
* Duplicates the internal interface definition
* Creates tight coupling to JDK internals
* Requires maintenance when JDK internal structures change
* Adds complexity to the build and dependency management
h2. Problems with Current Approach
h3. 1. Java Module System Violations
*Severity: High*
The current approach directly violates the Java Platform Module System (JPMS)
encapsulation:
* Accesses internal implementation details of {{sun.nio.ch}} package
* Bypasses the module system's access controls
* Requires explicit {{--add-exports}} directive to override module boundaries
* Opens entire {{sun.nio.ch}} package to all unnamed modules
{*}Risk{*}: Future Java versions may further restrict or remove the ability to
export internal packages, even with command-line flags. The {{sun.*}} packages
are explicitly documented as internal and subject to change without notice.
h3. 2. Maintenance Burden and Fragility
*Severity: High*
The implementation is brittle and creates ongoing maintenance costs:
* Internal interface definitions may change between JDK releases
* Method signatures in {{sun.nio.ch.DirectBuffer}} could be modified
* The {{attachment()}} mechanism could be removed or redesigned
* Wrapper class must be manually synchronized with JDK changes
* No compiler warnings when internal APIs change
{*}Historical precedent{*}: Oracle has removed or modified internal APIs in
past JDK releases, breaking code that depended on them. The {{sun.*}} namespace
carries no stability guarantees.
h3. 3. Security Concerns
*Severity: Medium-High*
Using {{{}--add-exports=java.base/sun.nio.ch=ALL-UNNAMED{}}}:
* Opens internal NIO channel implementation to all unnamed modules
* Grants broader access than necessary (only need buffer attachment tracking)
* Weakens Java's security model and module encapsulation
* May trigger security audit flags in enterprise environments
* Creates potential attack vectors by exposing internal implementation
{*}Security implications{*}:
* Internal {{sun.nio.ch}} classes handle low-level I/O operations
* Exposing these internals could enable manipulation of channel state
* Buffer management internals could be exploited if exposed
* Violates principle of least privilege
h3. 4. Operational Complexity
*Severity: Medium*
Current deployment requirements:
* JVM export flags must be correctly configured on all Geode servers
* Different flag configurations needed for different Java versions
* Documentation and deployment scripts must track required flags
* Missing flags cause runtime failures with cryptic error messages
* Troubleshooting is complicated by module system violations
>From {{{}MemberJvmOptions.java{}}}, the growing list of required flags:
{code:java}
static final List<String> JAVA_11_OPTIONS = Arrays.asList(
COM_SUN_JMX_REMOTE_SECURITY_EXPORT,
SUN_NIO_CH_EXPORT, // ← Required for DirectBuffer access
COM_SUN_MANAGEMENT_INTERNAL_OPEN,
JAVA_LANG_OPEN,
JAVA_NIO_OPEN);
{code}
Each additional flag increases:
* Configuration complexity
* Risk of misconfiguration
* Deployment documentation burden
* Operational support costs
h3. 5. Build and Dependency Complexity
*Severity: Low-Medium*
The {{geode-unsafe}} module approach:
* Adds extra module to build and maintain
* Creates circular dependency concerns
* Complicates IDE setup and development workflow
* Increases build time and complexity
* Requires special handling in build tools
h3. 6. Compliance and Audit Issues
*Severity: Medium*
Enterprise environments face challenges:
* Security audits flag usage of internal APIs
* Compliance reviews require justification for module violations
* Risk assessment must account for non-public API dependencies
* Change management processes are triggered by JDK upgrades
* Documentation burden for security exception approvals
h2. Business Impact
h3. Reduced Security Risk
Eliminating {{sun.nio.ch}} access:
* Removes need to export internal NIO channel implementation
* Reduces attack surface by not exposing low-level I/O internals
* Simplifies security audits and compliance reviews
* Follows principle of least privilege
* Eliminates security exception requirements
h3. Improved Operational Simplicity
Removing the JVM export flag requirement:
* Simplifies deployment procedures
* Reduces configuration errors in production
* Removes a potential point of failure during Java upgrades
* Improves compatibility with container orchestration platforms
* Reduces operational support burden
h3. Better Java Version Compatibility
A solution using only public APIs:
* Works seamlessly across all Java versions (17, 21+)
* Doesn't depend on JDK internal implementation details
* Reduces risk during major Java version upgrades
* Aligns with Java's "code to interfaces, not implementations" principle
* Protects against breaking changes in future JDKs
h3. Enhanced Long-Term Maintainability
Moving away from internal APIs:
* Eliminates the {{geode-unsafe}} wrapper module
* Reduces technical debt significantly
* Makes the codebase more sustainable
* Demonstrates modern Java best practices
* Simplifies onboarding for new contributors
h3. Reduced Build Complexity
Removing {{geode-unsafe}} module:
* Simplifies build configuration
* Reduces module interdependencies
* Improves IDE development experience
* Faster build times
* Cleaner dependency graph
h2. Scope
h3. Affected Components
*Source Files*
* {{geode-core/src/main/java/org/apache/geode/internal/tcp/Connection.java}} -
BufferPool class (primary)
*
{{geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/MemberJvmOptions.java}}
- JVM flags
*
{{geode-unsafe/src/main/java/org/apache/geode/unsafe/internal/sun/nio/ch/DirectBuffer.java}}
- Wrapper (to be removed)
*Test Files*
* {{geode-core/src/test/java/.../BufferPoolTest.java}} - Buffer pooling tests
* {{geode-unsafe/src/test/java/.../DirectBufferTest.java}} - Wrapper tests (to
be removed)
h3. Scope
* Analysis of buffer slice-to-original mapping requirements
* Evaluation of public API alternatives (WeakHashMap, custom tracking)
* Assessment of memory management implications
* Performance impact analysis
* Compatibility evaluation with existing BufferPool users
h2. Technical Constraints and Requirements
h3. Functional Requirements
* {*}Buffer tracking{*}: Must maintain ability to map buffer slices back to
their original buffers
* {*}Memory management{*}: Must support proper cleanup when buffers are no
longer needed
* {*}Thread safety{*}: Must work correctly in multi-threaded network I/O
scenarios
* {*}Performance{*}: Must not significantly impact network I/O performance
* {*}Backward compatibility{*}: Must maintain existing BufferPool API
h3. Non-Functional Requirements
* {*}Security{*}: Use only public Java APIs
* {*}Maintainability{*}: Solution should be simple and well-documented
* {*}Portability{*}: Work across all supported Java versions (11, 17, 21)
* {*}Memory efficiency{*}: No memory leaks from buffer tracking
* {*}Operational simplicity{*}: No special JVM flags required
h2. Related Work
h3. Similar Issues in Geode
This is part of a broader initiative to eliminate internal API usage:
* GEODE-10519: UnsafeThreadLocal accesses {{java.lang}} internals (requires
{{{}--add-opens{}}})
* AddressableMemoryManager: Accesses {{java.nio}} internals (requires
{{{}--add-opens{}}})
* Other {{sun.{*}{*}}} *and {{com.sun.}}* dependencies requiring module exports
h3. Module System Compliance Initiative
Goal: Make Apache Geode fully compliant with Java Platform Module System:
* Eliminate all {{-{-}add-opens{-}}} and {{-add-exports}} requirements
* Use only public, stable Java APIs
* Improve security posture
* Simplify deployment and operations
* Future-proof against JDK changes
h2. Priority Assessment
*Recommended Priority: Medium-High*
h3. Factors Supporting High Priority
* Security vulnerability from exposing internal NIO implementation
* Maintenance risk from dependency on unstable internal APIs
* Operational complexity from JVM flag requirements
* Compliance issues in enterprise environments
* Technical debt that grows with each JDK release
h3. Factors Supporting Medium Priority
* Current implementation works (with export flags)
* Not causing immediate production failures
* Workaround available (configure JVM flags)
* Other similar issues also need addressing
* Requires careful testing of buffer management
h3. Risk Assessment
{*}If not addressed{*}:
* Risk of breakage in future JDK releases (High probability)
* Continued security audit concerns (Medium impact)
* Ongoing operational complexity (Medium impact)
* Accumulating technical debt (High impact over time)
h2. Success Criteria
* No {{--add-exports=java.base/sun.nio.ch=ALL-UNNAMED}} flag required
* {{geode-unsafe}} module eliminated (or DirectBuffer wrapper removed)
* All buffer pooling functionality maintained
* No performance regression in network I/O
* Works on Java 17, and 21 without special configuration
* Code uses only public Java APIs
* Solution is well-documented and maintainable
h2. Additional Context
h3. Why This Matters
# {*}Security{*}: Exposing {{sun.nio.ch}} internals weakens module boundaries
for critical I/O code
# {*}Maintainability{*}: Internal NIO APIs have changed in past JDK releases,
breaking dependent code
# {*}Compliance{*}: Enterprise security reviews flag internal API usage as
high-risk
# {*}Simplicity{*}: Removing JVM export requirements reduces operational burden
h3. Historical Context
The original {{BufferPool}} implementation was created before the Java module
system existed. When JPMS was introduced in Java 9:
* Internal APIs became encapsulated in modules
* {{sun.*}} packages required explicit export flags
* The {{attachment()}} pattern became a module violation
* A wrapper class was created to isolate the internal access
With modern Java (11+), this approach is no longer sustainable:
* Security concerns have increased
* Module system enforcement is stricter
* Future JDK versions may block internal access entirely
* Better public API solutions are available
h3. Technical Background
*ByteBuffer slice tracking:*
When {{ByteBuffer.slice()}} creates a view of a buffer:
* The slice shares the same backing memory
* The slice doesn't inherently know its "parent" buffer
* The internal {{DirectBuffer.attachment()}} was used to track this
relationship
* This relationship is needed for proper buffer pooling and reuse
*Why tracking is needed:*
* BufferPool manages a pool of large buffers
* Small slices are created for individual messages
* When a slice is released, the original buffer must be returned to the pool
* Proper tracking prevents memory leaks and enables buffer reuse
was:
h2. Summary
UnsafeThreadLocal currently uses reflection to access private internals of
{{java.lang.ThreadLocal}} and {{{}java.lang.ThreadLocalMap{}}}, requiring the
JVM flag {{{}--add-opens=java.base/java.lang=ALL-UNNAMED{}}}. This creates
maintenance burden, security concerns, and compatibility risks with future Java
versions.
h2. Description
h3. Current Implementation
The {{UnsafeThreadLocal}} class (located in
{{{}geode-core/src/main/java/org/apache/geode/distributed/internal/deadlock/UnsafeThreadLocal.java{}}})
extends {{ThreadLocal<T>}} and provides a {{get(Thread)}} method that allows
reading ThreadLocal values from arbitrary threads - a capability needed for
deadlock detection.
The current implementation uses reflection to:
# Access the private {{getMap(Thread)}} method of ThreadLocal
# Access the private {{getEntry(ThreadLocal)}} method of ThreadLocalMap
# Access the private {{value}} field of ThreadLocalMap.Entry
*Code excerpt showing reflection usage:*
{code:java}
private static Object get(ThreadLocal threadLocal, Thread thread) {
try {
Object threadLocalMap = invokePrivate(threadLocal, "getMap",
new Class[] {Thread.class}, new Object[] {thread});
if (threadLocalMap != null) {
Object entry = invokePrivate(threadLocalMap, "getEntry",
new Class[] {ThreadLocal.class}, new Object[] {threadLocal});
if (entry != null) {
return getPrivate(entry, "value");
}
}
return null;
} catch (Exception e) {
throw new RuntimeException("Unable to extract thread local", e);
}
}
{code}
h3. Required JVM Flags
To enable this reflection access, Geode requires the following JVM flag
(defined in {{{}MemberJvmOptions.java{}}}):
{code:java}
private static final String JAVA_LANG_OPEN =
"--add-opens=java.base/java.lang=ALL-UNNAMED";
{code}
This flag must be passed to every Geode member JVM, adding operational
complexity.
h3. Usage Context
UnsafeThreadLocal is used in three key areas for deadlock detection:
# *DLockService* ({{{}geode-core/.../locks/DLockService.java{}}})
** Tracks what distributed lock a thread is blocked on
** Used by: {{private final UnsafeThreadLocal<Object> blockedOn}}
# *MessageDependencyMonitor*
({{{}geode-core/.../deadlock/MessageDependencyMonitor.java{}}})
** Tracks ReplyProcessor21 instances threads are waiting for
** Tracks MessageWithReply instances being processed
** Used by: {{private final UnsafeThreadLocal<ReplyProcessor21>
waitingProcessors}}
** Used by: {{private final UnsafeThreadLocal<MessageWithReply>
processingMessages}}
# *DLockDependencyMonitor*
({{{}geode-core/.../deadlock/DLockDependencyMonitor.java{}}})
** Queries blockedOn status from DLockService for dependency graph construction
h2. Problems with Current Approach
h3. 1. Java Module System Violations
*Severity: High*
The reflection-based approach directly violates the Java Platform Module System
(JPMS) encapsulation:
* Accesses internal implementation details of {{java.lang}} package
* Bypasses the module system's access controls
* Requires explicit {{--add-opens}} directive to override module boundaries
{*}Risk{*}: Future Java versions may further restrict or remove the ability to
open internal packages, even with command-line flags.
h3. 2. Maintenance and Fragility
*Severity: High*
The implementation is brittle and depends on:
* Internal class names remaining unchanged (ThreadLocalMap, Entry)
* Internal method signatures staying stable (getMap, getEntry)
* Internal field names not changing (value)
* Class hierarchy not being refactored
Any changes to ThreadLocal internals in future JDK releases could break Geode
without warning, as these are non-public APIs with no stability guarantees.
h3. 3. Security Concerns
*Severity: Medium*
Using {{{}--add-opens=java.base/java.lang=ALL-UNNAMED{}}}:
* Opens the entire {{java.lang}} package to all unnamed modules
* Grants broader access than necessary (only need ThreadLocal access)
* Weakens Java's security model and encapsulation guarantees
* May trigger security audit flags in enterprise environments
h3. 4. Operational Complexity
*Severity: Medium*
Current deployment requirements:
* JVM flags must be correctly configured on all Geode servers
* Different flags needed for Java 8 vs. Java 11+ deployments
* Documentation and deployment scripts must maintain flag lists
* Missing flags cause runtime failures that are difficult to diagnose
>From {{{}MemberJvmOptions.java{}}}:
{code:java}
static final List<String> JAVA_11_OPTIONS = Arrays.asList(
COM_SUN_JMX_REMOTE_SECURITY_EXPORT,
SUN_NIO_CH_EXPORT,
COM_SUN_MANAGEMENT_INTERNAL_OPEN,
JAVA_LANG_OPEN, // ← Required only for UnsafeThreadLocal
JAVA_NIO_OPEN);
{code}
h3. 5. Performance Overhead
*Severity: Low*
Reflection calls have performance costs:
* Method lookup via reflection on each invocation
* Access permission checks
* Potential JIT optimization barriers
* Exception handling overhead
While not critical for deadlock detection (which is infrequent), it's still
unnecessary overhead.
h3. 6. Testing and CI/CD Complexity
*Severity: Low*
Test infrastructure must:
* Configure JVM flags correctly for all test executions
* Maintain separate flag lists for different Java versions
* Handle flag propagation in parallel test runs
* Document requirements for external contributors
h2. Business Impact
h3. Reduced Operational Risk
Eliminating the need for {{--add-opens}} flags:
* Simplifies deployment procedures
* Reduces configuration errors in production
* Removes a potential point of failure during Java upgrades
* Improves compatibility with container orchestration platforms
h3. Better Java Version Compatibility
A non-reflection implementation:
* Works seamlessly across all Java versions (8, 11, 17, 21+)
* Doesn't depend on JDK internal implementation details
* Reduces risk during major Java version upgrades
* Aligns with Java's "code to interfaces, not implementations" principle
h3. Enhanced Security Posture
Removing reflection-based access:
* Eliminates the need to weaken module system protections
* Reduces attack surface by not opening internal packages
* Simplifies security audits and compliance reviews
* Follows principle of least privilege
h3. Improved Developer Experience
A cleaner implementation:
* Easier to understand and maintain
* No special JVM configuration needed for development
* Simpler IDE setup and debugging
* Reduces onboarding friction for new contributors
h3. Long-Term Maintainability
Moving away from internal APIs:
* Protects against breaking changes in future JDK releases
* Reduces technical debt
* Makes the codebase more sustainable
* Demonstrates modern Java best practices
h2. Affected Components
h3. Files Modified in This Refactoring
*
{{geode-core/src/main/java/org/apache/geode/distributed/internal/deadlock/UnsafeThreadLocal.java}}
*
{{geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/MemberJvmOptions.java}}
h3. Files Using UnsafeThreadLocal (Not Modified)
These files use {{UnsafeThreadLocal}} but require no changes as the public API
remains unchanged:
*
{{geode-core/src/main/java/org/apache/geode/distributed/internal/locks/DLockService.java}}
- Uses {{UnsafeThreadLocal<Object> blockedOn}} for deadlock detection
*
{{geode-core/src/main/java/org/apache/geode/distributed/internal/deadlock/MessageDependencyMonitor.java}}
- Uses two UnsafeThreadLocal instances for tracking message dependencies
*
{{geode-core/src/main/java/org/apache/geode/distributed/internal/deadlock/DLockDependencyMonitor.java}}
- Queries UnsafeThreadLocal values for dependency graph construction
h3. Test Files (Executed, Not Modified)
*
{{geode-core/src/test/java/org/apache/geode/distributed/internal/deadlock/UnsafeThreadLocalJUnitTest.java}}
- All tests pass with new implementation
h2. Scope
* Refactoring UnsafeThreadLocal implementation to eliminate reflection
* Updating MemberJvmOptions to remove JAVA_LANG_OPEN flag
* Ensuring all existing tests pass with new implementation
* Maintaining exact same public API and behavior
* Supporting all currently supported Java versions (17, 21)
h2. Testing Requirements
h3. Functional Testing
* All existing UnsafeThreadLocal unit tests must pass
* Deadlock detection integration tests must pass
* DLock-related distributed tests must pass
* Message processing tests must pass
h3. Cross-Version Testing
* Verify functionality on Java 17, and 21
* Ensure no regression in deadlock detection accuracy
* Validate thread safety under concurrent access
h3. Performance Testing
* Verify no performance regression in deadlock detection
* Measure overhead reduction from removing reflection
* Test under high concurrency scenarios
h3. Negative Testing
* Verify proper cleanup when threads terminate
* Test behavior with many ThreadLocal instances
* Validate memory leak prevention
h2. Compatibility
h3. Backward Compatibility
* Public API of UnsafeThreadLocal must remain unchanged
* Behavior must be functionally equivalent to current implementation
* No changes to deadlock detection output or timing
h3. Forward Compatibility
* Implementation should work with future Java versions
* Should not depend on version-specific features
* Should gracefully handle JVM updates
h2. Priority Assessment
*Recommended Priority: Medium-High*
h3. Factors Supporting High Priority
* Eliminates security concerns with module system violations
* Improves long-term maintainability significantly
* Reduces operational complexity
* Removes technical debt that will only grow over time
h3. Factors Supporting Medium Priority
* Current implementation works (with JVM flags)
* Not causing immediate production issues
* Other reflection usage in Geode also needs addressing
* Requires careful testing of deadlock detection
h2. Acceptance Criteria
# UnsafeThreadLocal no longer uses reflection to access ThreadLocal internals
# No {{--add-opens=java.base/java.lang=ALL-UNNAMED}} flag required
# All existing tests pass without modification
# Deadlock detection functionality unchanged
# Works on Java 11, 17, and 21 without special configuration
# Performance is equal or better than current implementation
# No memory leaks when threads are terminated
# Code is well-documented explaining the new approach
h2. Related Issues
* Related to any other tickets addressing reflection usage in Geode
* May be part of larger "Java Module System Compliance" initiative
> Security: Eliminate DirectBuffer Access to sun.nio.ch Internal Package
> ----------------------------------------------------------------------
>
> Key: GEODE-10520
> URL: https://issues.apache.org/jira/browse/GEODE-10520
> Project: Geode
> Issue Type: Improvement
> Reporter: Jinwoo Hwang
> Assignee: Jinwoo Hwang
> Priority: Major
> Fix For: 2.1.0
>
>
> h2. Summary
> Apache Geode currently accesses the internal {{sun.nio.ch.DirectBuffer}}
> interface to track buffer slice-to-original mappings in {{{}BufferPool{}}}.
> This requires the JVM export flag
> {{{}--add-exports=java.base/sun.nio.ch=ALL-UNNAMED{}}}, creating security
> vulnerabilities, operational complexity, and maintenance risks. This access
> violates Java Platform Module System (JPMS) encapsulation and depends on
> non-public APIs that could change or be removed in future JDK releases.
> h2. Problem Description
> h3. Current Implementation Dependencies
> The {{BufferPool}} class in
> {{geode-core/src/main/java/org/apache/geode/internal/tcp/Connection.java}}
> manages ByteBuffer pooling for network communication. When creating buffer
> slices, it needs to track the relationship between slices and their original
> buffers to support proper buffer pooling and reuse.
> *Current approach:*
> * Uses {{sun.nio.ch.DirectBuffer}} internal interface
> * Calls {{DirectBuffer.attachment()}} method to retrieve the original buffer
> from a slice
> * Requires a wrapper class in {{geode-unsafe}} module to access this
> internal API
> * Necessitates JVM export flag:
> {{--add-exports=java.base/sun.nio.ch=ALL-UNNAMED}}
> *Code location:*
> {{geode-core/src/main/java/org/apache/geode/internal/tcp/Connection.java}}
> {code:java}
> // Current implementation accessing internal API
> import org.apache.geode.unsafe.internal.sun.nio.ch.DirectBuffer;
> // In BufferPool class
> DirectBuffer directBuffer = (DirectBuffer) buffer;
> ByteBuffer attachment = (ByteBuffer) directBuffer.attachment();
> {code}
> h3. Required JVM Configuration
> To enable this internal API access, Geode requires the following JVM export
> flag (defined in {{{}MemberJvmOptions.java{}}}):
> {code:java}
> /**
> * export needed by DirectBuffer wrapper
> */
> private static final String SUN_NIO_CH_EXPORT =
> "--add-exports=java.base/sun.nio.ch=ALL-UNNAMED";
> {code}
> This flag must be configured on all Geode server JVMs, adding to the
> operational burden alongside other required flags for {{java.lang}} and
> {{java.nio}} access.
> h3. Wrapper Module Requirements
> The {{geode-unsafe}} module contains a wrapper class to access the internal
> DirectBuffer interface:
> {{geode-unsafe/src/main/java/org/apache/geode/unsafe/internal/sun/nio/ch/DirectBuffer.java}}
> This wrapper:
> * Duplicates the internal interface definition
> * Creates tight coupling to JDK internals
> * Requires maintenance when JDK internal structures change
> * Adds complexity to the build and dependency management
> h2. Problems with Current Approach
> h3. 1. Java Module System Violations
> *Severity: High*
> The current approach directly violates the Java Platform Module System (JPMS)
> encapsulation:
> * Accesses internal implementation details of {{sun.nio.ch}} package
> * Bypasses the module system's access controls
> * Requires explicit {{--add-exports}} directive to override module boundaries
> * Opens entire {{sun.nio.ch}} package to all unnamed modules
> {*}Risk{*}: Future Java versions may further restrict or remove the ability
> to export internal packages, even with command-line flags. The {{sun.*}}
> packages are explicitly documented as internal and subject to change without
> notice.
> h3. 2. Maintenance Burden and Fragility
> *Severity: High*
> The implementation is brittle and creates ongoing maintenance costs:
> * Internal interface definitions may change between JDK releases
> * Method signatures in {{sun.nio.ch.DirectBuffer}} could be modified
> * The {{attachment()}} mechanism could be removed or redesigned
> * Wrapper class must be manually synchronized with JDK changes
> * No compiler warnings when internal APIs change
> {*}Historical precedent{*}: Oracle has removed or modified internal APIs in
> past JDK releases, breaking code that depended on them. The {{sun.*}}
> namespace carries no stability guarantees.
> h3. 3. Security Concerns
> *Severity: Medium-High*
> Using {{{}--add-exports=java.base/sun.nio.ch=ALL-UNNAMED{}}}:
> * Opens internal NIO channel implementation to all unnamed modules
> * Grants broader access than necessary (only need buffer attachment tracking)
> * Weakens Java's security model and module encapsulation
> * May trigger security audit flags in enterprise environments
> * Creates potential attack vectors by exposing internal implementation
> {*}Security implications{*}:
> * Internal {{sun.nio.ch}} classes handle low-level I/O operations
> * Exposing these internals could enable manipulation of channel state
> * Buffer management internals could be exploited if exposed
> * Violates principle of least privilege
> h3. 4. Operational Complexity
> *Severity: Medium*
> Current deployment requirements:
> * JVM export flags must be correctly configured on all Geode servers
> * Different flag configurations needed for different Java versions
> * Documentation and deployment scripts must track required flags
> * Missing flags cause runtime failures with cryptic error messages
> * Troubleshooting is complicated by module system violations
> From {{{}MemberJvmOptions.java{}}}, the growing list of required flags:
> {code:java}
> static final List<String> JAVA_11_OPTIONS = Arrays.asList(
> COM_SUN_JMX_REMOTE_SECURITY_EXPORT,
> SUN_NIO_CH_EXPORT, // ← Required for DirectBuffer access
> COM_SUN_MANAGEMENT_INTERNAL_OPEN,
> JAVA_LANG_OPEN,
> JAVA_NIO_OPEN);
> {code}
> Each additional flag increases:
> * Configuration complexity
> * Risk of misconfiguration
> * Deployment documentation burden
> * Operational support costs
> h3. 5. Build and Dependency Complexity
> *Severity: Low-Medium*
> The {{geode-unsafe}} module approach:
> * Adds extra module to build and maintain
> * Creates circular dependency concerns
> * Complicates IDE setup and development workflow
> * Increases build time and complexity
> * Requires special handling in build tools
> h3. 6. Compliance and Audit Issues
> *Severity: Medium*
> Enterprise environments face challenges:
> * Security audits flag usage of internal APIs
> * Compliance reviews require justification for module violations
> * Risk assessment must account for non-public API dependencies
> * Change management processes are triggered by JDK upgrades
> * Documentation burden for security exception approvals
> h2. Business Impact
> h3. Reduced Security Risk
> Eliminating {{sun.nio.ch}} access:
> * Removes need to export internal NIO channel implementation
> * Reduces attack surface by not exposing low-level I/O internals
> * Simplifies security audits and compliance reviews
> * Follows principle of least privilege
> * Eliminates security exception requirements
> h3. Improved Operational Simplicity
> Removing the JVM export flag requirement:
> * Simplifies deployment procedures
> * Reduces configuration errors in production
> * Removes a potential point of failure during Java upgrades
> * Improves compatibility with container orchestration platforms
> * Reduces operational support burden
> h3. Better Java Version Compatibility
> A solution using only public APIs:
> * Works seamlessly across all Java versions (17, 21+)
> * Doesn't depend on JDK internal implementation details
> * Reduces risk during major Java version upgrades
> * Aligns with Java's "code to interfaces, not implementations" principle
> * Protects against breaking changes in future JDKs
> h3. Enhanced Long-Term Maintainability
> Moving away from internal APIs:
> * Eliminates the {{geode-unsafe}} wrapper module
> * Reduces technical debt significantly
> * Makes the codebase more sustainable
> * Demonstrates modern Java best practices
> * Simplifies onboarding for new contributors
> h3. Reduced Build Complexity
> Removing {{geode-unsafe}} module:
> * Simplifies build configuration
> * Reduces module interdependencies
> * Improves IDE development experience
> * Faster build times
> * Cleaner dependency graph
> h2. Scope
> h3. Affected Components
> *Source Files*
> * {{geode-core/src/main/java/org/apache/geode/internal/tcp/Connection.java}}
> - BufferPool class (primary)
> *
> {{geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/MemberJvmOptions.java}}
> - JVM flags
> *
> {{geode-unsafe/src/main/java/org/apache/geode/unsafe/internal/sun/nio/ch/DirectBuffer.java}}
> - Wrapper (to be removed)
> *Test Files*
> * {{geode-core/src/test/java/.../BufferPoolTest.java}} - Buffer pooling tests
> * {{geode-unsafe/src/test/java/.../DirectBufferTest.java}} - Wrapper tests
> (to be removed)
> h3. Scope
> * Analysis of buffer slice-to-original mapping requirements
> * Evaluation of public API alternatives (WeakHashMap, custom tracking)
> * Assessment of memory management implications
> * Performance impact analysis
> * Compatibility evaluation with existing BufferPool users
> h2. Technical Constraints and Requirements
> h3. Functional Requirements
> * {*}Buffer tracking{*}: Must maintain ability to map buffer slices back to
> their original buffers
> * {*}Memory management{*}: Must support proper cleanup when buffers are no
> longer needed
> * {*}Thread safety{*}: Must work correctly in multi-threaded network I/O
> scenarios
> * {*}Performance{*}: Must not significantly impact network I/O performance
> * {*}Backward compatibility{*}: Must maintain existing BufferPool API
> h3. Non-Functional Requirements
> * {*}Security{*}: Use only public Java APIs
> * {*}Maintainability{*}: Solution should be simple and well-documented
> * {*}Portability{*}: Work across all supported Java versions (11, 17, 21)
> * {*}Memory efficiency{*}: No memory leaks from buffer tracking
> * {*}Operational simplicity{*}: No special JVM flags required
> h2. Related Work
> h3. Similar Issues in Geode
> This is part of a broader initiative to eliminate internal API usage:
> * GEODE-10519: UnsafeThreadLocal accesses {{java.lang}} internals (requires
> {{{}--add-opens{}}})
> * AddressableMemoryManager: Accesses {{java.nio}} internals (requires
> {{{}--add-opens{}}})
> * Other {{sun.{*}{*}}} *and {{com.sun.}}* dependencies requiring module
> exports
> h3. Module System Compliance Initiative
> Goal: Make Apache Geode fully compliant with Java Platform Module System:
> * Eliminate all {{-{-}add-opens{-}}} and {{-add-exports}} requirements
> * Use only public, stable Java APIs
> * Improve security posture
> * Simplify deployment and operations
> * Future-proof against JDK changes
> h2. Priority Assessment
> *Recommended Priority: Medium-High*
> h3. Factors Supporting High Priority
> * Security vulnerability from exposing internal NIO implementation
> * Maintenance risk from dependency on unstable internal APIs
> * Operational complexity from JVM flag requirements
> * Compliance issues in enterprise environments
> * Technical debt that grows with each JDK release
> h3. Factors Supporting Medium Priority
> * Current implementation works (with export flags)
> * Not causing immediate production failures
> * Workaround available (configure JVM flags)
> * Other similar issues also need addressing
> * Requires careful testing of buffer management
> h3. Risk Assessment
> {*}If not addressed{*}:
> * Risk of breakage in future JDK releases (High probability)
> * Continued security audit concerns (Medium impact)
> * Ongoing operational complexity (Medium impact)
> * Accumulating technical debt (High impact over time)
> h2. Success Criteria
> * No {{--add-exports=java.base/sun.nio.ch=ALL-UNNAMED}} flag required
> * {{geode-unsafe}} module eliminated (or DirectBuffer wrapper removed)
> * All buffer pooling functionality maintained
> * No performance regression in network I/O
> * Works on Java 17, and 21 without special configuration
> * Code uses only public Java APIs
> * Solution is well-documented and maintainable
> h2. Additional Context
> h3. Why This Matters
> # {*}Security{*}: Exposing {{sun.nio.ch}} internals weakens module
> boundaries for critical I/O code
> # {*}Maintainability{*}: Internal NIO APIs have changed in past JDK
> releases, breaking dependent code
> # {*}Compliance{*}: Enterprise security reviews flag internal API usage as
> high-risk
> # {*}Simplicity{*}: Removing JVM export requirements reduces operational
> burden
> h3. Historical Context
> The original {{BufferPool}} implementation was created before the Java module
> system existed. When JPMS was introduced in Java 9:
> * Internal APIs became encapsulated in modules
> * {{sun.*}} packages required explicit export flags
> * The {{attachment()}} pattern became a module violation
> * A wrapper class was created to isolate the internal access
> With modern Java (11+), this approach is no longer sustainable:
> * Security concerns have increased
> * Module system enforcement is stricter
> * Future JDK versions may block internal access entirely
> * Better public API solutions are available
> h3. Technical Background
> *ByteBuffer slice tracking:*
> When {{ByteBuffer.slice()}} creates a view of a buffer:
> * The slice shares the same backing memory
> * The slice doesn't inherently know its "parent" buffer
> * The internal {{DirectBuffer.attachment()}} was used to track this
> relationship
> * This relationship is needed for proper buffer pooling and reuse
> *Why tracking is needed:*
> * BufferPool manages a pool of large buffers
> * Small slices are created for individual messages
> * When a slice is released, the original buffer must be returned to the pool
> * Proper tracking prevents memory leaks and enables buffer reuse
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
