Resource leak: Socket not closed when enableTLS1_2 throws IllegalArgumentException in LegacyTLSSocketFactory.createSocket()

[CyberSecurity-NCC]

Summary

The method LegacyTLSSocketFactory.createSocket(Socket, String, int, boolean) obtains a new Socket from the delegate and passes it to enableTLS1_2() before returning it. If enableTLS1_2() throws an IllegalArgumentException – typically because the device does not support TLSv1.2 – the Socket is neither returned nor closed, causing a socket resource leak.

Impact

• File descriptor exhaustion: Each failed invocation leaks one java.net.Socket together with its underlying native file descriptors. If network requests happen frequently (e.g. in an app that retries or polls), the process can exhaust the file descriptor limit, leading to SocketException: Too many open files, degraded connectivity, random crashes, or even OutOfMemoryError.
• System strain: Leaked sockets and their associated native resources put unnecessary pressure on the garbage collector and finalizer, which can contribute to degraded application performance over time.

Code Analysis

File: RedReader/src/main/java/org/quantumbadger/redreader/http/LegacyTLSSocketFactory.java
Method: createSocket(final Socket s, final String host, final int port, final boolean autoClose) (around lines 49-55)

@Override
public Socket createSocket(
        final Socket s,
        final String host,
        final int port,
        final boolean autoClose) throws IOException {
    return enableTLS1_2(delegate.createSocket(s, host, port, autoClose));
}

The helper method enableTLS1_2 is defined as:

private Socket enableTLS1_2(final Socket s) {
    if (s instanceof SSLSocket) {
        ((SSLSocket)s).setEnabledProtocols(TLS_V1_2_ONLY);
    }
    return s;
}

If setEnabledProtocols(...) throws an IllegalArgumentException, the exception propagates out of createSocket. The Socket that was successfully created by the delegate is then left unreferenced and never closed. The same pattern exists in all other createSocket overloads.

Suggested Fix

Wrap the call to enableTLS1_2 in a try-catch block that closes the newly created socket before rethrowing the exception. The following snippet shows the fix for one overload; the same approach should be applied to every createSocket overload:

@Override
public Socket createSocket(
        final Socket s,
        final String host,
        final int port,
        final boolean autoClose) throws IOException {
    final Socket socket = delegate.createSocket(s, host, port, autoClose);
    try {
        return enableTLS1_2(socket);
    } catch (final RuntimeException e) {
        // enableTLS1_2 may throw IllegalArgumentException.
        // Ensure the socket is closed to prevent leakage.
        try {
            socket.close();
        } catch (final IOException ignored) {
            // best effort
        }
        throw e;
    }
}

Note: Catching IllegalArgumentException specifically is also acceptable, but catching the broader RuntimeException adds safety for any unforeseen runtime failures.

Additional Notes

There are some other resource leak issues:

Resource leak: Cursor not closed on exception path in CacheDbManager.getFilesToPrune()

Code Analysis

File: RedReader/src/main/java/org/quantumbadger/redreader/cache/CacheDbManager.java
Method: getFilesToPrune (around lines 265-378)

public synchronized ArrayList<Long> getFilesToPrune(
        final HashSet<Long> currentFiles,
        final HashMap<Integer, TimeDuration> maxAge,
        final TimeDuration defaultMaxAge) {

    final SQLiteDatabase db = this.getWritableDatabase();
    final TimestampUTC currentTime = TimestampUTC.now();

    final Cursor cursor = db.query(
            TABLE,
            new String[] {FIELD_ID, FIELD_TIMESTAMP, FIELD_TYPE},
            null, null, null, null, null);

    // ... while(cursor.moveToNext()) loop ...
    // ... other logic ...

    if(!entriesToDelete.isEmpty()) {
        // ... delete query ...
    }

    cursor.close();  // <-- not protected by try/finally

    return filesToDelete;
}

Other methods in the same class (e.g., selectById, select) correctly use try-with-resources to guarantee the Cursor is closed even on exceptions. This method deviates from that safe pattern, introducing a resource leak on exceptional paths.

Suggested Fix

Wrap the Cursor usage in a try-with-resources block and restructure the logic so that all data extracted from the cursor is collected before any subsequent database mutations (such as DELETE statements). This ensures the cursor is always closed and the extracted data remains available for later use.

