ppkarwasz commented on code in PR #747:
URL: https://github.com/apache/commons-compress/pull/747#discussion_r2515490757
##########
src/test/java/org/apache/commons/compress/harmony/pack200/Compress626Test.java:
##########
@@ -37,15 +37,15 @@ class Compress626Test {
void test() throws Exception {
final CPUTF8 name = new CPUTF8("");
final CPUTF8 layout = new CPUTF8("[");
- assertDoesNotThrow(() -> new NewAttributeBands(1, null, null,
+ assertThrows(Pack200Exception.class, () -> new NewAttributeBands(1,
null, null,
Review Comment:
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](https://issues.apache.org/jira/browse/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.
##########
src/test/java/org/apache/commons/compress/harmony/pack200/Compress628Test.java:
##########
@@ -29,8 +29,8 @@ class Compress628Test {
void test() throws Exception {
final CPUTF8 name = new CPUTF8("");
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)));
Review Comment:
[COMPRESS-628](https://issues.apache.org/jira/browse/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`).
##########
src/test/java/org/apache/commons/compress/harmony/pack200/NewAttributeBandsTest.java:
##########
@@ -183,7 +183,7 @@ void testLayoutWithCalls() throws IOException {
final CPUTF8 name = new CPUTF8("TestAttribute");
// @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)]]()[]]"
Review Comment:
This test string originates from Harmony (commit
b1ecf0b3c8ab5f69f0c7afffe01f46438f39c887).
The embedded spaces are invalid per the [Pack200 attribute layout
specification](https://docs.oracle.com/en/java/javase/11/docs/specs/pack-spec.html#attribute-layout-definitions),
and the nested `[...]` callables are also non-conforming.
In the native implementation (see
[`parseLayout`](https://github.com/openjdk/jdk11u/blob/9c5f0dd4d7f5c28c57ae0b28f06bb4b90a496d5c/src/jdk.pack/share/native/common-unpack/unpack.cpp#L1806)),
an unknown symbol triggers a parse failure, whereas our previous
implementation incorrectly interpreted it as end-of-input.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
