patchimap-send: link against libcrypto for HMAC and others

When using stricter linkers, such as GNU gold or Darwin ld, transitive
dependencies are not counted towards symbol resolution. If we don't link
imap-send to libcrypto, we'll have undefined references to the HMAC_*,
EVP_* and ERR_* functions families.
---
 Makefile |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Diego Elio Pettenò <flameeyes@gmail.com> writes:

Thanks.  Sign-off, please.

When using stricter linkers, such as GNU gold or Darwin ld, transitive dependencies are not counted towards symbol resolution. If we don't link imap-send to libcrypto, we'll have undefined references to the HMAC_*, EVP_* and ERR_* functions families.

Signed-off-by: Diego Elio Pettenò <flameeyes@gmail.com>
---
 Makefile |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

On Wed, 2010-11-24 at 21:03 +0100, Diego Elio Pettenò wrote:

This broke the build with NO_OPENSSL=1, so Debian will need to revert it:

    CC imap-send.o
    LINK git-imap-send
/usr/bin/ld: cannot find -lcrypto
collect2: ld returned 1 exit status
make[1]: *** [git-imap-send] Error 1

Also, the Makefile already has a NEEDS_CRYPTO_WITH_SSL flag that’s automatically set on Darwin, Windows, and MinGW. We shouldn’t have two mechanisms for addressing the same problem; maybe we just need to enable the existing flag on more (or all) platforms?

Anders

Il giorno lun, 06/12/2010 alle 00.28 -0500, Anders Kaseorg ha scritto:

I'll try a NO_OPENSSL build later on today and see to get it fixed.

No, these should be different issues; you may have a libssl (which uses libcrypto) requiring libcrypto to be linked in, even if you only use interfaces from libssl (and that's what NEEDS_CRYPTO_WITH_SSL seem to be designed to deal with), but in this case what you have is rather imap-send using the libcrypto interfaces _as well as_ the libssl interfaces.

I have blogged a detailed analysis of the problem, if you wish to see the details:

http://blog.flameeyes.eu/2010/11/26/it-s-not-all-gold-that-shines-why-underlinking-is-a-bad-thing

Diego Elio Pettenò wrote:

Are you sure? The change description for v1.6.5-rc1~23 (Makefile: Add NEEDS_CRYPTO_WITH_SSL, 2009-10-08) says:

    The Makefile comment for NEEDS_SSL_WITH_CRYPTO says to define it "if
    you need -lcrypto with -lssl (Darwin)."  However, what it actually
    does is add -lssl when you use -lcrypto and not the other way around.
    However, libcrypto contains a majority of the ERR_* functions from
    OpenSSL (at least on OS X) so we need it both ways.

and grepping that version reveals the same hit I think your patch deals with:

 $ git grep -F -e ERR_ v1.6.5-rc1~23
 v1.6.5-rc1~23:imap-send.c:      fprintf(stderr, "%s: %s\n", func, ERR_error_string(ERR_get_error(), NULL));

I am guessing the reality is closer to:

    Previously we relied on -lssl pulling in libcrypto on most
    platforms.  That has at least three problems:

     (1) it is not resiliant against future changes in libssl.
         The DT_NEEDED entries in a library are generally
         considered an implementation detail that is allowed to
         change.

     (2) it does not work on all platforms; for example, ld on
         Mac OS X and ld --no-copy-dt-needed-entries on Linux
         do not permit that trick.

     (3) it is tricky.  A simple rule is "explicitly link to
         all libraries whos symbols you use directly".

    To fix this, eliminate the NEEDS_CRYPTO_WITH_SSL knob and
    always treat it as true.  NEEDS_SSL_WITH_CRYPTO can stay
    because there really are differences between platforms
    for that one.

Warning: I could be totally wrong. :)  I have not verified any
of these claims.

Diego Elio Pettenò <flameeyes@gmail.com> writes:

You are both correct; the point of NO_OPENSSL is not to link with anything from openssl suite, so we need a separate mechanism to address this.

Anders, wouldn't this be a better fix for NO_OPENSSL build, than reverting a fix for an incorrect ld invocation?

 Makefile |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

Could we get this fixup patch into master? Leaving the original patch in without it doesn't seem like a good idea when it breaks the build.

Jared Hance <jaredhance@gmail.com> writes:

That depends on what Diego and Anders would say/report, and that is why their address were on To/Cc in the message you are replying to. Why did you cull Cc from your message?

One thing I do not like about it is that the "fixup" is ugly. It makes people expect to add $(LIB_4_CRYPTO) on their linker command line to get a correct linking with -lcrypto library, when all it does is _not_ to link with -lcrypto at all, so whatever program that uses it needs to know about NO_OPENSSL and refrain from using the symbols from that library.

On Sun, 5 Dec 2010, Junio C Hamano wrote:

It works for me. I agree it isn’t beautiful but I don’t think I have a better idea at this time.

(It’s worth pointing out that $(OPENSSL_LINK) is duplicated in the git-imap-send build command, once directly and once indirectly via $(LIB_4_CRYPTO), but I assume this is intentional for clarity.)

Anders