Redact object data in heap snapshots, with option to opt-out #55326
Conversation
|
Seems like a good default, but might be worth putting behind a switch as the string content might be helpful in understanding what it is? |
|
Right. I agree with Ian. And i've used the string content before to try and localize where it came from. Maybe we add a |
|
we could go further then this and zero out all bitstypes |
|
How do folks feel about adding a global option into |
|
I'd want the ability to decide to not redact when collecting a heap snapshot within a session, not require restarting. |
|
yeah, +1 to ian's comment. i think this should go in the API request, rather than in the whole runtime. I'll leave a comment in the code |
@oscardssmith: Agreed. If there are any other sources of data in the snapshot, then yeah we should not emit them. However, when I made the same point internally, the team pointed out that strings are the only primitive data values we actually copy out from memory into the snapshot. For everything else, we only copy object sizes, types, and their outbound pointers. So there shouldn't be any other isbits data in these snapshots as far as I know. @IanButterworth and @gbaraldi can you confirm that? |
|
Thanks for the PR, Kiran! :) |
kpamnany
force-pushed
the
kp/safe-heap-snapshot
branch
from
bc88c63
to
087705c
Compare
|
Please take another look @IanButterworth, @gbaraldi and @oscardssmith? |
kpamnany
changed the title
IanButterworth
added
needs tests
| sshot = read(fname, String) | ||
| @test sshot != "" | ||
| @test !contains(sshot, "redact_this") |
There was a problem hiding this comment.
Can you please add another test that if you set redact_data=false, it does have the string data? 🙏
|
Just needs a news entry. |
IanButterworth
changed the title
kpamnany
force-pushed
the
kp/safe-heap-snapshot
branch
from
c9accd6
to
7bac5e0
Compare
IanButterworth
added
status:merge me
Currently, this only applies to strings, which are the only type of object for which we emit content in the heap snapshot.
Co-authored-by: Nathan Daly <[email protected]>
kpamnany
force-pushed
the
kp/safe-heap-snapshot
branch
from
c112fcb
to
cd02ce8
Compare
|
Not sure what's up with CI. Hope you can get this merged Ian! |
DilumAluthge
removed
the
status:merge me
…ng#55326) The contents of strings can contain user data which may be proprietary and emitting them in the heap snapshot makes the heap snapshot a potential vulnerability rather than a useful debugging artifact. There are likely other tweaks necessary to make heap snapshots "safe", but this is one less. --------- Co-authored-by: Nathan Daly <[email protected]> Co-authored-by: Ian Butterworth <[email protected]>
…ng#55326) (#174) The contents of strings can contain user data which may be proprietary and emitting them in the heap snapshot makes the heap snapshot a potential vulnerability rather than a useful debugging artifact. There are likely other tweaks necessary to make heap snapshots "safe", but this is one less. --------- Co-authored-by: Nathan Daly <[email protected]> Co-authored-by: Ian Butterworth <[email protected]>
The contents of strings can contain user data which may be proprietary and emitting them in the heap snapshot makes the heap snapshot a potential vulnerability rather than a useful debugging artifact. There are likely other tweaks necessary to make heap snapshots "safe", but this is one less. --------- Co-authored-by: Nathan Daly <[email protected]> Co-authored-by: Ian Butterworth <[email protected]>
The contents of strings can contain user data which may be proprietary and emitting them in the heap snapshot makes the heap snapshot a potential vulnerability rather than a useful debugging artifact.
There are likely other tweaks necessary to make heap snapshots "safe", but this is one less.