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.

Sign up for GitHub

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

Jump to bottom

Obfuscate virtual field accessors #4385

Merged
merged 5 commits into from
Oct 15, 2021
Merged

Obfuscate virtual field accessors #4385

merged 5 commits into from
Oct 15, 2021

Conversation

trask
Copy link
Member

@trask trask commented Oct 14, 2021

Resolves application errors when monitoring Confluence with the javaagent:

2021-10-14 15:02:48,369 ERROR [Catalina-utility-1] [springframework.web.context.ContextLoader] initWebApplicationContext Context initialization failed
org.springframework.beans.factory.BeanCreationException: Error creating bean with name 'clusterCacheCompactor' defined in class path resource [services/cacheServiceContext.xml]: Invocation of init method failed; nested exception is org.springframework.beans.factory.UnsatisfiedDependencyException: Error creating bean with name 'com.atlassian.confluence.cache.NoopClusterCacheCompactor': Unsatisfied dependency expressed through bean property '__opentelemetryVirtualField$java$lang$Runnable$io$opentelemetry$javaagent$instrumentation$api$concurrent$PropagatedContext': Set this property value or disable dependency checking for this bean.
	at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.initializeBean(AbstractAutowireCapableBeanFactory.java:1778)
	at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.doCreateBean(AbstractAutowireCapableBeanFactory.java:593)
	at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.createBean(AbstractAutowireCapableBeanFactory.java:515)
	at org.springframework.beans.factory.support.AbstractBeanFactory.lambda$doGetBean$0(AbstractBeanFactory.java:320)
	at org.springframework.beans.factory.support.DefaultSingletonBeanRegistry.getSingleton(DefaultSingletonBeanRegistry.java:222)
	at org.springframework.beans.factory.support.AbstractBeanFactory.doGetBean(AbstractBeanFactory.java:318)
	at org.springframework.beans.factory.support.AbstractBeanFactory.getBean(AbstractBeanFactory.java:199)
	at org.springframework.beans.factory.support.DefaultListableBeanFactory.preInstantiateSingletons(DefaultListableBeanFactory.java:826)
	at org.springframework.context.support.AbstractApplicationContext.finishBeanFactoryInitialization(AbstractApplicationContext.java:877)
	at org.springframework.context.support.AbstractApplicationContext.refresh(AbstractApplicationContext.java:549)
	at org.springframework.web.context.ContextLoader.configureAndRefreshWebApplicationContext(ContextLoader.java:400)
	at org.springframework.web.context.ContextLoader.initWebApplicationContext(ContextLoader.java:291)
	at org.springframework.web.context.ContextLoaderListener.contextInitialized(ContextLoaderListener.java:103)
	at com.atlassian.spring.container.ContainerContextLoaderListener.contextInitialized(ContainerContextLoaderListener.java:32)
	at com.atlassian.confluence.setup.ConfluenceBootstrappedContextLoaderListener.contextInitialized(ConfluenceBootstrappedContextLoaderListener.java:38)
	at org.apache.catalina.core.StandardContext.listenerStart(StandardContext.java:4690)
	at org.apache.catalina.core.StandardContext.startInternal(StandardContext.java:5151)
	at org.apache.catalina.util.LifecycleBase.start(LifecycleBase.java:183)
	at org.apache.catalina.core.ContainerBase$StartChild.call(ContainerBase.java:1384)
	at org.apache.catalina.core.ContainerBase$StartChild.call(ContainerBase.java:1374)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
	at java.base/java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:304)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
	at org.apache.tomcat.util.threads.TaskThread$WrappingRunnable.run(TaskThread.java:61)
	at java.base/java.lang.Thread.run(Thread.java:829)
