[COMPRESS-714] Internal IllegalArgumentException in ZipFile and TarFile creation is not caught

[garydgregory]

Fixes Jira https://issues.apache.org/jira/browse/COMPRESS-714

Before you push a pull request, review this list:

• Read the contribution guidelines for this project.
• Read the ASF Generative Tooling Guidance if you use Artificial Intelligence (AI).
• I used AI to create any part of, or all of, this pull request.
• Run a successful build using the default Maven goal with mvn; that's mvn on the command line by itself.
• Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied. This may not always be possible, but it is a best practice.
• Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
• Each commit in the pull request should have a meaningful subject line and body. Note that a maintainer may squash commits during the merge process.

[ppkarwasz]

I’m wondering whether extending the catch clause from IOException to IOException | IllegalArgumentException is the right boundary. If the intention is to shield users from any unexpected internal failures we should rather catch Exception.

However, I see unchecked exceptions here as symptoms of underlying bugs. Masking them in the constructor can improve user experience, but only hides the bugs, instead of solving them.

In the specific case of COMPRESS-714, the IllegalArgumentException comes from a bug in SeekableInMemoryByteChannel. According to the position() contract, providing a position larger than the channel size should not result in an exception, so the current implementation violates that contract:

commons-compress/src/main/java/org/apache/commons/compress/utils/SeekableInMemoryByteChannel.java

Lines 114 to 121 in b83da24

	public SeekableByteChannel position(final long newPosition) throws IOException {
	ensureOpen();
	if (newPosition < 0L || newPosition > Integer.MAX_VALUE) {
	throw new IllegalArgumentException(String.format("Position must be in range [0..%,d]: %,d", Integer.MAX_VALUE, newPosition));
	}
	position = (int) newPosition;
	return this;
	}

[garydgregory]

Hm good catch, here's a fix: #756 (now merged).

[ppkarwasz]

Honestly, I wouldn't delay the release of commons-compress version 1.29.0 more than it already has been to wait for a new version of commons-io.

Let's use what we have in 2.21.0:

        } catch (final Exception e) {
            final IOException ioe = e instanceof IOException ? (IOException) e : new ArchiveException(e);
            IOUtils.closeQuietly(channel, ioe::addSuppressed);
            throw ioe;
        }

When we do release Commons IO version 2.22.0, we'll update this call site and all the other call sites that might benefit from the new method. There is no chance to forget that, since closeQuietly becomes ambiguous in IO 2.22.0.

[ppkarwasz]

Same as above: I wouldn't rely on an unpublished IO method and use what is available now.

        } catch (final Exception e) {
            this.closed = true;
            final IOException ioe = e instanceof IOException ? (IOException) e : new ArchiveException(e);
            IOUtils.closeQuietly(archive, ioe::addSuppressed);
            throw ioe;
        }