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

Prevent calling size method from unknown classes #5946

Merged
merged 2 commits into from
Sep 27, 2023
Merged

Prevent calling size method from unknown classes #5946

merged 2 commits into from
Sep 27, 2023

Conversation

jpbempel
Copy link
Member

What Does This Do

we are preventing to call on unsafe implementation. We are limiting to JDK implementation first.

Motivation

Collection and Map interfaces can be implemented by anyone and therefore may not be safe to call. example: persistent layer that load lazily large amount of rows from DB.

Additional Notes

@jpbempel jpbempel requested a review from a team as a code owner September 27, 2023 09:48
@jpbempel jpbempel requested review from ojung and removed request for a team September 27, 2023 09:48
@jpbempel jpbempel added the comp: debugger Dynamic Instrumentation label Sep 27, 2023
@pr-commenter
Copy link

pr-commenter bot commented Sep 27, 2023 •
edited
Loading

Benchmarks

Startup

Parameters

Baseline Candidate
commit 1.22.0-SNAPSHOT~626e570b5c 1.22.0-SNAPSHOT~a07d37dc72
config baseline candidate
See matching parameters
Baseline Candidate
application insecure-bank insecure-bank
module Agent Agent
parent None None
variant appsec appsec

Summary

Found 0 performance improvements and 0 performance regressions! Performance is the same for 62 cases.

Startup time reports for petclinic 
gantt
    title petclinic - global startup overhead: candidate=1.22.0-SNAPSHOT~a07d37dc72, baseline=1.22.0-SNAPSHOT~626e570b5c

    dateFormat X
    axisFormat %s
section tracing
Agent [baseline] (1.006 s) : 0, 1006288
Total [baseline] (9.197 s) : 0, 9197262
Agent [candidate] (1.017 s) : 0, 1017221
Total [candidate] (9.25 s) : 0, 9249969
section appsec
Agent [baseline] (1.095 s) : 0, 1094898
Total [baseline] (9.319 s) : 0, 9319101
Agent [candidate] (1.103 s) : 0, 1102565
Total [candidate] (9.263 s) : 0, 9262770
section iast
Agent [baseline] (1.125 s) : 0, 1125126
Total [baseline] (9.435 s) : 0, 9435481
Agent [candidate] (1.119 s) : 0, 1119461
Total [candidate] (9.434 s) : 0, 9434361
section profiling
Agent [baseline] (1.186 s) : 0, 1186083
Total [baseline] (9.442 s) : 0, 9441877
Agent [candidate] (1.203 s) : 0, 1203285
Total [candidate] (9.559 s) : 0, 9559399
Loading
  • baseline results
Module Variant Duration Δ tracing
Agent tracing 1.006 s -
Agent appsec 1.095 s 88.609 ms (8.8%)
Agent iast 1.125 s 118.837 ms (11.8%)
Agent profiling 1.186 s 179.795 ms (17.9%)
Total tracing 9.197 s -
Total appsec 9.319 s 121.839 ms (1.3%)
Total iast 9.435 s 238.219 ms (2.6%)
Total profiling 9.442 s 244.615 ms (2.7%)
  • candidate results
Module Variant Duration Δ tracing
Agent tracing 1.017 s -
Agent appsec 1.103 s 85.344 ms (8.4%)
Agent iast 1.119 s 102.239 ms (10.1%)
Agent profiling 1.203 s 186.064 ms (18.3%)
Total tracing 9.25 s -
Total appsec 9.263 s 12.802 ms (0.1%)
Total iast 9.434 s 184.393 ms (2.0%)
Total profiling 9.559 s 309.431 ms (3.3%) 
gantt
    title petclinic - break down per module: candidate=1.22.0-SNAPSHOT~a07d37dc72, baseline=1.22.0-SNAPSHOT~626e570b5c

    dateFormat X
    axisFormat %s
