[GEODE-10520] Security : Eliminate DirectBuffer Access to sun.nio.ch Internal Package

[JinwooHwang]

Summary

This PR removes Apache Geode's dependency on the internal sun.nio.ch.DirectBuffer interface, eliminating the need for the --add-exports=java.base/sun.nio.ch=ALL-UNNAMED JVM flag. The refactoring uses only public Java APIs with a thread-safe IdentityHashMap-based tracker while maintaining all existing functionality for buffer slice tracking in the BufferPool.

Motivation

Problem

Apache Geode previously accessed the internal sun.nio.ch.DirectBuffer interface to track buffer slice-to-original mappings in BufferPool. This approach had several critical issues:

1. Security Risk: Required exporting internal NIO channel implementation to all unnamed modules
2. Module System Violation: Bypassed Java Platform Module System (JPMS) encapsulation
3. Maintenance Burden: Depended on non-public APIs that could change without notice
4. Operational Complexity: Required JVM export flags on all Geode server instances
5. Compliance Issues: Triggered security audit flags in enterprise environments

Impact

• One less JVM flag to configure (SUN_NIO_CH_EXPORT)
• Improved security posture by not exposing internal I/O implementation
• Better forward compatibility with future Java versions
• Simpler deployment and operations
• Reduced technical debt

Changes

New Component: BufferAttachmentTracker

Created BufferAttachmentTracker class in geode-core to replace internal API usage:

Key Features:

• Uses Collections.synchronizedMap(new IdentityHashMap<>()) for slice-to-original mapping
• Thread-safe operations using synchronized wrapper
• Uses object identity (==) instead of equals() to avoid ByteBuffer.equals() issues
• Explicit cleanup via removeTracking() to prevent memory leaks
• Simple public API: recordSlice(), getOriginal(), and removeTracking()

Implementation Details:

class BufferAttachmentTracker {
  @SuppressWarnings("PMD.StaticFieldsMustBeImmutable")
  private static final Map<ByteBuffer, ByteBuffer> sliceToOriginal =
      Collections.synchronizedMap(new IdentityHashMap<>());

  static void recordSlice(ByteBuffer slice, ByteBuffer original) {
    sliceToOriginal.put(slice, original);
  }

  static ByteBuffer getOriginal(ByteBuffer buffer) {
    ByteBuffer original = sliceToOriginal.get(buffer);
    return original != null ? original : buffer;
  }

  static void removeTracking(ByteBuffer buffer) {
    sliceToOriginal.remove(buffer);
  }
}

Why IdentityHashMap:

• Object Identity: Uses reference equality (==) instead of content-based equals()
• Prevents IndexOutOfBoundsException: ByteBuffer.equals() compares buffer content based on position/limit, which can throw exceptions when buffer state changes after being used as a map key
• Safe for Mutable Keys: IdentityHashMap doesn't call equals() or hashCode(), making it safe for ByteBuffer keys
• Thread-Safe: Collections.synchronizedMap provides thread-safe access
• Explicit Cleanup: Provides better control over memory management

PMD Suppression Justification:

The mutable static field is intentionally designed for global buffer tracking and properly justified:

1. Mutable shared state is required to track buffer relationships across all threads
2. IdentityHashMap uses object identity (==) avoiding equals()/hashCode() issues
3. Collections.synchronizedMap provides thread-safe operations
4. This is the most efficient design for this use case

Modified: BufferPool.java

Updated buffer slice creation and retrieval:

Before (using internal API):

import org.apache.geode.unsafe.internal.sun.nio.ch.DirectBuffer;

ByteBuffer getPoolableBuffer(ByteBuffer buffer) {
  if (buffer instanceof DirectBuffer) {
    ByteBuffer attachment = ((DirectBuffer) buffer).attachment();
    if (attachment != null) {
      return attachment;
    }
  }
  return buffer;
}

After (using public API):

ByteBuffer acquireDirectBuffer(int size, boolean send) {
  // ... buffer allocation code ...
  if (result.capacity() > size) {
    ByteBuffer original = result;
    result.position(0).limit(size);
    result = result.slice();
    // Track the slice-to-original mapping
    BufferAttachmentTracker.recordSlice(result, original);
  }
  return result;
}

@VisibleForTesting
ByteBuffer getPoolableBuffer(final ByteBuffer buffer) {
  return BufferAttachmentTracker.getOriginal(buffer);
}