Caused by: org.springframework.beans.factory.UnsatisfiedDependencyException: Error creating bean with name 'com.atlassian.confluence.cache.NoopClusterCacheCompactor': Unsatisfied dependency expressed through bean property '__opentelemetryVirtualField$java$lang$Runnable$io$opentelemetry$javaagent$instrumentation$api$concurrent$PropagatedContext': Set this property value or disable dependency checking for this bean.
	at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.checkDependencies(AbstractAutowireCapableBeanFactory.java:1618)
	at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.populateBean(AbstractAutowireCapableBeanFactory.java:1429)
	at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.doCreateBean(AbstractAutowireCapableBeanFactory.java:592)
	at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.createBean(AbstractAutowireCapableBeanFactory.java:515)
	at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.createBean(AbstractAutowireCapableBeanFactory.java:351)
	at com.atlassian.confluence.cluster.ClusterAwareFactoryBean.getSingleMatchingBean(ClusterAwareFactoryBean.java:140)
	at com.atlassian.confluence.cluster.ClusterAwareFactoryBean.getMatchingImplementationBean(ClusterAwareFactoryBean.java:96)
	at com.atlassian.confluence.cluster.ClusterAwareFactoryBean.createInstance(ClusterAwareFactoryBean.java:86)
	at org.springframework.beans.factory.config.AbstractFactoryBean.afterPropertiesSet(AbstractFactoryBean.java:142)
	at com.atlassian.confluence.cluster.ClusterAwareFactoryBean.afterPropertiesSet(ClusterAwareFactoryBean.java:64)
	at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.invokeInitMethods(AbstractAutowireCapableBeanFactory.java:1837)
	at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.initializeBean(AbstractAutowireCapableBeanFactory.java:1774)
	... 25 more

breedx-splk
breedx-splk approved these changes Oct 14, 2021
View reviewed changes
Copy link
Contributor

@breedx-splk breedx-splk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, and appreciate the clear comment.

@trask trask force-pushed the obfuscate-virtual-field-accessors branch from d93f028 to 38c16bb Compare October 15, 2021 00:49
iNikem
iNikem approved these changes Oct 15, 2021
View reviewed changes
...g/src/test/java/io/opentelemetry/javaagent/tooling/field/GeneratedVirtualFieldNamesTest.java
@@ -39,15 +39,15 @@ void field() {
@Test
void setter() {
assertEquals(
"set__opentelemetryVirtualField$java$lang$Runnable$java$lang$String__",
"__set__opentelemetryVirtualField$java$lang$Runnable$java$lang$String__",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need then __ both before and after set/get prefix?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The last __ is for encoding [] in array class name suffix. This is needed because [ is one of the few characters that can't be used in a field or method name https://docs.oracle.com/javase/specs/jvms/se17/html/jvms-4.html#jvms-4.2.2

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant the second __ in __set__

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's not needed, but seems ok to me (and code is shorter to include it since it's part of the field name)

@laurit laurit mentioned this pull request Oct 15, 2021
Merged
laurit
laurit approved these changes Oct 15, 2021
View reviewed changes
Copy link
Contributor

@laurit laurit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I think that the idea of this pr is good, adding __ prefix to get/set methods makes them less likely to confuse something that is looking for plain getters and setters I also believe that this pr does not tackle the root cause of the problem, user code should never have seen these methods in the first place. Made another pr #4390 that hopefully fixes this.

@trask
Copy link
Member Author

trask commented Oct 15, 2021

I removed the comment, since #4390 takes care of that now, but I think worth keeping the method name change

@trask trask merged commit 0652894 into open-telemetry:main Oct 15, 2021
@trask trask deleted the obfuscate-virtual-field-accessors branch October 15, 2021 21:36
RashmiRam pushed a commit to RashmiRam/opentelemetry-auto-instr-java that referenced this pull request May 23, 2022
@trask @RashmiRam
Obfuscate virtual field accessors (open-telemetry#4385)
587a399 
* Obfuscate virtual field accessors

* Update internal-reflection instrumentation

* Update test

* Remove outdated comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iNikem iNikem iNikem approved these changes

@laurit laurit laurit approved these changes

@mateuszrzeszutek mateuszrzeszutek mateuszrzeszutek approved these changes

@breedx-splk breedx-splk breedx-splk approved these changes

@anuraaga anuraaga Awaiting requested review from anuraaga

@jkwatson jkwatson Awaiting requested review from jkwatson

@pavolloffay pavolloffay Awaiting requested review from pavolloffay

@tylerbenson tylerbenson Awaiting requested review from tylerbenson

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@trask @laurit @iNikem @mateuszrzeszutek @breedx-splk