public synchronized ArrayList<Long> getFilesToPrune(
        final HashSet<Long> currentFiles,
        final HashMap<Integer, TimeDuration> maxAge,
        final TimeDuration defaultMaxAge) {

    final SQLiteDatabase db = this.getWritableDatabase();
    final TimestampUTC currentTime = TimestampUTC.now();

    final HashSet<Long> currentEntries = new HashSet<>();
    final ArrayList<Long> entriesToDelete = new ArrayList<>();
    final ArrayList<Long> filesToDelete = new ArrayList<>(32);

    // Read all cursor data inside try-with-resources
    try (Cursor cursor = db.query(
            TABLE,
            new String[] {FIELD_ID, FIELD_TIMESTAMP, FIELD_TYPE},
            null, null, null, null, null)) {

        while (cursor.moveToNext()) {
            final long id = cursor.getLong(0);
            final TimestampUTC timestamp = TimestampUTC.fromUtcMs(cursor.getLong(1));
            final int type = cursor.getInt(2);

            final TimestampUTC pruneIfBeforeMs;
            if (maxAge.containsKey(type)) {
                pruneIfBeforeMs = currentTime.subtract(maxAge.get(type));
            } else {
                Log.e("RR DEBUG cache", "Using default age! Filetype " + type);
                pruneIfBeforeMs = currentTime.subtract(defaultMaxAge);
            }

            if (!currentFiles.contains(id)) {
                entriesToDelete.add(id);
            } else if (timestamp.isLessThan(pruneIfBeforeMs)) {
                entriesToDelete.add(id);
                filesToDelete.add(id);
            } else {
                currentEntries.add(id);
            }
        }
    } // Cursor auto‑closed here, even if an exception occurs

    for (final long id : currentFiles) {
        if (!currentEntries.contains(id)) {
            filesToDelete.add(id);
        }
    }

    if (!entriesToDelete.isEmpty()) {
        // ... existing DELETE logic ...
    }

    return filesToDelete;
}

With this structure, the Cursor is reliably closed by the try-with-resources block, eliminating the leak regardless of any exceptions thrown during iteration.

InputStream leak in CacheDownload.performDownload when request is cancelled after success callback

Code Analysis

File: RedReader/src/main/java/org/quantumbadger/redreader/cache/CacheDownload.java
Method: performDownload → anonymous HTTPBackend.Listener#onSuccess

When the HTTP response arrives and onSuccess is invoked, the code first tests
mCancelled. If the request has already been cancelled it logs a message and
returns immediately, without closing the InputStream. In the normal
(non‑cancelled) path the stream is properly closed in a finally block via
General.closeSafely(is), but the early return bypasses that protection.

@Override
public void onSuccess(
        final String mimetype,
        final Long bodyBytes,
        final InputStream is) {

    if (mCancelled) {
        Log.i(TAG, "Request cancelled at start of onSuccess()");
        return; // ⚠️ InputStream 'is' not closed
    }
    // ... download logic ...
    // ... in finally block: General.closeSafely(is);
}

Suggested Fix

Close the InputStream before leaving the cancelled branch. The simplest and
safest change mirrors the existing clean‑up pattern used later in the method:

 if (mCancelled) {
     Log.i(TAG, "Request cancelled at start of onSuccess()");
+    General.closeSafely(is);
     return;
 }

This ensures the descriptor is released immediately in all cases. An alternative
would be to wrap the entire callback body in a try‑finally block, but the patch
above is minimal and sufficient.

InputStream not closed in playGIFWithLegacyDecoder when GifDecoderThread is stopped or interrupted

Code Analysis

File: RedReader/src/main/java/org/quantumbadger/redreader/activities/ImageViewActivity.java
playGIFWithLegacyDecoder method:

// The GIF decoder thread will close this itself
@SuppressWarnings("PMD.CloseResource") final InputStream is;
try {
    is = streamFactory.create();
} catch(final IOException e) {
    // ...
    return;
}
gifThread = new GifDecoderThread(is, listener);
gifThread.start();

The stream is handed off to GifDecoderThread with the comment “The GIF decoder
thread will close this itself”. However, there is no language-level guarantee that
the thread actually closes the stream in all termination scenarios:

• When the activity is destroyed, onDestroy() invokes gifThread.stopPlaying().
• The thread may be interrupted or forced to stop without ever reaching a finally
  block that closes the stream.
• The developer can change GifDecoderThread in the future and inadvertently
  break the assumed contract, causing silent leaks.

Suggested Fix

Option A (preferred): Ensure GifDecoderThread always closes the stream

Modify GifDecoderThread.run() to close the input stream in a finally block,
regardless of how the decoding loop exits. Additionally, stopPlaying() should
trigger a clean shutdown (e.g., interrupt the thread and wait for it to finish)
so that the finally block is guaranteed to execute.

Option B: Defensive closure in ImageViewActivity

If the thread class cannot be changed, store the stream reference and close it
safely when the activity is destroyed and the thread is stopped. Be sure to
coordinate the closure with thread termination to avoid IOExceptions from
reading a closed stream.

private InputStream mGifInputStream;

private void playGIFWithLegacyDecoder(...) {
    // ...
    try {
        mGifInputStream = streamFactory.create();
    } catch (IOException e) { /* ... */ return; }
    gifThread = new GifDecoderThread(mGifInputStream, listener);
    gifThread.start();
}

@Override
public void onDestroy() {
    super.onDestroy();
    mIsDestroyed = true;
    if (gifThread != null) {
        gifThread.stopPlaying();
    }
    if (mGifInputStream != null) {
        try { mGifInputStream.close(); } catch (IOException ignored) {}
        mGifInputStream = null;
    }
    // ...
}

Context & Acknowledgement

This issue was identified during our academic research on Java resource management. We have manually reviewed this finding to ensure its validity.
Thank you for maintaining this open-source project! We hope this report helps.