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

Allow SDK to run in environments prohibiting use of sun.misc.Unsafe #4902

Merged
merged 2 commits into from
Nov 9, 2022

Conversation

rrva
Copy link
Contributor

@rrva rrva commented Oct 31, 2022

Some applications run under strict java.security permissions which do not allow access to sun.misc.Unsafe.

BatchSpanProcessor uses Unsafe via jctools, but has a fallback to ArrayBlockingQueue. Extending that fallback rule to cover java security exceptions as well.

Since the entire java security manager is marked for deprecation in future java versions, I went with string-matching on the root cause message, which removes deprecation warnings when building with newer java but still does the job in those versions which use java.security policies.

@rrva rrva requested a review from a team October 31, 2022 21:39
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 31, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: rrva / name: Ragnar Rova (5c283d9)

@rrva
Copy link
Contributor Author

rrva commented Oct 31, 2022

The Individual CLA option is not enabled for this project.

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

thx @rrva! do you have any ideas on how to run some(?) tests under the security manager?

@trask
Copy link
Member

trask commented Oct 31, 2022

The Individual CLA option is not enabled for this project.

hi @rrva, it should be, can you attach a screenshot of what you see?

@rrva
Copy link
Contributor Author

rrva commented Nov 1, 2022

The Individual CLA option is not enabled for this project.

hi @rrva, it should be, can you attach a screenshot of what you see?

Signed. Yesterday I got an error saying the individual option was not enabled and I should contact the project admin.

@rrva
Copy link
Contributor Author

rrva commented Nov 1, 2022

thx @rrva! do you have any ideas on how to run some(?) tests under the security manager?

I have added a test and squashed all commits

@codecov
Copy link

codecov bot commented Nov 1, 2022

Codecov Report

Base: 90.93% // Head: 90.96% // Increases project coverage by +0.03% 🎉

Coverage data is based on head (511a3ac) compared to base (ef076c2).
Patch coverage: 33.33% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #4902      +/-   ##
============================================
+ Coverage     90.93%   90.96%   +0.03%     
- Complexity     4817     4820       +3     
============================================
  Files           545      545              
  Lines         14383    14388       +5     
  Branches       1383     1384       +1     
============================================
+ Hits          13079    13088       +9     
+ Misses          901      897       -4     
  Partials        403      403              
Impacted Files Coverage Δ
...a/io/opentelemetry/sdk/trace/internal/JcTools.java 31.57% <33.33%> (+3.00%) ⬆️
...metry/sdk/logs/export/BatchLogRecordProcessor.java 89.70% <0.00%> (+0.73%) ⬆️
.../io/opentelemetry/api/internal/PercentEscaper.java 84.21% <0.00%> (+4.51%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@rrva rrva force-pushed the security branch 3 times, most recently from da4d961 to 919d292 Compare November 1, 2022 21:32
@rrva
Copy link
Contributor Author

rrva commented Nov 1, 2022

I get higher code coverage of JcTools.java than codecov when checking with "Run with coverage" in IntelliJ. What could be wrong?

@rrva
Copy link
Contributor Author

rrva commented Nov 1, 2022

@trask Please have a look at what I could do to the coverage issue of JcTools.java, it's a bit tricky.

I think what is happening is that MpscArrayQueueProducerIndexField.P_INDEX_OFFSET is sometimes sucessfully initialized before the SunMiscProhibitedSecurityManager is installed. Since it is a static field it is only initialized once per JVM invocation, it stays that way. This initialization is needed to fail if JcToolsSecurityManagerTest is to be allowed to cover the code paths I have changed.

I'm a bit unsure if this theory holds but thats what I think is happening when running jacoco.

I think we either need to find a way that jacoco runs JcToolsSecurityManagerTest in an isolated jvm or excluding the changed code from coverage checks.

When I run JcToolsSecurityManagerTest and JcToolsTest test separately in IntelliJ I do get coverage on all paths. When I run JcToolsSecurityManagerTest without the changes in this PR it fails as expected.

@jkwatson
Copy link
Contributor

jkwatson commented Nov 2, 2022

codecov is claiming quite a bit of uncovered new code in this PR. Can you add more test cases to cover what it says isn't covered?

sdk/trace-shaded-deps/build.gradle.kts Outdated Show resolved Hide resolved
@rrva
Copy link
Contributor Author

rrva commented Nov 2, 2022

@jsuereth

I simplified the actual change to the code down to a single line, the rest is test code (which jacoco has trouble recognizing coverage of when running the entire projects' tests).

It also needs this ugly thing (only in the build.gradle.kts of the sdk/trace-shaded-deps module which today only contains a single other test)

tasks.withType<Test>().configureEach {
  // JcToolsSecurityManagerTest interferes with JcToolsTest
  setForkEvery(1)
}

If you're fine with that, thats OK by me. OR, I drop the entire test since:

  • Jacoco did not recognize coverage properly
  • Tests that interfere with each other and leave the test JVM in some unclean state (yuck)
  • The old code had a clause for catching NoClassDefFoundError, with a reworked simplified impl I am actually just adding a case to that, and that functionality did not have any test coverage either ;)

@rrva
Copy link
Contributor Author

rrva commented Nov 2, 2022

sorry the tests probably do not leave the test JVM in an unclean state, but if other tests are run concurrently with the JcToolsSecurityManagerTest then it will get ArrayBlockingQueue instead of JcTools queue (a bit randomly since test execution parallelism/ordering is not deterministic)

Copy link
Member

@jack-berg jack-berg left a comment

Choose a reason for hiding this comment

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

Just a couple of small comments but looks good otherwise

@rrva
Copy link
Contributor Author

rrva commented Nov 8, 2022

is this mergeable?

@rrva
Copy link
Contributor Author

rrva commented Nov 8, 2022

the only thing I am afraid of is that it will degrade performance in these environments without any warning so perhaps a warning should be logged

@jack-berg
Copy link
Member

the only thing I am afraid of is that it will degrade performance in these environments without any warning so perhaps a warning should be logged

I'm in favor of logging a warning though. Should make sure its only logged once, rather than each time a queue is created.

@rrva
Copy link
Contributor Author

rrva commented Nov 8, 2022

the only thing I am afraid of is that it will degrade performance in these environments without any warning so perhaps a warning should be logged

I'm in favor of logging a warning though. Should make sure its only logged once, rather than each time a queue is created.

Warning added and duplicates mostly avoided (could still happen when creating many queues concurrently)

Rebased.

Good to merge?

@rrva rrva force-pushed the security branch 2 times, most recently from e07af3e to ed5c0d8 Compare November 8, 2022 20:30
Some applications run under strict java.security permissions
which do not allow access to sun.misc.Unsafe.

BatchSpanProcessor uses Unsafe via jctools, but has a fallback to
ArrayBlockingQueue. Extending that fallback rule to cover
java security exceptions as well.
@jack-berg jack-berg merged commit 36adb27 into open-telemetry:main Nov 9, 2022
dmarkwat pushed a commit to dmarkwat/opentelemetry-java that referenced this pull request Dec 30, 2022
…pen-telemetry#4902)

Some applications run under strict java.security permissions
which do not allow access to sun.misc.Unsafe.

BatchSpanProcessor uses Unsafe via jctools, but has a fallback to
ArrayBlockingQueue. Extending that fallback rule to cover
java security exceptions as well.

Co-authored-by: Jack Berg <jberg@newrelic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants