Page MenuHomeFreeBSD

Avoid symbol clashes between libmd and libcrypto
ClosedPublic

Authored by thomas on Apr 3 2015, 8:44 AM.

Details

Summary

This change ensures that libmd does not define any symbol that is also present in libcrypto, providing a fix for PR 199119.

Test Plan

Tested by rebuilding and reinstalling libmd, rebuilding and reinstalling crontab(1), and observing that it did not hang anymore in a context where nss_ldap is used and connects to an LDAP server using SSL.

Diff Detail

Repository
rS FreeBSD src repository
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

thomas retitled this revision from to Avoid symbol clashes between libmd and libcrypto.Apr 3 2015, 8:44 AM
thomas updated this object.
thomas edited the test plan for this revision. (Show Details)
thomas updated this revision to Diff 4628.
thomas updated this object.Apr 3 2015, 9:14 AM
roberto edited edge metadata.Apr 3 2015, 9:50 AM
roberto accepted this revision.

Looks reasonable to me.

This revision is now accepted and ready to land.Apr 3 2015, 9:50 AM
cperciva edited edge metadata.Apr 3 2015, 6:44 PM

Looks reasonable to me; I use the same approach in the hash code I have in libcperciva. Is there a reason you're not doing this to md4 and md5 as well though?

thomas added a comment.Apr 4 2015, 6:43 AM

I deliberately prefixed only those specific symbols that do clash with libcrypto. But indeed we could just as well consistently prefix all symbols in libmd.

thomas edited edge metadata.Apr 4 2015, 7:19 AM
thomas updated this revision to Diff 4652.

Alternate version prefixing all symbols in libmd.

This revision now requires review to proceed.Apr 4 2015, 7:19 AM
bapt added a reviewer: bapt.Apr 4 2015, 12:47 PM
bapt accepted this revision.
bapt added a subscriber: bapt.

I was exactly looking into doing that ! that will allow to modify things in base to always only depend on libmd like libarchive (making it easier have the ports tree use only openssl from ports and/or libressl from ports)!

This revision is now accepted and ready to land.Apr 4 2015, 12:47 PM
thomas added a comment.Apr 9 2015, 9:03 PM

@cperciva @roberto would you mind commenting on the second version (updated to uniformly prefix all symbols in libmd, rather than just those that actually, currently conflict with libcrypto)?

Thanks!
Thomas.

roberto edited edge metadata.Apr 10 2015, 1:30 PM
roberto accepted this revision.

Still OK with it, thanks.

delphij accepted this revision.
delphij added a subscriber: delphij.

Hi,

I got bite by this and used rS281372 to workaround it. Are you going to commit this anytime soon?

A second thought -- I wonder if we could use versioned symbols to achieve the same goal?

delphij resigned from this revision.

Not entirely clear to me how symbol versioning would help here. libmd provides just one version of these symbols, and we need to prevent any reference to the same-named symbols in libcrypto from getting (erroneously) resolved to the libmd version.

bapt added a comment.Apr 10 2015, 8:15 PM

Version symbols will only help if openssl also have version symbol

delphij added a subscriber: ume.Apr 10 2015, 8:40 PM
In D2216#29, @thomas wrote:

Not entirely clear to me how symbol versioning would help here. libmd provides just one version of these symbols, and we need to prevent any reference to the same-named symbols in libcrypto from getting (erroneously) resolved to the libmd version.

I think @bapt was right that symbol versioning will not help (and it does not solve the conflict issue anyway).

BTW. @ume have pointed D1542 to me which uses __weak_reference to create compatibility symbols that would only be used when libcrypto is not present, could you please take a look at that changeset and see if it's sensible to adopt the same technique in this changeset? That would avoid the shared library bump and make the change MFC'able.

Indeed, the weak aliases make sense, and avoid the need to bump the major. I'll produce an update patch. Thanks!

ume added a comment.Apr 11 2015, 2:55 PM
In D2216#33, @thomas wrote:

Indeed, the weak aliases make sense, and avoid the need to bump the major. I'll produce an update patch. Thanks!

I believe the patch in D1542 should solve your problem. Please try it.

thomas edited edge metadata.Apr 22 2015, 1:39 PM
thomas updated this revision to Diff 4949.

New version introducing weak aliases

This is to avoid having to rebuild other binaries, or bump the
shared library major version. Succesfully tested against the
original issue as described in PR 199119 (crontab(1) hanging
when using LDAP with SSL for the password database).

This revision now requires review to proceed.Apr 22 2015, 1:39 PM
roberto edited edge metadata.Apr 22 2015, 1:51 PM
roberto accepted this revision.

Ok for me.

This revision is now accepted and ready to land.Apr 22 2015, 1:51 PM
delphij accepted this revision.

Looks good to me, thanks for working on this!

thomas added a comment.May 4 2015, 7:25 AM

Unless further discussion takes place, I intend to commit the last version of this change at the end of the week.

thomas closed this revision.May 10 2015, 1:23 PM

Committed to head.