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.

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

Fix PLT function call lookup #1001

Merged
merged 1 commit into from Feb 15, 2021
Merged

Fix PLT function call lookup #1001

merged 1 commit into from Feb 15, 2021

Conversation

mxz297
Copy link
Member

@mxz297 mxz297 commented Feb 9, 2021

BPatch creates additional SymtabAPI::Function objects for PLT stubs but does not put these SymtabAPI::Function objects back to Symtab objects. Later BPatch look up function callees through the Symtab object and cannot find PLT stubs.

This PR records functions created for PLT and fix the lookup.

@hainest hainest self-assigned this Feb 9, 2021
@hainest hainest added BPatch This issue is directly related to BPatch bug labels Feb 9, 2021
@hainest hainest added this to the Release 11.0 milestone Feb 9, 2021
@hainest
Copy link
Contributor

hainest commented Feb 10, 2021

https://bottle.cs.wisc.edu/search?dyninst_branch=PR1001

No new regressions. I'm still working on creating a test specifically for this. I'll let you know when I get that finished.

@hainest
Copy link
Contributor

hainest commented Feb 11, 2021

The new test (dyninst/testsuite#182) is working for dynamic linking, but fails for static linking. For some reason, the compiler isn't putting their name at the call site- just their offsets:

0000000000407ab4 <_Z22test1_36_indirect_callv>:
callq  430710 <__libc_malloc>
callq  4010e0 <.plt+0xc0>
callq  40b620 <toupper>
callq  4010e0 <.plt+0xc0>
callq  430d00 <__free>

dyninst_group_test.stat_g++_64_none_none.tar.gz

@mxz297
Copy link
Member Author

mxz297 commented Feb 12, 2021

@hainest Static linking definitely complicates the issue. The problem here is that with static linking, the compiler has the freedom to bring lots of inner complexity of glibc into the binary.

Theoretically, static linking should have everything inside the binary, so there should not even be any PLT calls. However, due to some complicated optimizations, such as choosing architecture specific implementation for a function, there are still PLT calls inside a statically linked binary. But, as everything is now inside the binary, a function symbol is no longer necessary totie the call site to the callee.

I feel checking PLT calls for statically linked binaries is not well defined. This check should be skipped for statically linked binaries.

@hainest
Copy link
Contributor

hainest commented Feb 12, 2021

@mxz297 That sounds very reasonable. I was a little surprised to see references to the PLT in a static binary. The complication here is that I'll need to make a new test for this since the test1_36 should be run on static binaries (it tests parameter passing for direct calls). However, no one actually knows how to do that, so I'll work with Bolo to try to figure it out.

@hainest
Copy link
Contributor

hainest commented Feb 12, 2021

#1001 is merged, and I'm not seeing any conflicts here. I'll go ahead and re-test.

@hainest
Copy link
Contributor

hainest commented Feb 15, 2021

https://bottle.cs.wisc.edu/search?dyninst_branch=PR1001&since_date=2021-02-14

No new regressions and the new tests are passing.

@hainest hainest merged commit ce2a3eb into master Feb 15, 2021
@mxz297 mxz297 deleted the plt_call_lookup branch February 15, 2021 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BPatch This issue is directly related to BPatch bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants