RFR: 8313452: Improve Classfile API attributes handling safety [v3]

Brian Goetz briangoetz at openjdk.org
Thu Sep 14 16:37:45 UTC 2023


On Wed, 6 Sep 2023 15:03:24 GMT, Adam Sotona <asotona at openjdk.org> wrote:

>> This PR improved Classfile API attributes handling safety by introduction of attribute stability indicator
>> and by extension of UnknownAttributesOption to more general AttributesProcessingOption.
>
> Adam Sotona has updated the pull request incrementally with one additional commit since the last revision:
> 
>   magic moved to Util::isAttributeAllowed and fixed possible NPE

Marked as reviewed by briangoetz (Reviewer).

src/java.base/share/classes/jdk/internal/classfile/AttributeMapper.java line 117:

> 115:      * {@return attribute stability indicator}
> 116:      */
> 117:     AttributeStability attributeStability();

Perhaps the name `stability()` would be better here because all the methods here are about the attribute being described.

src/java.base/share/classes/jdk/internal/classfile/impl/AbstractDirectBuilder.java line 58:

> 56:             attributes.withAttribute(a);
> 57:         }
> 58:     }

The "is allowed" check should always say yes for unbound attributes, and only apply the option for bound attributes.  (You may already have done that, but I wanted to make sure this was captured in the review.)

src/java.base/share/classes/jdk/internal/classfile/impl/Util.java line 64:

> 62:                 ? ATTRIBUTE_STABILITY_COUNT - attr.attributeMapper().attributeStability().ordinal() > processingOption.ordinal()
> 63:                 : true;
> 64:     }

Ignore earlier comment, looks like you've got this :)

-------------

PR Review: https://git.openjdk.org/jdk/pull/15101#pullrequestreview-1627325194
PR Review Comment: https://git.openjdk.org/jdk/pull/15101#discussion_r1326230242
PR Review Comment: https://git.openjdk.org/jdk/pull/15101#discussion_r1326233656
PR Review Comment: https://git.openjdk.org/jdk/pull/15101#discussion_r1326235687


More information about the core-libs-dev mailing list