section tracing
BytebuddyAgent [baseline] (623.802 ms) : 0, 623802
BytebuddyAgent [candidate] (629.981 ms) : 0, 629981
GlobalTracer [baseline] (293.138 ms) : 0, 293138
GlobalTracer [candidate] (296.77 ms) : 0, 296770
AppSec [baseline] (48.362 ms) : 0, 48362
AppSec [candidate] (49.167 ms) : 0, 49167
Remote Config [baseline] (665.051 µs) : 0, 665
Remote Config [candidate] (674.256 µs) : 0, 674
Telemetry [baseline] (5.997 ms) : 0, 5997
Telemetry [candidate] (6.074 ms) : 0, 6074
section appsec
BytebuddyAgent [baseline] (625.12 ms) : 0, 625120
BytebuddyAgent [candidate] (629.532 ms) : 0, 629532
GlobalTracer [baseline] (293.377 ms) : 0, 293377
GlobalTracer [candidate] (296.028 ms) : 0, 296028
AppSec [baseline] (135.543 ms) : 0, 135543
AppSec [candidate] (135.97 ms) : 0, 135970
Remote Config [baseline] (653.198 µs) : 0, 653
Remote Config [candidate] (645.454 µs) : 0, 645
Telemetry [baseline] (5.877 ms) : 0, 5877
Telemetry [candidate] (5.871 ms) : 0, 5871
section iast
BytebuddyAgent [baseline] (744.602 ms) : 0, 744602
BytebuddyAgent [candidate] (739.477 ms) : 0, 739477
GlobalTracer [baseline] (278.744 ms) : 0, 278744
GlobalTracer [candidate] (278.605 ms) : 0, 278605
AppSec [baseline] (45.964 ms) : 0, 45964
AppSec [candidate] (46.103 ms) : 0, 46103
IAST [baseline] (14.592 ms) : 0, 14592
IAST [candidate] (14.335 ms) : 0, 14335
Remote Config [baseline] (569.692 µs) : 0, 570
Remote Config [candidate] (549.572 µs) : 0, 550
Telemetry [baseline] (5.841 ms) : 0, 5841
Telemetry [candidate] (5.74 ms) : 0, 5740
section profiling
BytebuddyAgent [baseline] (640.576 ms) : 0, 640576
BytebuddyAgent [candidate] (649.524 ms) : 0, 649524
GlobalTracer [baseline] (356.285 ms) : 0, 356285
GlobalTracer [candidate] (360.643 ms) : 0, 360643
AppSec [baseline] (48.903 ms) : 0, 48903
AppSec [candidate] (49.859 ms) : 0, 49859
Remote Config [baseline] (658.068 µs) : 0, 658
Remote Config [candidate] (668.921 µs) : 0, 669
Telemetry [baseline] (6.086 ms) : 0, 6086
Telemetry [candidate] (6.14 ms) : 0, 6140
ProfilingAgent [baseline] (80.216 ms) : 0, 80216
ProfilingAgent [candidate] (82.27 ms) : 0, 82270
Profiling [baseline] (80.24 ms) : 0, 80240
Profiling [candidate] (82.295 ms) : 0, 82295
Loading
Startup time reports for insecure-bank 
gantt
    title insecure-bank - global startup overhead: candidate=1.22.0-SNAPSHOT~a07d37dc72, baseline=1.22.0-SNAPSHOT~626e570b5c

    dateFormat X
    axisFormat %s
section tracing
Agent [baseline] (1.003 s) : 0, 1003427
Total [baseline] (8.678 s) : 0, 8678310
Agent [candidate] (1.007 s) : 0, 1007375
Total [candidate] (8.664 s) : 0, 8663701
section appsec
Agent [baseline] (1.091 s) : 0, 1091336
Total [baseline] (8.744 s) : 0, 8744126
Agent [candidate] (1.094 s) : 0, 1093923
Total [candidate] (8.738 s) : 0, 8737600
section iast
Agent [baseline] (1.117 s) : 0, 1116939
Total [baseline] (9.188 s) : 0, 9187504
Agent [candidate] (1.118 s) : 0, 1118053
Total [candidate] (9.186 s) : 0, 9185522
section profiling
Agent [baseline] (1.18 s) : 0, 1179674
Total [baseline] (8.9 s) : 0, 8899541
Agent [candidate] (1.181 s) : 0, 1181081
Total [candidate] (8.9 s) : 0, 8899800
Loading
  • baseline results
