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

Don't turn undefined symbols into an error #8740

Closed
wants to merge 1 commit into from

Conversation

kroeckx
Copy link
Member

@kroeckx kroeckx commented Apr 13, 2019

Clang does not seem to link to libasan when creating shared libraries.

Fixes: #8735

[extended tests]

@kroeckx
Copy link
Member Author

kroeckx commented Apr 13, 2019

So it seems #8537 introduces that -z defs things, and travis has been failing since.

@kroeckx
Copy link
Member Author

kroeckx commented Apr 14, 2019

At least for the msan case it seems we can't resolve this. It does not have a shared library we can link to, and the static library is not build using -fPIC and the name of that library depends on the architecture.

…r .so file"

This reverts commit 0be2cc5.

This breaks all builds using a clang sanitizer.

Fixes: openssl#8735

[extended tests]
@kroeckx kroeckx changed the title Add -lasan to the libraries when using clang Don't set turn undefined symbols into an error Apr 14, 2019
@kroeckx kroeckx changed the title Don't set turn undefined symbols into an error Don't turn undefined symbols into an error Apr 14, 2019
@levitte
Copy link
Member

levitte commented Apr 14, 2019

Hmm, what's up with Travis?

@levitte
Copy link
Member

levitte commented Apr 14, 2019

Also, how did the asan and msan stuff work before we ensured modules wouldn't be built without unresolved symbols?

@kroeckx
Copy link
Member Author

kroeckx commented Apr 14, 2019

The tests failing on travis already existed, this PR doesn't change that. Note that I've enabled extended tests. This fixes 3 of the 5 broken builds.

asan, ubsan and msan work just the same way as before we used that -z defs, by linking their library in the executable, not the shared library.

I'm not sure why this z defs was added. Did the provider pick up symbols from libcrypto.so, or did it also fail just at some later place?

@levitte
Copy link
Member

levitte commented Apr 14, 2019

asan, ubsan and msan work just the same way as before we used that -z defs, by linking their library in the executable, not the shared library.

Then it sounds like the problem lies there, that our shared libraries should be linked with the *san stuff when that's used.

I'm not sure why this z defs was added. Did the provider pick up symbols from libcrypto.so, or did it also fail just at some later place?

Most importantly, the FIPS provider module shouldn't pick up stuff that the application is linked with by accident. That completely defeats the purpose of it being self contained. We extended that to all modules, with the thought that for any external symbol, they should be explicitly linked with the needed libraries, not piggy-back on whatever the application is linked with (which is a recipe for failure anyway, since you can't be sure that the application is linked with all the module needs)

@kroeckx
Copy link
Member Author

kroeckx commented Apr 14, 2019 via email

@levitte
Copy link
Member

levitte commented Apr 14, 2019

https://clang.llvm.org/docs/MemorySanitizer.html confirms that one should not combine -z defs with msan. So that essentially means that we should not use that flag specifically when msan is enabled. Removing -z defs unconditionally is counter-productive, though.

@kroeckx
Copy link
Member Author

kroeckx commented Apr 14, 2019 via email

@levitte
Copy link
Member

levitte commented Apr 14, 2019

I will look tomorrow. Way too late tonight

@kroeckx kroeckx closed this Apr 15, 2019
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.

FIPS module in build with -z defs
3 participants