Fix concurrency issue in IOUtils.skip

[ppkarwasz]

This patch addresses a concurrency problem in IOUtils.skip, as reported in COMPRESS-666 and COMPRESS-697.

Previously, IOUtils.skip relied on InputStream#read to skip bytes, using a buffer shared across all threads. Although IOUtils.skip itself does not consume the data read, certain InputStream implementations (e.g. ChecksumInputStream) may process that data internally.

In concurrent scenarios, this shared buffer could be overwritten by another thread between the read and the subsequent internal processing (such as checksum calculation), leading to incorrect behavior.

This change reverts commit c12eaff and restores the use of a per-thread buffer in IOUtils.skip, ensuring thread safety and correct behavior in concurrent environments.

[ppkarwasz]

@garydgregory,

Do you happen to recall the rationale behind commit c908984, which zeroes out the per-thread buffers before each use? I’m not seeing a clear reason for this extra step, since:

• the buffers are private,
• they’re only used by skip and copyLarge,
• and in both cases they appear to be properly initialized with data before being read.

[garydgregory]

Hello @ppkarwasz
Thank you for the PR. I'm OK with it but I want to note that we've gone back and forth on this one in the past. Do you think this addresses https://issues.apache.org/jira/browse/IO-802 ?

[ppkarwasz]

I believe this PR does address a key part of IO-802: namely, it prevents shared buffer mutation between skipFully() (or skip) and concurrent reads of decompressing streams, which was the root of the multithreading failure observed.

I’m not entirely sure how InflaterInputStream itself uses the buffer, but its subclasses like GZIPInputStream and ZipInputStream definitely update a CRC as they read data. If another thread were to perform a skip operation on a different InputStream instance while sharing the same buffer, it could corrupt the data being validated and cause CRC verification to fail.

However, I don’t think this PR completely closes every corner case. In particular:

1. Reentrancy / nested skip calls
   Some streams (for example from Commons Compress, such as GzipCompressorInputStream) may internally call IOUtils.skip(...) or skipFully(...) from within their own read(...) implementations.
   Even with a ThreadLocal that internal skip uses the same buffer as an external skip.

2. Thread-local / buffer ownership tracking
   To guard against those cases, we should adopt a pattern similar to Log4j’s: maintain a flag to detect whether the local buffer is in use.

I’m happy to adapt this PR to follow that pattern.

[garydgregory]

Hi @ppkarwasz

Do you happen to recall the rationale behind commit c908984, which zeroes out the per-thread buffers before each use?

There was a concern that in a multi-tenant server/application, when multiple applications are served (sequentially, or not) on reused threads, one app might be able to read data written by a different app, creating a security issue.

I see:

        Arrays.fill(SCRATCH_BYTE_BUFFER_WO, (byte) 0);
        Arrays.fill(SCRATCH_CHAR_BUFFER_WO, (char) 0);

and calls to fill0() have been removed.

I think we need to keep zeroing out data to be on the safe side, unless you see something I don't.

I’m happy to adapt this PR to follow that pattern.

Let's not make it more complex for now.

[ppkarwasz]

I see:

        Arrays.fill(SCRATCH_BYTE_BUFFER_WO, (byte) 0);
        Arrays.fill(SCRATCH_CHAR_BUFFER_WO, (char) 0);

and calls to fill0() have been removed.

These were only removed, because the buffers themselves have been removed.

I’m happy to adapt this PR to follow that pattern.

Let's not make it more complex for now.

Adding a reentrancy guard to the thread-local buffers is a simple and well-established pattern: we’ve been using it in Log4j for years. I implemented it in 0bf22e0 by:

• Moving the thread locals into a dedicated class, so they are only instantiated on first use (lazy initialization).
• Using only JDK-provided classes to avoid potential memory leaks in application server environments.

[Copilot]

Pull Request Overview

This PR fixes a concurrency issue in IOUtils.skip by implementing per-thread scratch buffers instead of shared global buffers. The change addresses race conditions where multiple threads could corrupt each other's data when using checksum-aware InputStreams.

Key changes:

• Introduced ScratchBufferHolder with per-thread buffer management and reentrant access support
• Replaced shared global buffers with thread-local buffers that require explicit acquisition and release
• Added comprehensive concurrent testing for various IOUtils methods

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
src/main/java/org/apache/commons/io/IOUtils.java	Implements new ScratchBufferHolder class and updates buffer usage throughout IOUtils methods
src/main/java/org/apache/commons/io/CopyUtils.java	Updates to use new buffer acquisition/release pattern
src/test/java/org/apache/commons/io/IOCaseTest.java	Updates existing tests to use new buffer API
src/test/java/org/apache/commons/io/IOUtilsConcurrentTest.java	Adds comprehensive concurrent testing for IOUtils methods
src/changes/changes.xml	Documents the concurrency fix

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[garydgregory]

Merged, ty @ppkarwasz