void releaseBuffer(ByteBuffer bb, boolean send) {
  // ... release logic ...
  BufferAttachmentTracker.removeTracking(bb);
  // ... return to pool ...
}

Key Changes:

• Record slice mapping when buffer is sliced
• Simplified getPoolableBuffer() to use tracker
• Explicit cleanup in releaseBuffer() prevents memory leaks
• No dependency on internal APIs
• Maintains exact same functionality

Removed: DirectBuffer Wrapper

Deleted internal API wrapper from geode-unsafe module:

• geode-unsafe/src/main/java/.../DirectBuffer.java (36 lines)
• geode-unsafe/src/test/java/.../DirectBufferTest.java (53 lines)

These files are no longer needed as we don't access internal APIs.

Updated: MemberJvmOptions.java

Removed SUN_NIO_CH_EXPORT from required JVM options:

Before:

private static final String SUN_NIO_CH_EXPORT = 
  "--add-exports=java.base/sun.nio.ch=ALL-UNNAMED";

static final List<String> JAVA_11_OPTIONS = Arrays.asList(
    COM_SUN_JMX_REMOTE_SECURITY_EXPORT,
    SUN_NIO_CH_EXPORT,  // Now removed
    COM_SUN_MANAGEMENT_INTERNAL_OPEN,
    JAVA_LANG_OPEN,
    JAVA_NIO_OPEN);

After:

// SUN_NIO_CH_EXPORT removed - no longer needed

static final List<String> JAVA_11_OPTIONS = Arrays.asList(
    COM_SUN_JMX_REMOTE_SECURITY_EXPORT,
    COM_SUN_MANAGEMENT_INTERNAL_OPEN,
    JAVA_LANG_OPEN,
    JAVA_NIO_OPEN);

New Tests: BufferAttachmentTrackerTest.java

Comprehensive test coverage (130 lines) includes:

Test Cases:

• getOriginal_returnsOriginalBufferForSlice() - Basic slice tracking
• getOriginal_returnsBufferItselfWhenNotTracked() - Identity for non-tracked buffers
• removeTracking_removesSliceMapping() - Cleanup verification
• trackingMapSize_reflectsCurrentMappings() - Size tracking
• clearTracking_removesAllMappings() - Bulk cleanup
• recordSlice_canOverwriteExistingMapping() - Update behavior
• worksWithHeapBuffers() - Non-direct buffer support
• simpleThreadSafetyTest() - Basic concurrent access
• concurrentAccessToSameSlice_isThreadSafe() - 10 threads × 1,000 operations = 10,000 concurrent operations
• memoryCleanup_releasesBuffersCorrectly() - Memory cleanup behavior

Thread Safety Test:

@Test
public void concurrentAccessToSameSlice_isThreadSafe() throws InterruptedException {
  final int numThreads = 10;
  final int iterations = 1000;
  final ExecutorService executor = Executors.newFixedThreadPool(numThreads);
  final CountDownLatch startLatch = new CountDownLatch(1);
  final CountDownLatch doneLatch = new CountDownLatch(numThreads);
  final AtomicInteger errors = new AtomicInteger(0);

  ByteBuffer original = ByteBuffer.allocateDirect(1024);
  ByteBuffer slice = original.slice();

  for (int i = 0; i < numThreads; i++) {
    executor.submit(() -> {
      try {
        startLatch.await();
        for (int j = 0; j < iterations; j++) {
          BufferAttachmentTracker.recordSlice(slice, original);
          ByteBuffer retrieved = BufferAttachmentTracker.getOriginal(slice);
          if (retrieved != original) {
            errors.incrementAndGet();
          }
        }
      } finally {
        doneLatch.countDown();
      }
    });
  }

  startLatch.countDown();
  boolean completed = doneLatch.await(30, TimeUnit.SECONDS);
  executor.shutdown();

  assertThat(completed).isTrue();
  assertThat(errors.get()).isEqualTo(0);  // Zero errors under concurrent load
}

Testing

Existing Tests

• All BufferPool tests pass without modification
• No functional changes to buffer pooling behavior
• Existing integration tests validate end-to-end functionality

New Tests

• BufferAttachmentTrackerTest provides comprehensive coverage
• Thread safety validated: 10,000 concurrent operations with zero errors
• Tests validate correctness, memory cleanup, and edge cases

Validation

All tests pass successfully. PMD static analysis passes with justified suppression.

Performance Impact

Expected: Negligible to none

Analysis:

• IdentityHashMap lookup: O(1) average case with synchronized access
• Synchronization overhead minimal for typical usage patterns
• Memory overhead: One IdentityHashMap entry per slice (small fixed cost)
• Object identity comparison (==) is faster than content-based equals()

Comparison:

Operation	DirectBuffer (Internal API)	IdentityHashMap (Public API)
Lookup	Field access	IdentityHashMap lookup (synchronized)
Thread Safety	Not applicable	Synchronized map wrapper
Memory Cleanup	Manual	Explicit (removeTracking)
API Stability	Internal (unstable)	Public (stable)
Key Comparison	N/A	Object identity (==) - safe for mutable keys

Compatibility

Backward Compatibility

• Fully backward compatible - no API changes to BufferPool
• Existing code continues to work without modification
• No changes to buffer pooling behavior or semantics

Forward Compatibility

• Works on Java 17, 21, and future versions
• No dependency on JDK internals that could change
• Aligns with Java module system best practices

Security Improvements

1. Reduced Attack Surface: No longer exposes internal NIO channel implementation
2. Principle of Least Privilege: Uses only necessary public APIs
3. Module Encapsulation: Respects JPMS boundaries
4. Audit Compliance: Eliminates security audit flags for internal API usage

Related Work

This PR is part of a broader initiative to eliminate all internal API usage in Apache Geode:

• GEODE-10519: Eliminated UnsafeThreadLocal access to java.lang internals (completed)
• GEODE-10520: This PR - Eliminate DirectBuffer access to sun.nio.ch internals
• Future: AddressableMemoryManager access to java.nio internals
• Future: VMStats50 access to com.sun.management.internal internals

Goal: Make Apache Geode fully compliant with Java Platform Module System, eliminating all --add-opens and --add-exports requirements.

Documentation Updates

• Added comprehensive Javadoc to BufferAttachmentTracker explaining IdentityHashMap choice
• Detailed inline comments explaining why IdentityHashMap is safe for ByteBuffer keys
• Updated BufferPool.getPoolableBuffer() documentation
• PMD suppression includes 4-point justification for thread safety

Checklist

✅ Code follows Apache Geode coding standards
✅ All new code has comprehensive test coverage
✅ All existing tests pass without modification
✅ No breaking changes to public APIs
✅ Internal API usage eliminated
✅ Thread safety validated (10,000 concurrent operations)
✅ Memory management verified (explicit cleanup)
✅ Documentation added for new components with thread-safety justification
✅ JVM flag removed from MemberJvmOptions
✅ Backward compatibility maintained
✅ PMD static analysis passes

Files Changed

Added

• geode-core/src/main/java/org/apache/geode/internal/net/BufferAttachmentTracker.java (89 lines)

• New tracker class using synchronized IdentityHashMap for thread-safe slice-to-original mappings
  • geode-core/src/test/java/org/apache/geode/internal/net/BufferAttachmentTrackerTest.java (130 lines)
• Comprehensive test suite including thread-safety validation

Modified

• geode-core/src/main/java/org/apache/geode/internal/net/BufferPool.java (20 lines changed)

• Updated to use BufferAttachmentTracker instead of DirectBuffer
  • geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/MemberJvmOptions.java (7 lines deleted)
• Removed SUN_NIO_CH_EXPORT constant and usage

Deleted

• geode-unsafe/src/main/java/org/apache/geode/unsafe/internal/sun/nio/ch/DirectBuffer.java (36 lines)

• Internal API wrapper no longer needed
  • geode-unsafe/src/test/java/org/apache/geode/unsafe/internal/sun/nio/ch/DirectBufferTest.java (53 lines)
• Tests for removed wrapper

Summary: 6 files changed, 224 insertions(+), 111 deletions(-)

Review Focus Areas

1. Thread Safety: Review IdentityHashMap usage with synchronized wrapper and PMD suppression justification
2. Memory Management: Verify explicit cleanup in releaseBuffer() prevents memory leaks
3. Test Coverage: Validate thread-safety test with 10,000 concurrent operations
4. Performance: Confirm synchronized access has minimal overhead for typical usage
5. API Compatibility: Confirm no breaking changes to BufferPool API
6. ByteBuffer Key Safety: Review why IdentityHashMap is required instead of ConcurrentHashMap

Additional Notes

Implementation Evolution

Initial Approach: ConcurrentHashMap

Initially implemented with ConcurrentHashMap<ByteBuffer, ByteBuffer> for lock-free concurrent access. However, this caused IndexOutOfBoundsException during distributed tests (discovered in WANHostNameVerificationDistributedTest).

