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.
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
add hooks to debug OpenSSL memory #101626
add hooks to debug OpenSSL memory #101626
Changes from 17 commits
a163dbf
9b5b3d3
ea6ac3f
0338b06
8744c2d
401178f
803fc94
cd55d63
638df97
dd9b017
d42f271
01f556f
173827a
a86c713
c0838ea
60e933e
7c03ee3
1fe580f
c0e1a98
2fc9779
9c3da4f
ac41d7f
7f0885b
67d70ba
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understood the original comment from @jkotas, then this is still a potential problem
#101626 (comment)
Or does that hold only for the malloc/free calls and not GC-allocated memory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not safe to use any managed code if this can be called from places like thread destructor. #101626 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right. I split the change now into two parts. The basic counters are implemented in native as @janvorli suggested. That also eliminates need to fiddle with the crypto initialization.
Now for the managed part. I made this whole section
#if DEBUG
for now to limit the exposure. While this limits use in production it would allow us to experiment more and perhaps hook it to test runs. I'm yet to see case where it actually fails. Since this can be set during run for some particular operation(s) it may avoid the cases we are concern about e.g. threads operations. AFAIK there is API to get loaded providers so we may for example check FIPS or 3rd party modules.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original PR had
IntPtr
, is there some reason to useUIntPtr
overIntPtr
here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the field is not used anywhere, can we move the call to
GetMemoryDebug
to cctor?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should also be moved to Interop.Crypto, because following code
Will enable tracking, but later when Interop.OpenSsl gets initialized, it turns the tracking of because of
in
GetMemoryDebug()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the comment. Based on the feedback I specifically removed the option to enable the detailed tracking via environment to avoid cases when managed code may be invoked on incompatible thread. What remains is ability to subscribe for the detailed reporting later (when all the initialization is done) when caller feels it is safe. I know this part may be tricky to describe. But so far I failed to construct case where it failed e.g. all the cases I was interested in so far just worked and provided the info I was looking for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following program will not print any memory addresses, although one would expect that it should
It starts to work if you uncomment the first two lines.