8268788: Annotations with lambda expressions can still cause AnnotationFormatError

[fdesu]

Change #3294 helps to avoid AnnotaionFormatException in sun.reflect.annotation.AnnotationInvocationHandler.validateAnnotationMethods. While it fixes the case with e.g. Runnable that generates the synthetic method without parameters, validation still fails on synthetic methods that have parameters e.g. Function, BiFunction, etc.

This change removes the restriction on parameters count to be zero i.e. lambdas with parameters
would be skipped from validation.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Issue

• JDK-8268788: Annotations with lambda expressions can still cause AnnotationFormatError

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4642/head:pull/4642
$ git checkout pull/4642

Update a local copy of the PR:
$ git checkout pull/4642
$ git pull https://git.openjdk.java.net/jdk pull/4642/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4642

View PR using the GUI difftool:
$ git pr show -t 4642

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4642.diff

[bridgekeeper]

👋 Welcome back fdesu! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@fdesu The following label will be automatically applied to this pull request:

• core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 03: Full - Incremental (9cf2f0c1)
• 02: Full (1f5fd0a5)
• 01: Full - Incremental (fc042710)
• 00: Full (bb08498f)

[fdesu]

Hi, I would appreciate if anyone could take a look and let me know their opinion. Removing the parameters count == 0 condition from the sun.reflect.annotation.AnnotationInvocationHandler:507 fixes the problem, though I'm not entirely sure if it brings some risks with it.

[mlbridge]

Mailing list message from Joe Darcy on core-libs-dev:

Hello,

I should be able to look at this within the next week or two; thanks,

-Joe

On 7/6/2021 1:22 AM, Sergei Ustimenko wrote:

[mlbridge]

Mailing list message from Joe Darcy on core-libs-dev:

Hello,

I should be able to look at this within the next week or two; thanks,

-Joe

On 7/6/2021 1:22 AM, Sergei Ustimenko wrote:

[mlbridge]

Mailing list message from Sergei Ustimenko on core-libs-dev:

Hi Joe,

Thanks, I appreciate your feedback very much!

There was one more thing I wanted to discuss: there is something that
might be improved in the AnnotationInvocationHandler#validateAnnotationMethods
and I wanted to know if it makes sense to go ahead with the change.

There are `if` statements that set the variable called `valid` to false
in case validation fails. Later, depending on the value of the `valid`, an
instance of AnnotationFormatError being thrown. I was thinking if it makes sense
to simplify the validation loop (this is not the complete diff - just gives a
gist of what I mean):

@@ -493,10 +493,7 @@ class AnnotationInvocationHandler implements InvocationHandler, Serializable {
* Specification citations below are from JLS
* 9.6.1. Annotation Type Elements
*/
- boolean valid = true;
- Method currentMethod = null;
for(Method method : memberMethods) {
- currentMethod = method;
int modifiers = method.getModifiers();
// Skip over methods that may be a static initializer or
// similar construct. A static initializer may be used for
@@ -524,8 +521,7 @@ class AnnotationInvocationHandler implements InvocationHandler, Serializable {
method.isDefault() ||
method.getParameterCount() != 0 ||
method.getExceptionTypes().length != 0) {
- valid = false;
- break;
+ throw newAnnotationFormatErrorOf(method);
}

...
}
- if (valid)
- return;
- else
- throw new AnnotationFormatError("Malformed method on an annotation type: " +
- currentMethod.toString());
+ }
+
+ private static AnnotationFormatError newAnnotationFormatErrorOf(Method malformedMethod) {
+ return new AnnotationFormatError("Malformed method on an annotation type: " + malformedMethod);
}

What do you think? Does it make sense?

Thanks,
Sergei

??????? Original Message ???????

On Tuesday, July 6th, 2021 at 16:01, Joe Darcy <joe.darcy at oracle.com> wrote:

Hello,

I should be able to look at this within the next week or two; thanks,

-Joe

[mlbridge]

Mailing list message from Sergei Ustimenko on core-libs-dev:

Hi Joe,

Thanks, I appreciate your feedback very much!

There was one more thing I wanted to discuss: there is something that
might be improved in the AnnotationInvocationHandler#validateAnnotationMethods
and I wanted to know if it makes sense to go ahead with the change.

There are `if` statements that set the variable called `valid` to false
in case validation fails. Later, depending on the value of the `valid`, an
instance of AnnotationFormatError being thrown. I was thinking if it makes sense
to simplify the validation loop (this is not the complete diff - just gives a
gist of what I mean):

@@ -493,10 +493,7 @@ class AnnotationInvocationHandler implements InvocationHandler, Serializable {
* Specification citations below are from JLS
* 9.6.1. Annotation Type Elements
*/
- boolean valid = true;
- Method currentMethod = null;
for(Method method : memberMethods) {
- currentMethod = method;
int modifiers = method.getModifiers();
// Skip over methods that may be a static initializer or
// similar construct. A static initializer may be used for
@@ -524,8 +521,7 @@ class AnnotationInvocationHandler implements InvocationHandler, Serializable {
method.isDefault() ||
method.getParameterCount() != 0 ||
method.getExceptionTypes().length != 0) {
- valid = false;
- break;
+ throw newAnnotationFormatErrorOf(method);
}

...
}
- if (valid)
- return;
- else
- throw new AnnotationFormatError("Malformed method on an annotation type: " +
- currentMethod.toString());
+ }
+
+ private static AnnotationFormatError newAnnotationFormatErrorOf(Method malformedMethod) {
+ return new AnnotationFormatError("Malformed method on an annotation type: " + malformedMethod);
}

What do you think? Does it make sense?

Thanks,
Sergei

??????? Original Message ???????

On Tuesday, July 6th, 2021 at 16:01, Joe Darcy <joe.darcy at oracle.com> wrote:

Hello,

I should be able to look at this within the next week or two; thanks,

-Joe

[bridgekeeper]

@fdesu This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[fdesu]

Hi,
I've reworked code in few places a bit. As we skip synthetic methods in couple
of places in AnnotationInvocationHandler:

1. In equalsImpl while going method-by-method, comparing them
2. In validateAnnotationMethods

(and as these two above work together to store methods in the memberMethods field) I though it makes sense to skip adding synthetic methods to the memberMethods field
altogether.

What do you think, does it makes sense? Thanks for taking a look!

[jddarcy]

Hello,

Thanks for the refined PR. Doing another round of review remain on my to-do list.

-Joe

[bridgekeeper]

@fdesu This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[bridgekeeper]

@fdesu This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

[jddarcy]

/open

[openjdk]

@jddarcy Only the pull request author can set the pull request state to "open"

[fdesu]

/open

[openjdk]

@fdesu This pull request is now open

[bridgekeeper]

@fdesu This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[bridgekeeper]

@fdesu This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.