Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CGLIB does not allow packages to start with "java" #27622

Closed
MuellerMP opened this issue Oct 29, 2021 · 9 comments
Closed

CGLIB does not allow packages to start with "java" #27622

MuellerMP opened this issue Oct 29, 2021 · 9 comments
Assignees
Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement
Milestone

Comments

@MuellerMP
Copy link

MuellerMP commented Oct 29, 2021

net.sf.cglib.core.DefaultNamingPolicy, which is also used by the SpringNamingPolicy of the Spring Framework checks explicitly for a java prefix.

A problem that might occur here is that someone tries to build a javabeat.net Spring example. This would cause a crash during any CGLIB define class due to the prepended dollar sign ($).

Wouldn't it therefore make sense to check for java. as a prefix instead of java?

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Oct 29, 2021
@sbrannen
Copy link
Member

sbrannen commented Oct 29, 2021

A problem that might occur here is that e.g. someone tries to build a "javabeat.net" spring example.

Wouldn't the package name be net.javabeat, which starts with net instead of java?

This would cause a crash during any cglib define class due to the prepended dollar sign...

Do you have a concrete use case that results in such a crash?

Also, what version of the JDK/JRE are you using?

@sbrannen sbrannen added the status: waiting-for-feedback We need additional information before we can continue label Oct 29, 2021
@MuellerMP
Copy link
Author

I mean the example was just meant to be demonstrative...
But I actually ran across the issue while trying to run a spring-boot v2.5.6 (spring framework 5.3.12) sample app under orcale jdk 17.
Due to the fact that the fallback method of using ClassLoader::defineClass not being available without a "--add-opens java.base/java.lang=ALL-UNNAMED" it just results in a InaccessibleObjectException.

Full version info for my java binary: Java HotSpot(TM) 64-Bit Server VM (build 17+35-LTS-2724, mixed mode, sharing)

It does work under java 11.. but yea i find it kind of odd that cglib implies these restrictions and doesn't make it clear that it does in fact not support these usecases.
Oracles naming convention itself does not impose such restrictions:
https://docs.oracle.com/javase/tutorial/java/package/namingpkgs.html

Example.jar and sources:
https://drive.google.com/file/d/1Y9ejDOixpaPPP9yMUsIftg3xA0bkcNJv/view?usp=sharing

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Oct 29, 2021
@sbrannen
Copy link
Member

Thanks for the feedback.

It sounds like you're saying it's only a result of enforcement of strong encapsulation since JDK 16/17 -- right?

And in that case, there is in fact a workaround as you've mentioned with --add-opens ....

As for the reason why CGLIB prepends $ to any fully qualified class name starting with java instead of java. or javax., perhaps @vlsi can shed some light on that.

@vlsi
Copy link
Contributor

vlsi commented Oct 30, 2021

@MuellerMP ,

  1. I guess you are right, and it would be better to use java. and javax. prefix tests (see https://github.com/cglib/cglib/blame/975e481faf39c91b8ac5b9b3d62822b7c52c5f47/cglib/src/main/java/net/sf/cglib/core/DefaultNamingPolicy.java#L41-L42 )
  2. However, I guess, it does not matter much, as $ should be a valid character in the class name even at the very beginning.

Do you have a stack trace and/or reproducer at hand?

@MuellerMP
Copy link
Author

@sbrannen i think the fallback method using a reflective call to ClassLoader::defineClass is a separate design issue of the CGLIB...
Current state in spring and the problem in my case is the naming issue.
It could also confuse users to see a InaccessibleObjectException just because they named there package in an apparently wrong way. So atleast a helpful exception would be nice trat this usecase is in fact unsupported.

@vlsi Hey! Can you access the google drive link I posted? It should contain a jar and sources to the sample that reproduces the exception under java 16+.

@vlsi
Copy link
Contributor

vlsi commented Oct 30, 2021

@MuellerMP , I've never worked with maven-based spring boot project, and it would take me ages to start. Could you please share the stack trace related to $-based class name?

@MuellerMP
Copy link
Author

MuellerMP commented Oct 30, 2021

this is the complete log of the program runthrough (including exception in the reflectutils).
example-run.txt

Original Exception is caught here:
https://github.com/spring-projects/spring-framework/blob/v5.3.12/spring-core/src/main/java/org/springframework/cglib/core/ReflectUtils.java#L566
Fallthrough throw here:
https://github.com/spring-projects/spring-framework/blob/v5.3.12/spring-core/src/main/java/org/springframework/cglib/core/ReflectUtils.java#L589

The exception with the naming issue isnt thrown though and occurs here:
https://github.com/spring-projects/spring-framework/blob/v5.3.12/spring-core/src/main/java/org/springframework/cglib/core/ReflectUtils.java#L507

because the package name starts with a $ sign so the ContextClass is not in the same package and it results in a IllegalArgumentException.
For IllegalArgumentException and LinkageErrors there exists a fallback in the ReflectUtils using the ClassLoader::defineClass via reflection which is no longer allowed due to the strong encapsulation.

the package name is determined in this stacktrace:
AbstractClassGenerator::generate(ClassLoaderData data)
https://github.com/spring-projects/spring-framework/blob/v5.3.12/spring-core/src/main/java/org/springframework/cglib/core/AbstractClassGenerator.java#L345
AbstractClassGenerator::generateClassName(Predicate nameTestPrediacte)
https://github.com/spring-projects/spring-framework/blob/v5.3.12/spring-core/src/main/java/org/springframework/cglib/core/AbstractClassGenerator.java#L177
SpringNamingPolicy via inheritance from DefaultNamingPolicy::getClassName(String prefix, String source, Object key, Predicate names):
https://github.com/cglib/cglib/blob/master/cglib/src/main/java/net/sf/cglib/core/DefaultNamingPolicy.java#L42

@MuellerMP
Copy link
Author

MuellerMP commented Oct 30, 2021

I have to admit debugging this sort of error handling is non-trivial and using a reassignable temporary variable that is thrown somewhere else instead of using the Throwable(Throwable cause) constructor first and throwing after is very evil... =(

  1. You dont know where the exception actually occured
  2. You dont know if there occured an exception beforehand because it is simply ignored and overriden by the latter occuring one

IMO calling Throwable::addSurpressed would be a better alternative if you really dont want to throw...

@rstoyanchev rstoyanchev added the in: core Issues in core modules (aop, beans, core, context, expression) label Nov 10, 2021
@sbrannen sbrannen changed the title cglib doesnt allow packages to start with "java...." CGLIB does not allow packages to start with "java" Feb 1, 2022
@sbrannen
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

6 participants