Module Variant Duration Δ tracing
Agent tracing 1.003 s -
Agent appsec 1.091 s 87.909 ms (8.8%)
Agent iast 1.117 s 113.512 ms (11.3%)
Agent profiling 1.18 s 176.247 ms (17.6%)
Total tracing 8.678 s -
Total appsec 8.744 s 65.817 ms (0.8%)
Total iast 9.188 s 509.194 ms (5.9%)
Total profiling 8.9 s 221.231 ms (2.5%)
  • candidate results
Module Variant Duration Δ tracing
Agent tracing 1.007 s -
Agent appsec 1.094 s 86.548 ms (8.6%)
Agent iast 1.118 s 110.678 ms (11.0%)
Agent profiling 1.181 s 173.707 ms (17.2%)
Total tracing 8.664 s -
Total appsec 8.738 s 73.899 ms (0.9%)
Total iast 9.186 s 521.821 ms (6.0%)
Total profiling 8.9 s 236.099 ms (2.7%) 
gantt
    title insecure-bank - break down per module: candidate=1.22.0-SNAPSHOT~a07d37dc72, baseline=1.22.0-SNAPSHOT~626e570b5c

    dateFormat X
    axisFormat %s
section tracing
BytebuddyAgent [baseline] (622.144 ms) : 0, 622144
BytebuddyAgent [candidate] (623.89 ms) : 0, 623890
GlobalTracer [baseline] (292.193 ms) : 0, 292193
GlobalTracer [candidate] (293.816 ms) : 0, 293816
AppSec [baseline] (48.167 ms) : 0, 48167
AppSec [candidate] (48.58 ms) : 0, 48580
Remote Config [baseline] (669.811 µs) : 0, 670
Remote Config [candidate] (668.776 µs) : 0, 669
Telemetry [baseline] (6.016 ms) : 0, 6016
Telemetry [candidate] (6.059 ms) : 0, 6059
section appsec
BytebuddyAgent [baseline] (622.958 ms) : 0, 622958
BytebuddyAgent [candidate] (624.281 ms) : 0, 624281
GlobalTracer [baseline] (292.606 ms) : 0, 292606
GlobalTracer [candidate] (293.548 ms) : 0, 293548
AppSec [baseline] (135.162 ms) : 0, 135162
AppSec [candidate] (135.345 ms) : 0, 135345
Remote Config [baseline] (644.922 µs) : 0, 645
Remote Config [candidate] (641.893 µs) : 0, 642
Telemetry [baseline] (5.862 ms) : 0, 5862
Telemetry [candidate] (5.832 ms) : 0, 5832
section iast
BytebuddyAgent [baseline] (738.755 ms) : 0, 738755
BytebuddyAgent [candidate] (738.982 ms) : 0, 738982
GlobalTracer [baseline] (276.511 ms) : 0, 276511
GlobalTracer [candidate] (277.856 ms) : 0, 277856
AppSec [baseline] (46.341 ms) : 0, 46341
AppSec [candidate] (45.982 ms) : 0, 45982
IAST [baseline] (14.365 ms) : 0, 14365
IAST [candidate] (14.303 ms) : 0, 14303
Remote Config [baseline] (550.237 µs) : 0, 550
Remote Config [candidate] (550.95 µs) : 0, 551
Telemetry [baseline] (5.743 ms) : 0, 5743
Telemetry [candidate] (5.73 ms) : 0, 5730
section profiling
BytebuddyAgent [baseline] (636.357 ms) : 0, 636357
BytebuddyAgent [candidate] (637.327 ms) : 0, 637327
GlobalTracer [baseline] (354.175 ms) : 0, 354175
GlobalTracer [candidate] (354.747 ms) : 0, 354747
AppSec [baseline] (48.927 ms) : 0, 48927
AppSec [candidate] (48.859 ms) : 0, 48859
Remote Config [baseline] (663.002 µs) : 0, 663
Remote Config [candidate] (651.681 µs) : 0, 652
Telemetry [baseline] (6.133 ms) : 0, 6133
Telemetry [candidate] (6.021 ms) : 0, 6021
ProfilingAgent [baseline] (80.34 ms) : 0, 80340
ProfilingAgent [candidate] (80.322 ms) : 0, 80322
Profiling [baseline] (80.364 ms) : 0, 80364
Profiling [candidate] (80.346 ms) : 0, 80346
Loading

