Enable ByteCode verification by default#5774
Merged
Merged
Conversation
BenchmarksStartupParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 62 cases. Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.20.0-SNAPSHOT~460ad10f3f, baseline=1.20.0-SNAPSHOT~be378f24bc
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (986.592 ms) : 0, 986592
Total [baseline] (9.156 s) : 0, 9156286
Agent [candidate] (987.838 ms) : 0, 987838
Total [candidate] (9.122 s) : 0, 9121949
section appsec
Agent [baseline] (1.071 s) : 0, 1071036
Total [baseline] (9.253 s) : 0, 9253178
Agent [candidate] (1.069 s) : 0, 1069262
Total [candidate] (9.235 s) : 0, 9234853
section iast
Agent [baseline] (1.09 s) : 0, 1089821
Total [baseline] (9.387 s) : 0, 9386729
Agent [candidate] (1.09 s) : 0, 1089570
Total [candidate] (9.392 s) : 0, 9392332
section profiling
Agent [baseline] (1.16 s) : 0, 1160058
Total [baseline] (9.412 s) : 0, 9411552
Agent [candidate] (1.158 s) : 0, 1157600
Total [candidate] (9.49 s) : 0, 9489991
gantt
title petclinic - break down per module: candidate=1.20.0-SNAPSHOT~460ad10f3f, baseline=1.20.0-SNAPSHOT~be378f24bc
dateFormat X
axisFormat %s
section tracing
BytebuddyAgent [baseline] (611.838 ms) : 0, 611838
BytebuddyAgent [candidate] (612.206 ms) : 0, 612206
GlobalTracer [baseline] (283.731 ms) : 0, 283731
GlobalTracer [candidate] (284.421 ms) : 0, 284421
AppSec [baseline] (49.996 ms) : 0, 49996
AppSec [candidate] (50.233 ms) : 0, 50233
Remote Config [baseline] (666.688 µs) : 0, 667
Remote Config [candidate] (665.001 µs) : 0, 665
Telemetry [baseline] (5.988 ms) : 0, 5988
Telemetry [candidate] (6.056 ms) : 0, 6056
section appsec
BytebuddyAgent [baseline] (612.38 ms) : 0, 612380
BytebuddyAgent [candidate] (610.752 ms) : 0, 610752
GlobalTracer [baseline] (283.072 ms) : 0, 283072
GlobalTracer [candidate] (283.121 ms) : 0, 283121
AppSec [baseline] (134.872 ms) : 0, 134872
AppSec [candidate] (134.879 ms) : 0, 134879
Remote Config [baseline] (752.346 µs) : 0, 752
Remote Config [candidate] (721.675 µs) : 0, 722
Telemetry [baseline] (5.714 ms) : 0, 5714
Telemetry [candidate] (5.708 ms) : 0, 5708
section iast
BytebuddyAgent [baseline] (718.073 ms) : 0, 718073
BytebuddyAgent [candidate] (718.136 ms) : 0, 718136
GlobalTracer [baseline] (270.089 ms) : 0, 270089
GlobalTracer [candidate] (269.725 ms) : 0, 269725
AppSec [baseline] (46.664 ms) : 0, 46664
AppSec [candidate] (46.975 ms) : 0, 46975
Remote Config [baseline] (574.84 µs) : 0, 575
Remote Config [candidate] (566.1 µs) : 0, 566
Telemetry [baseline] (5.761 ms) : 0, 5761
Telemetry [candidate] (5.663 ms) : 0, 5663
IAST [baseline] (14.378 ms) : 0, 14378
IAST [candidate] (14.33 ms) : 0, 14330
section profiling
BytebuddyAgent [baseline] (626.258 ms) : 0, 626258
BytebuddyAgent [candidate] (625.041 ms) : 0, 625041
GlobalTracer [baseline] (347.33 ms) : 0, 347330
GlobalTracer [candidate] (347.086 ms) : 0, 347086
AppSec [baseline] (48.966 ms) : 0, 48966
AppSec [candidate] (49.218 ms) : 0, 49218
Remote Config [baseline] (636.507 µs) : 0, 637
Remote Config [candidate] (641.711 µs) : 0, 642
Telemetry [baseline] (5.959 ms) : 0, 5959
Telemetry [candidate] (5.983 ms) : 0, 5983
ProfilingAgent [baseline] (78.101 ms) : 0, 78101
ProfilingAgent [candidate] (76.978 ms) : 0, 76978
Profiling [baseline] (78.125 ms) : 0, 78125
Profiling [candidate] (77.002 ms) : 0, 77002
Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.20.0-SNAPSHOT~460ad10f3f, baseline=1.20.0-SNAPSHOT~be378f24bc
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (987.127 ms) : 0, 987127
Total [baseline] (8.617 s) : 0, 8616542
Agent [candidate] (989.922 ms) : 0, 989922
Total [candidate] (8.625 s) : 0, 8625108
section appsec
Agent [baseline] (1.073 s) : 0, 1072672
Total [baseline] (8.681 s) : 0, 8680618
Agent [candidate] (1.073 s) : 0, 1072882
Total [candidate] (8.688 s) : 0, 8688409
section iast
Agent [baseline] (1.088 s) : 0, 1088349
Total [baseline] (9.148 s) : 0, 9147772
Agent [candidate] (1.097 s) : 0, 1097498
Total [candidate] (9.192 s) : 0, 9191601
section profiling
Agent [baseline] (1.164 s) : 0, 1164012
Total [baseline] (8.857 s) : 0, 8857142
Agent [candidate] (1.158 s) : 0, 1158157
Total [candidate] (8.849 s) : 0, 8849416
gantt
title insecure-bank - break down per module: candidate=1.20.0-SNAPSHOT~460ad10f3f, baseline=1.20.0-SNAPSHOT~be378f24bc
dateFormat X
axisFormat %s
section tracing
BytebuddyAgent [baseline] (612.119 ms) : 0, 612119
BytebuddyAgent [candidate] (614.135 ms) : 0, 614135
GlobalTracer [baseline] (284.317 ms) : 0, 284317
GlobalTracer [candidate] (284.775 ms) : 0, 284775
AppSec [baseline] (49.753 ms) : 0, 49753
AppSec [candidate] (50.032 ms) : 0, 50032
Remote Config [baseline] (660.779 µs) : 0, 661
Remote Config [candidate] (661.494 µs) : 0, 661
Telemetry [baseline] (5.962 ms) : 0, 5962
Telemetry [candidate] (5.973 ms) : 0, 5973
section appsec
BytebuddyAgent [baseline] (612.468 ms) : 0, 612468
BytebuddyAgent [candidate] (613.274 ms) : 0, 613274
GlobalTracer [baseline] (284.126 ms) : 0, 284126
GlobalTracer [candidate] (284.05 ms) : 0, 284050
AppSec [baseline] (135.263 ms) : 0, 135263
AppSec [candidate] (134.818 ms) : 0, 134818
Remote Config [baseline] (723.967 µs) : 0, 724
Remote Config [candidate] (720.605 µs) : 0, 721
Telemetry [baseline] (5.731 ms) : 0, 5731
Telemetry [candidate] (5.707 ms) : 0, 5707
section iast
BytebuddyAgent [baseline] (717.663 ms) : 0, 717663
BytebuddyAgent [candidate] (723.501 ms) : 0, 723501
GlobalTracer [baseline] (269.124 ms) : 0, 269124
GlobalTracer [candidate] (271.804 ms) : 0, 271804
AppSec [baseline] (46.657 ms) : 0, 46657
AppSec [candidate] (46.965 ms) : 0, 46965
Remote Config [baseline] (573.296 µs) : 0, 573
Remote Config [candidate] (578.719 µs) : 0, 579
Telemetry [baseline] (5.705 ms) : 0, 5705
Telemetry [candidate] (5.753 ms) : 0, 5753
IAST [baseline] (14.34 ms) : 0, 14340
IAST [candidate] (14.445 ms) : 0, 14445
section profiling
BytebuddyAgent [baseline] (628.766 ms) : 0, 628766
BytebuddyAgent [candidate] (624.87 ms) : 0, 624870
GlobalTracer [baseline] (349.495 ms) : 0, 349495
GlobalTracer [candidate] (347.09 ms) : 0, 347090
AppSec [baseline] (49.256 ms) : 0, 49256
AppSec [candidate] (49.058 ms) : 0, 49058
Remote Config [baseline] (663.215 µs) : 0, 663
Remote Config [candidate] (630.295 µs) : 0, 630
Telemetry [baseline] (6.019 ms) : 0, 6019
Telemetry [candidate] (5.873 ms) : 0, 5873
ProfilingAgent [baseline] (76.602 ms) : 0, 76602
ProfilingAgent [candidate] (78.012 ms) : 0, 78012
Profiling [baseline] (76.627 ms) : 0, 76627
Profiling [candidate] (78.036 ms) : 0, 78036
LoadParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 24 cases. Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.20.0-SNAPSHOT~460ad10f3f, baseline=1.20.0-SNAPSHOT~be378f24bc
dateFormat X
axisFormat %s
section baseline
no_agent (1.318 ms) : 1299, 1337
. : milestone, 1318,
appsec (1.681 ms) : 1656, 1706
. : milestone, 1681,
iast (1.446 ms) : 1422, 1471
. : milestone, 1446,
profiling (1.436 ms) : 1411, 1460
. : milestone, 1436,
tracing (1.433 ms) : 1409, 1456
. : milestone, 1433,
section candidate
no_agent (1.343 ms) : 1324, 1362
. : milestone, 1343,
appsec (1.662 ms) : 1638, 1686
. : milestone, 1662,
iast (1.467 ms) : 1443, 1491
. : milestone, 1467,
profiling (1.46 ms) : 1436, 1484
. : milestone, 1460,
tracing (1.453 ms) : 1429, 1476
. : milestone, 1453,
Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.20.0-SNAPSHOT~460ad10f3f, baseline=1.20.0-SNAPSHOT~be378f24bc
dateFormat X
axisFormat %s
section baseline
no_agent (366.481 µs) : 346, 387
. : milestone, 366,
appsec (654.94 µs) : 634, 676
. : milestone, 655,
iast (444.806 µs) : 424, 465
. : milestone, 445,
iast_FULL (512.142 µs) : 491, 534
. : milestone, 512,
iast_INACTIVE (425.993 µs) : 405, 447
. : milestone, 426,
profiling (425.899 µs) : 405, 447
. : milestone, 426,
tracing (423.087 µs) : 402, 444
. : milestone, 423,
section candidate
no_agent (361.048 µs) : 341, 381
. : milestone, 361,
appsec (659.298 µs) : 638, 680
. : milestone, 659,
iast (447.965 µs) : 427, 469
. : milestone, 448,
iast_FULL (513.747 µs) : 493, 535
. : milestone, 514,
iast_INACTIVE (433.225 µs) : 412, 454
. : milestone, 433,
profiling (434.738 µs) : 413, 456
. : milestone, 435,
tracing (426.725 µs) : 406, 447
. : milestone, 427,
|
shatzi
approved these changes
Aug 27, 2023
shatzi
left a comment
shatzi
left a comment
Contributor
There was a problem hiding this comment.
Awesome stuff + few comments
| } | ||
|
|
||
| @Test | ||
| public void lineRangeErrorSimpleSpan() throws IOException, URISyntaxException { |
Contributor
There was a problem hiding this comment.
This test, while verify the code changes here, is very hard to understand.
this test doesn't not verify we run the verification logic, but instead that we didn't generate a span. so this can hide bug and not protect against regression. For example, let assume we got a different code that fails the instrumentation, this test will pass. but we don't have a code that verify that we run the static code analysis.
Can we create a more specific test verify we call static code analysis function? or maybe there is a better test to verify our safeguards are on by default?
Member
Author
There was a problem hiding this comment.
yes that's true that this test does not specially test that the static analysis is run. but it's really its purpose it's more that if an error during instrumentation happens, we dont' blow up the program and it still run correctly.
So in a sense it's more an integration test than a unit test.
I don't really care if this is the static analysis that is run or not, just that the program run correctly with or without ant instrumentation failure. here this is one.
Contributor
There was a problem hiding this comment.
Maybe add a log assertion for "Verify result" or "Verification of instrumented class {} failed" if possible?
Member
Author
There was a problem hiding this comment.
Added an additional test that can only pass with bytecode verification.
| Config config = mock(Config.class); | ||
| when(config.isDebuggerEnabled()).thenReturn(true); | ||
| when(config.isDebuggerClassFileDumpEnabled()).thenReturn(true); | ||
| when(config.isDebuggerVerifyByteCode()).thenReturn(true); |
Contributor
There was a problem hiding this comment.
Why we no longer use "true" here? but "true" in other tests? what is the logic here?
Contributor
There was a problem hiding this comment.
I'm also confused about this change and would appreciate an explanation.
Member
Author
There was a problem hiding this comment.
This is not necessary for those tests, and all tests passed without it.
| Config config = mock(Config.class); | ||
| when(config.isDebuggerEnabled()).thenReturn(true); | ||
| when(config.isDebuggerClassFileDumpEnabled()).thenReturn(true); | ||
| when(config.isDebuggerVerifyByteCode()).thenReturn(true); |
Contributor
There was a problem hiding this comment.
if DEFAULT_DEBUGGER_VERIFY_BYTECODE = true; - do we still need this?
Member
Author
There was a problem hiding this comment.
yes because the default mock have all its fields initialize with default value, i.e. false.
ygree
reviewed
Aug 28, 2023
| } | ||
|
|
||
| private void verifyByteCode(String classFilePath, byte[] classFile) { | ||
| if (config.isDebuggerVerifyByteCode()) { |
Contributor
There was a problem hiding this comment.
This part of the change is a bit distracting because it doesn't change anything.
ygree
approved these changes
Aug 28, 2023
ygree
left a comment
ygree
left a comment
Contributor
There was a problem hiding this comment.
Overall looks good. One suggestion for adding a validation log record test assertion if possible.
Once we have generated the new classfile we run a static analysis of the bytecode using the ASM library's analyzer. It helps find out before sending back the modified classfile to the JVM if the bytecode is not correct and then cancelling the instrumentation to not impact the user code.
only pass test if verify bytecode is run
jpbempel
force-pushed
the
jpbempel/enable-bytecode-verification
branch
from
August 29, 2023 16:10
d786e0a to
460ad10
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What Does This Do
Once we have generated the new classfile we run a static analysis of the bytecode using the ASM library's analyzer. It helps find out before sending back the modified classfile to the JVM if the bytecode is not correct and then cancelling the instrumentation to not impact the user code.
Motivation
avoid verifier errors preventing class to be loaded
Additional Notes