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

i#3315: avoid conflicts with memset and memcpy #3374

Merged
merged 1 commit into from
Feb 12, 2019

Conversation

derekbruening
Copy link
Contributor

Core DR's copies of memset and memcpy were placed into drhelper to
avoid glibc-versioned symbols in our shipped binaries (#1504).
They are linked into the static drfrontendlib and drinjectlib
libraries. They are also in core DR when built as a static library.
In these static library cases they can conflict with symbols of the
same name elsewhere in the linking application.

To solve this, we first separate memset and memcpy from drhelper into
their own library, drmemfuncs. We simply avoid linking it into static
core DR, and into drdecode. For static core DR we assume that
whatever versions the application or glibc provides will be
re-entrant.

We can't avoid linking it into drfrontendlib and drinjectlib (we hit
the glibc versioning problem), but we reduce the chance of conflicts
with these libraries by marking memset and memcpy as weak.

Fixes #3315

Core DR's copies of memset and memcpy were placed into drhelper to
avoid glibc-versioned symbols in our shipped binaries (#1504).
They are linked into the static drfrontendlib and drinjectlib
libraries.  They are also in core DR when built as a static library.
In these static library cases they can conflict with symbols of the
same name elsewhere in the linking application.

To solve this, we first separate memset and memcpy from drhelper into
their own library, drmemfuncs.  We simply avoid linking it into static
core DR, and into drdecode.  For static core DR we assume that
whatever versions the application or glibc provides will be
re-entrant.

We can't avoid linking it into drfrontendlib and drinjectlib (we hit
the glibc versioning problem), but we reduce the chance of conflicts
with these libraries by marking memset and memcpy as weak.

Fixes #3315
@derekbruening derekbruening merged commit da98a60 into master Feb 12, 2019
@derekbruening derekbruening deleted the i3315-shared-memset branch February 12, 2019 04:12
* those definitions and intercept them. In particular, this occurs when
* running an app that links in the Address Sanitizer runtime. Since we already
* need a reasonably efficient assembly memcpy implementation for safe_read, we
* go ahead and reuse the code for private memcpy and memset.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see an explicit reference to safe_read in here, but im guessing the comment is referring to these macros which might be shared? Do you mind clearing up exactly what parts or shared, or fixing this comment up if the following functions don't actually reuse stuff which is specifically for safe_read?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

safe_read_asm shares the REP_STRING_OP macro which is also used here.

This comment does bring up something I did not make clear in the issue: there is a downside to having static DR use glibc memcpy. It lets sanitizers or other interceptors override
DR's memcpy. However, we already have interoperability concerns with sanitizers, so I was assuming we could live with that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want to also add the sanitizer concern to the code comments and not just the issue so I can send you a comment update PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PR #3376

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.

2 participants