Pack200: enforce strict attribute layout parsing by ppkarwasz · Pull Request #747 · apache/commons-compress

ppkarwasz · 2025-11-11T19:11:29Z

The parser for the Pack200 attribute layout definitions micro-language was previously overly permissive, allowing invalid layouts and masking archive corruption.

This change implements strict parsing in accordance with the Java 11 version of the Pack200 specification and removes duplication of the micro-language parsing logic between pack200.NewAttributeBands and unpack200.NewAttributeBands.

The parser for the [Pack200 attribute layout definitions](https://docs.oracle.com/en/java/javase/11/docs/specs/pack-spec.html#attribute-layout-definitions) micro-language was previously overly permissive, allowing invalid layouts and masking archive corruption. This change implements strict parsing in accordance with the [Java 11 specification](https://docs.oracle.com/en/java/javase/11/docs/specs/pack-spec.html) and removes duplication of the micro-language parsing logic between `pack200.NewAttributeBands` and `unpack200.NewAttributeBands`.

ppkarwasz

Stricter validation introduced by the parser caused several tests to fail.

Rather than relaxing the parser to match potentially incorrect test expectations, each failing test was reviewed individually, and the specific issues are described below.

ppkarwasz · 2025-11-11T19:30:28Z

        // @formatter:off
        final CPUTF8 layout = new CPUTF8(
-          "[NH[(1)]][RSH NH[RUH(1)]][TB(66,67,73,83,90)[KIH](68)[KDH](70)[KFH](74)[KJH](99)[RSH](101)[RSH RUH](115)[RUH](91)[NH[(0)]](64)[RSH[RUH(0)]]()[]]"
+          "[NH[(1)]][RSHNH[RUH(1)]][TB(66,67,73,83,90)[KIH](68)[KDH](70)[KFH](74)[KJH](99)[RSH](101)[RSHRUH](115)[RUH](91)[NH[(0)]](64)[RSHNH[RUH(0)]]()[]]"


This test string originates from Harmony (commit b1ecf0b).

The embedded spaces are invalid per the Pack200 attribute layout specification, and the nested [...] callables are also non-conforming.

In the native implementation (see parseLayout), an unknown symbol triggers a parse failure, whereas our previous implementation incorrectly interpreted it as end-of-input.

ppkarwasz · 2025-11-11T19:39:41Z

        final CPUTF8 name = new CPUTF8("");
        final CPUTF8 layout = new CPUTF8("[");
-        assertDoesNotThrow(() -> new NewAttributeBands(1, null, null,
+        assertThrows(Pack200Exception.class, () -> new NewAttributeBands(1, null, null,


I looked into the cause of this failing test:

The native parseLayout implementation (see reference above) treats EOF as equivalent to a closing bracket ].

On the other hand, I wasn't able to use the JDK 11 Java implementation to parse this example: it failed even before reaching attribute layouts,

Given that COMPRESS-626 specifically aims to ensure that unmatched brackets do not lead to OOM conditions and that the behavior of the C and Java implementations differ on this one, I think that this change is acceptable.

ppkarwasz · 2025-11-11T19:42:01Z

        final CPUTF8 layout = new CPUTF8("Re\\T");
-        assertDoesNotThrow(() -> new NewAttributeBands(1, null, null,
-                new AttributeDefinitionBands.AttributeDefinition(35, AttributeDefinitionBands.CONTEXT_CLASS, name, layout)));
+        assertThrows(Pack200Exception.class, () -> new NewAttributeBands(1, null, null, new AttributeDefinitionBands.AttributeDefinition(35,
+                AttributeDefinitionBands.CONTEXT_CLASS, name, layout)));


COMPRESS-628 was also about an OOM, so the change in behavior should not be relevant.

The native C implementation should also fail on the first unrecognized letter (e).

Copilot

Pull Request Overview

This PR implements strict parsing for Pack200 attribute layout definitions according to the Java 11 specification. The parser now properly validates layout syntax and rejects invalid or corrupted archives that were previously accepted.

Key changes:

Introduced new shared parsing logic in AttributeLayoutParser and AttributeLayoutUtils classes to eliminate code duplication between pack200 and unpack200 implementations
Added validation for integral, reference, and other layout tags with proper error handling
Updated tests to expect exceptions for previously accepted invalid layouts

Reviewed Changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
`src/main/java/org/apache/commons/compress/harmony/internal/AttributeLayoutParser.java`	New parser implementation for attribute layout definitions with strict validation
`src/main/java/org/apache/commons/compress/harmony/internal/AttributeLayoutUtils.java`	Utility methods for validating layout tags and processing layout elements
`src/main/java/org/apache/commons/compress/harmony/internal/package-info.java`	Package documentation for new internal package
`src/main/java/org/apache/commons/compress/harmony/unpack200/NewAttributeBands.java`	Refactored to use new shared parser, removed duplicate parsing logic
`src/main/java/org/apache/commons/compress/harmony/pack200/NewAttributeBands.java`	Refactored to use new shared parser, removed duplicate parsing logic
`src/test/java/org/apache/commons/compress/harmony/unpack200/NewAttributeBandsTest.java`	Added tests for empty body validation and corrected layout strings
`src/test/java/org/apache/commons/compress/harmony/pack200/NewAttributeBandsTest.java`	Corrected layout strings to remove invalid whitespace
`src/test/java/org/apache/commons/compress/harmony/pack200/Compress628Test.java`	Updated to expect Pack200Exception for invalid layout
`src/test/java/org/apache/commons/compress/harmony/pack200/Compress626Test.java`	Updated to expect Pack200Exception for invalid layout
`src/test/java/org/apache/commons/compress/harmony/internal/AttributeLayoutUtilsTest.java`	New tests for utility validation methods
`src/test/java/org/apache/commons/compress/harmony/internal/AttributeLayoutParserTest.java`	New comprehensive parser tests for valid and invalid layouts
`src/main/java/org/apache/commons/compress/harmony/archive/internal/nls/package-info.java`	Enhanced package documentation
`pom.xml`	Updated OSGi and module configuration to exclude internal packages from exports
`src/changes/changes.xml`	Added changelog entry

Comments suppressed due to low confidence (2)

src/main/java/org/apache/commons/compress/harmony/internal/AttributeLayoutUtils.java:185

@param tag ">" does not match any actual parameter of method "readAttributeLayout()".

     * @param <ALE>> the type of AttributeLayoutElement.

src/main/java/org/apache/commons/compress/harmony/internal/AttributeLayoutUtils.java:213

@param tag ">" does not match any actual parameter of method "readBody()".

     * @param <ALE>> the type of AttributeLayoutElement.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

This fails on JDK up to 11 and is yet another Javadoc quirk.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

garydgregory · 2025-11-11T20:43:44Z

Ah, we're missing a continue-on-error: ${{ matrix.experimental }} in our workflow it seems.

ppkarwasz · 2025-11-11T21:37:41Z

Ah, we're missing a continue-on-error: ${{ matrix.experimental }} in our workflow it seems.

No, the macos-13 runners are just being phased off. See apache/commons-csv#591 on how to migrate to macos-latest.

The regular need to upgrade workflows to:

Fix problems with some runners,
Add and remove Java version to the matrix.

is the reason why I would prefer to have reusable workflows. The Maven build of different Commons projects is not so different.

garydgregory · 2025-11-11T21:50:53Z

Ah, right, I remember holding off on migrating Compress to macos-latest while the IO branch was still open to avoid possible conflicts. Fixed now.

…t-parser

garydgregory

Hi @ppkarwasz

Thank you for the PR. A few comments here and there. Note that Lang has an IntegerRange to use instead of Range<Integer>.

This change adds a limit to the nesting level of `layout_element`s (64). Since all standard attribute layouts have a nesting level lower than 3, the limit should be suitable for most purposes.

ppkarwasz · 2025-11-16T21:42:41Z

@garydgregory

I’ve applied your review suggestions and also made a small refactoring to eliminate a potential infinite recursion in the parser. The parser should now be ready for another review pass.

ppkarwasz commented Nov 11, 2025

View reviewed changes

ppkarwasz added 2 commits November 11, 2025 20:54

fix: PMD failure

88b7c45

fix: restore purpose of lastPIntegral

3e298f2

ppkarwasz requested review from Copilot and garydgregory November 11, 2025 20:01

Copilot started reviewing on behalf of ppkarwasz November 11, 2025 20:02 View session

Copilot finished reviewing on behalf of ppkarwasz November 11, 2025 20:04

Copilot AI reviewed Nov 11, 2025

View reviewed changes

ppkarwasz and others added 9 commits November 11, 2025 21:08

fix: remove type parameters from Javadoc

21b0bee

This fails on JDK up to 11 and is yet another Javadoc quirk.

fix: additional bracket in Javadoc

e260e60

fix: AttributeLayoutParser Javadoc

d02e61d

fix: formatting

f67301f

Apply suggestions from code review

45f0d8c

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

fix: Javadoc copy/paste

6aa4dad

fix: remove unused import

100199b

fix: additional Javadoc

306eb95

fix: checkAnyIntTag error message

fd2bbb8

Merge remote-tracking branch 'apache/master' into fix/attribute-layou…

152ae52

…t-parser

garydgregory requested changes Nov 12, 2025

View reviewed changes

ppkarwasz marked this pull request as draft November 13, 2025 09:45

ppkarwasz added 6 commits November 13, 2025 12:24

fix: remove difference AttributeLayoutElement/LayoutElement

6ccaf88

feat: add tests with deeply nested layouts

18d1aaa

fix: Add nesting level limit

071247e

This change adds a limit to the nesting level of `layout_element`s (64). Since all standard attribute layouts have a nesting level lower than 3, the limit should be suitable for most purposes.

fix: AttributeLayoutUtils Javadoc

06b83d7

fix: invalid test assertions

00df3f0

fix: checkstyle

a2aa70c

ppkarwasz added 2 commits November 16, 2025 22:19

Replace Range<Integer> with IntegerRange

1518a14

Improve internal package Javadoc

4ad704b

ppkarwasz marked this pull request as ready for review November 16, 2025 21:40

ppkarwasz requested a review from garydgregory November 16, 2025 21:42

garydgregory merged commit e78e02c into master Nov 17, 2025
19 checks passed

garydgregory deleted the fix/attribute-layout-parser branch November 17, 2025 11:50

Uh oh!

Conversation

ppkarwasz commented Nov 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ppkarwasz left a comment

Choose a reason for hiding this comment

Uh oh!

ppkarwasz Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

ppkarwasz Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

ppkarwasz Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

garydgregory commented Nov 11, 2025

Uh oh!

ppkarwasz commented Nov 11, 2025

Uh oh!

garydgregory commented Nov 11, 2025

Uh oh!

garydgregory left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ppkarwasz commented Nov 16, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ppkarwasz commented Nov 11, 2025 •

edited

Loading