Load

Parameters

Baseline Candidate
commit 1.22.0-SNAPSHOT~626e570b5c 1.22.0-SNAPSHOT~a07d37dc72
config baseline candidate
end_time 2023-09-27T19:10:24 2023-09-27T19:28:31
start_time 2023-09-27T19:10:06 2023-09-27T19:28:13
See matching parameters
Baseline Candidate
application insecure-bank insecure-bank
variant appsec appsec

Summary

Found 0 performance improvements and 0 performance regressions! Performance is the same for 24 cases.

Request duration reports for petclinic 
gantt
    title petclinic - request duration [CI 0.99] : candidate=1.22.0-SNAPSHOT~a07d37dc72, baseline=1.22.0-SNAPSHOT~626e570b5c
    dateFormat X
    axisFormat %s
section baseline
no_agent (1.327 ms) : 1308, 1346
.   : milestone, 1327,
appsec (1.67 ms) : 1646, 1695
.   : milestone, 1670,
iast (1.475 ms) : 1451, 1499
.   : milestone, 1475,
profiling (1.468 ms) : 1442, 1493
.   : milestone, 1468,
tracing (1.456 ms) : 1431, 1481
.   : milestone, 1456,
section candidate
no_agent (1.322 ms) : 1303, 1342
.   : milestone, 1322,
appsec (1.709 ms) : 1684, 1733
.   : milestone, 1709,
iast (1.447 ms) : 1423, 1472
.   : milestone, 1447,
profiling (1.485 ms) : 1459, 1510
.   : milestone, 1485,
tracing (1.449 ms) : 1424, 1473
.   : milestone, 1449,
Loading
  • baseline results
Variant Request duration [CI 0.99] Δ no_agent
no_agent 1.327 ms [1.308 ms, 1.346 ms] -
appsec 1.67 ms [1.646 ms, 1.695 ms] 343.068 µs (25.8%)
iast 1.475 ms [1.451 ms, 1.499 ms] 148.155 µs (11.2%)
profiling 1.468 ms [1.442 ms, 1.493 ms] 140.564 µs (10.6%)
tracing 1.456 ms [1.431 ms, 1.481 ms] 128.659 µs (9.7%)
  • candidate results
Variant Request duration [CI 0.99] Δ no_agent
no_agent 1.322 ms [1.303 ms, 1.342 ms] -
appsec 1.709 ms [1.684 ms, 1.733 ms] 386.367 µs (29.2%)
iast 1.447 ms [1.423 ms, 1.472 ms] 125.111 µs (9.5%)
profiling 1.485 ms [1.459 ms, 1.51 ms] 162.194 µs (12.3%)
tracing 1.449 ms [1.424 ms, 1.473 ms] 126.413 µs (9.6%)
Request duration reports for insecure-bank 
gantt
    title insecure-bank - request duration [CI 0.99] : candidate=1.22.0-SNAPSHOT~a07d37dc72, baseline=1.22.0-SNAPSHOT~626e570b5c
    dateFormat X
    axisFormat %s