Root Cause:

ByteBuffer implements content-based equals() and hashCode() that depend on the buffer's current position, limit, and content. When a ByteBuffer is used as a HashMap key:

1. The buffer's hashCode is computed based on its current state
2. If the buffer's position/limit changes after insertion, the hashCode changes
3. HashMap.get() looks in the wrong bucket
4. HashMap.putVal() calls equals() for collision resolution
5. ByteBuffer.equals() tries to compare content but throws IndexOutOfBoundsException if position/limit are in invalid states

Stack Trace:

java.lang.IndexOutOfBoundsException
    at java.base/java.nio.Buffer.checkIndex(Buffer.java:743)
    at java.base/java.nio.DirectByteBuffer.get(DirectByteBuffer.java:339)
    at java.base/java.nio.BufferMismatch.mismatch(BufferMismatch.java:40)
    at java.base/java.nio.ByteBuffer.equals(ByteBuffer.java:1750)
    at java.base/java.util.concurrent.ConcurrentHashMap.putVal(ConcurrentHashMap.java:1039)
    at org.apache.geode.internal.net.BufferAttachmentTracker.recordSlice(BufferAttachmentTracker.java:61)

Solution: IdentityHashMap

Switched to Collections.synchronizedMap(new IdentityHashMap<>()) which:

• Uses object identity (==) instead of equals() for key comparison
• Never calls ByteBuffer.equals() or hashCode()
• Safe for mutable keys like ByteBuffer
• Provides thread-safe access via synchronized wrapper
• Correctly tracks buffer relationships based on object identity, not content

Why Not Other Approaches?

Alternative 1: Store attachment in custom field

• Requires modifying ByteBuffer (not possible - final class)
• Would need wrapper class (defeats purpose of removing wrappers)

Alternative 2: WeakHashMap with Collections.synchronizedMap

• Initially attempted but discovered WeakHashMap is NOT thread-safe even with external synchronization
• WeakHashMap can modify internal structure during reads when GC cleans up references
• Led to race conditions under concurrent load (discovered via testing)

Alternative 3: Keep DirectBuffer wrapper

• Rejected - doesn't solve the core problem
• Still requires JVM export flags
• Still depends on unstable internal APIs

Alternative 4: ConcurrentHashMap (attempted)

• Lock-free concurrent access is appealing
• FAILED: ByteBuffer.equals() throws IndexOutOfBoundsException when used as HashMap key
• Not safe for mutable keys whose state changes after insertion

Chosen: Synchronized IdentityHashMap

• Object identity (==) comparison - safe for mutable keys
• Never calls equals() or hashCode()
• Thread-safe via Collections.synchronizedMap
• Explicit cleanup provides clear lifecycle management
• Validated with 10,000 concurrent operations with zero errors

Thread Safety Validation

The implementation was rigorously tested for thread safety:

1. Initial implementation used ConcurrentHashMap
2. Discovered IndexOutOfBoundsException in distributed tests
3. Root cause: ByteBuffer.equals() fails when buffer state changes
4. Solution: Switched to IdentityHashMap with synchronized wrapper
5. Final validation: 10 threads × 1,000 operations = 10,000 concurrent operations with zero errors

Future Enhancements

Potential improvements for future consideration:

1. Monitor IdentityHashMap size for metrics/debugging
2. Add configurable cleanup thresholds if needed
3. Consider automatic cleanup strategies if manual cleanup is missed

However, current implementation with explicit cleanup is sufficient and preferred for production use.

References

• JIRA: GEODE-10520
• Related PR: GEODE-10519 (UnsafeThreadLocal refactoring)
• Documentation: Java Platform Module System
• IdentityHashMap: Java API Documentation

For all changes, please confirm:

• Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
• Has your PR been rebased against the latest commit within the target branch (typically develop)?
• Is your initial contribution a single, squashed commit?
• Does gradlew build run cleanly?
• Have you written or updated unit tests to verify your changes?
• If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?

[JinwooHwang]

Hi @sboorlagadda . All tests have passed. Please let me know if you have any questions. Thank you.

[JinwooHwang]

We are ready to merge. Please let me know if you have any concerns. Thank you very much for your support.

[sboorlagadda]

LGTM. Thanks for this refactoring and brining JPMS compliance.

[JinwooHwang]

Thank you @sboorlagadda! Really appreciate your support throughout this initiative. Excited to see Apache Geode achieve full JPMS compliance!