Skip to content

Protect patching with an open handle - #1264

Merged
apangin merged 100 commits into
async-profiler:masterfrom
fandreuz:sim-294-keep-handle-while-patching
May 20, 2025
Merged

apangin merged 100 commits into
async-profiler:masterfrom
fandreuz:sim-294-keep-handle-while-patching

Conversation

@fandreuz

Copy link
Copy Markdown
Contributor

Description

In this PR I introduce additional protection during patches. The main change is a new function isSafeToPatch, which combines the checks introduced in #1261, #1263, #1243, and can be used in other parts of the code like MallocTracer.

Thanks @Baraa-Hasheesh for the discussion and feedback on the code.

Related issues

#1250, #1256

Motivation and context

Keeping an open handle makes sure we the library cannot be unloaded while the patch is being made, thus providing additional protection against the failures in the linked issues.

How has this been tested?

make test


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@fandreuz

Copy link
Copy Markdown
Contributor Author

Tested that after this PR the test in asprof-reproducers does not fail anymore, and fails in master.

@fandreuz

Copy link
Copy Markdown
Contributor Author

Also tested the crash in the stress test by @Baraa-Hasheesh, and could not reproduce the issue anymore after this PR

Comment thread src/symbols.h Outdated
Co-authored-by: Bara' Hasheesh <bara.hasheesh@gmail.com>
Signed-off-by: Francesco Andreuzzi <andreuzzi.francesco@gmail.com>
@krk

krk commented Apr 25, 2025

Copy link
Copy Markdown
Contributor

Let's add minimal reproducers as cpp unit tests.

Comment thread src/codeCache.h Outdated
Comment thread src/symbols.h Outdated
Comment thread src/codeCache.h Outdated
@fandreuz

fandreuz commented Apr 25, 2025 •

Copy link
Copy Markdown
Contributor Author

Let's add minimal reproducers as cpp unit tests.

@krk Adapted test from @Baraa-Hasheesh's repo

@fandreuz
fandreuz force-pushed the sim-294-keep-handle-while-patching branch from 10c0a73 to 42879e3 Compare May 14, 2025 16:16
@fandreuz
fandreuz force-pushed the sim-294-keep-handle-while-patching branch from 1258b41 to ff43185 Compare May 15, 2025 14:06
@Baraa-Hasheesh Baraa-Hasheesh mentioned this pull request May 16, 2025
Comment thread .github/workflows/test-and-publish-nightly.yml Outdated
Comment thread src/symbols_linux.cpp
Comment thread src/symbols_linux.cpp Outdated
Comment thread test/test/nativemem/nativemem_known_lib_crash.c Outdated
Comment thread test/test/nativemem/nativemem_known_lib_crash.c Outdated
Comment thread test/test/nativemem/nativemem_known_lib_crash.c Outdated
Comment thread src/symbols_linux.cpp Outdated
Comment thread src/symbols_linux.cpp Outdated
// Protect library from unloading while parsing in-memory ELF program headers.
// Also, dlopen() ensures the library is fully loaded.
_lib_handle = dlopen(stripped_name, RTLD_LAZY | RTLD_NOLOAD);
_valid = isValidHandle(cc, _lib_handle);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not the best name, IMO.
Any non-null handle is valid, but what we really check here is whether it is the same shared library that was seen at CodeCache creation time.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

  • checkHandleConsistency
  • isHandleConsistent

I'll think about it, let me know if you have any preference

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Comment thread src/symbols_linux.cpp Outdated
@fandreuz

Copy link
Copy Markdown
Contributor Author

@apangin all comments addressed

@apangin
apangin merged commit c17de4c into async-profiler:master May 20, 2025
@apangin

apangin commented May 20, 2025

Copy link
Copy Markdown
Member

100 commits, 140 comments, 5 contributors. That was an epic PR. Thanks everyone involved!

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