section baseline
no_agent (362.386 µs) : 341, 384
.   : milestone, 362,
appsec (668.72 µs) : 648, 689
.   : milestone, 669,
iast (459.027 µs) : 438, 480
.   : milestone, 459,
iast_FULL (515.635 µs) : 495, 536
.   : milestone, 516,
iast_INACTIVE (434.163 µs) : 413, 455
.   : milestone, 434,
profiling (435.729 µs) : 414, 457
.   : milestone, 436,
tracing (430.175 µs) : 410, 450
.   : milestone, 430,
section candidate
no_agent (364.597 µs) : 345, 384
.   : milestone, 365,
appsec (678.895 µs) : 657, 700
.   : milestone, 679,
iast (456.81 µs) : 436, 478
.   : milestone, 457,
iast_FULL (528.021 µs) : 507, 549
.   : milestone, 528,
iast_INACTIVE (435.288 µs) : 414, 456
.   : milestone, 435,
profiling (433.149 µs) : 412, 454
.   : milestone, 433,
tracing (437.683 µs) : 417, 459
.   : milestone, 438,
Loading
  • baseline results
Variant Request duration [CI 0.99] Δ no_agent
no_agent 362.386 µs [340.852 µs, 383.92 µs] -
appsec 668.72 µs [648.425 µs, 689.015 µs] 306.334 µs (84.5%)
iast 459.027 µs [437.971 µs, 480.082 µs] 96.641 µs (26.7%)
iast_FULL 515.635 µs [495.139 µs, 536.131 µs] 153.249 µs (42.3%)
iast_INACTIVE 434.163 µs [412.944 µs, 455.382 µs] 71.777 µs (19.8%)
profiling 435.729 µs [414.331 µs, 457.127 µs] 73.343 µs (20.2%)
tracing 430.175 µs [409.858 µs, 450.492 µs] 67.789 µs (18.7%)
  • candidate results
Variant Request duration [CI 0.99] Δ no_agent
no_agent 364.597 µs [344.75 µs, 384.445 µs] -
appsec 678.895 µs [657.483 µs, 700.307 µs] 314.297 µs (86.2%)
iast 456.81 µs [435.979 µs, 477.641 µs] 92.213 µs (25.3%)
iast_FULL 528.021 µs [506.99 µs, 549.051 µs] 163.423 µs (44.8%)
iast_INACTIVE 435.288 µs [414.244 µs, 456.331 µs] 70.69 µs (19.4%)
profiling 433.149 µs [412.204 µs, 454.094 µs] 68.552 µs (18.8%)
tracing 437.683 µs [416.764 µs, 458.601 µs] 73.085 µs (20.0%)

@jpbempel
Prevent calling size method from unknown classes
a22c68d 
Collection and Map interfaces can be implemented by anyone and
therefore may not be safe to call. example: persistent layer that
load lazily large amount of rows from DB.
we are preventing to call on unsafe implementation. We are limiting
to JDK impleemntation first.
@jpbempel jpbempel force-pushed the jpbempel/limit-collection-size branch from 96360b6 to a22c68d Compare September 27, 2023 11:06
shatzi
shatzi approved these changes Sep 27, 2023
View reviewed changes
Copy link
Contributor

@shatzi shatzi left a comment

Choose a reason for hiding this comment

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

nice MVP. I like that we only capture java.*

dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/CapturedSnapshotTest.java
int result = Reflect.on(testClass).call("main", "").get();
Assertions.assertEquals(1, result);
Snapshot snapshot = assertOneSnapshot(listener);
assertEquals(1, snapshot.getEvaluationErrors().size());
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe also add something like this: assertEquals("len(holder)", snapshot.getEvaluationErrors().get(0).getExpr());

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@jpbempel jpbempel merged commit a61bbfd into master Sep 27, 2023
66 checks passed
@jpbempel jpbempel deleted the jpbempel/limit-collection-size branch September 27, 2023 21:26
@github-actions github-actions bot added this to the 1.22.0 milestone Sep 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shatzi shatzi shatzi approved these changes

@ojung ojung Awaiting requested review from ojung ojung is a code owner automatically assigned from DataDog/debugger-java

Assignees
No one assigned
Labels
comp: debugger Dynamic Instrumentation type: bug
Projects
None yet
Milestone
1.22.0
Development

Successfully merging this pull request may close these issues.
2 participants
@jpbempel @shatzi