[PATCH] imap-send: link against libcrypto for HMAC and others

[PATCH] imap-send: link against libcrypto for HMAC and others

[Diego Elio Pettenò]

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(-)

diff --git a/Makefile b/Makefile
index 1f1ce04..c8377d4 100644
--- a/Makefile
+++ b/Makefile
@@ -1927,7 +1927,7 @@ git-%$X: %.o $(GITLIBS)
 
 git-imap-send$X: imap-send.o $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
-		$(LIBS) $(OPENSSL_LINK) $(OPENSSL_LIBSSL)
+		$(LIBS) $(OPENSSL_LINK) $(OPENSSL_LIBSSL) $(LIB_4_CRYPTO)
 
 git-http-fetch$X: revision.o http.o http-walker.o http-fetch.o $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
-- 
1.7.3.2

Re: [PATCH] imap-send: link against libcrypto for HMAC and others

[Junio C Hamano]

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

> 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.
> ---

Thanks.  Sign-off, please.

>  Makefile |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 1f1ce04..c8377d4 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1927,7 +1927,7 @@ git-%$X: %.o $(GITLIBS)
>  
>  git-imap-send$X: imap-send.o $(GITLIBS)
>  	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
> -		$(LIBS) $(OPENSSL_LINK) $(OPENSSL_LIBSSL)
> +		$(LIBS) $(OPENSSL_LINK) $(OPENSSL_LIBSSL) $(LIB_4_CRYPTO)
>  
>  git-http-fetch$X: revision.o http.o http-walker.o http-fetch.o $(GITLIBS)
>  	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
> -- 
> 1.7.3.2

[PATCH] imap-send: link against libcrypto for HMAC and others

[Diego Elio Pettenò]

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(-)

diff --git a/Makefile b/Makefile
index 1f1ce04..c8377d4 100644
--- a/Makefile
+++ b/Makefile
@@ -1927,7 +1927,7 @@ git-%$X: %.o $(GITLIBS)
 
 git-imap-send$X: imap-send.o $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
-		$(LIBS) $(OPENSSL_LINK) $(OPENSSL_LIBSSL)
+		$(LIBS) $(OPENSSL_LINK) $(OPENSSL_LIBSSL) $(LIB_4_CRYPTO)
 
 git-http-fetch$X: revision.o http.o http-walker.o http-fetch.o $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
-- 
1.7.3.2

Re: [PATCH] imap-send: link against libcrypto for HMAC and others

[Anders Kaseorg]

On Wed, 2010-11-24 at 21:03 +0100, Diego Elio Pettenò wrote:
> 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.
> […]
>  git-imap-send$X: imap-send.o $(GITLIBS)
>  	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
> -		$(LIBS) $(OPENSSL_LINK) $(OPENSSL_LIBSSL)
> +		$(LIBS) $(OPENSSL_LINK) $(OPENSSL_LIBSSL) $(LIB_4_CRYPTO)

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

Re: [PATCH] imap-send: link against libcrypto for HMAC and others

[Diego Elio Pettenò]

Il giorno lun, 06/12/2010 alle 00.28 -0500, Anders Kaseorg ha scritto:
> This broke the build with NO_OPENSSL=1, so Debian will need to revert
> it:

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

> 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?

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ò — “Flameeyes”
http://blog.flameeyes.eu/

If you found a .asc file in this mail and know not what it is,
it's a GnuPG digital signature: http://www.gnupg.org/

Re: [PATCH] imap-send: link against libcrypto for HMAC and others

[Jonathan Nieder]

Diego Elio Pettenò wrote:
> Il giorno lun, 06/12/2010 alle 00.28 -0500, Anders Kaseorg ha scritto:

>> 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?
>
> No, these should be different issues

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.

Re: [PATCH] imap-send: link against libcrypto for HMAC and others

[Junio C Hamano]

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

> Il giorno lun, 06/12/2010 alle 00.28 -0500, Anders Kaseorg ha scritto:
>> This broke the build with NO_OPENSSL=1, so Debian will need to revert
>> it:
>
> I'll try a NO_OPENSSL build later on today and see to get it fixed.
>
>> 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?
>
> 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.

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(-)

diff --git a/Makefile b/Makefile
index 7a5fb69..b20ae1a 100644
--- a/Makefile
+++ b/Makefile
@@ -1296,11 +1296,15 @@ else
 	BLK_SHA1 = 1
 	OPENSSL_LIBSSL =
 endif
+ifdef NO_OPENSSL
+	LIB_4_CRYPTO =
+else
 ifdef NEEDS_SSL_WITH_CRYPTO
 	LIB_4_CRYPTO = $(OPENSSL_LINK) -lcrypto -lssl
 else
 	LIB_4_CRYPTO = $(OPENSSL_LINK) -lcrypto
 endif
+endif
 ifdef NEEDS_LIBICONV
 	ifdef ICONVDIR
 		BASIC_CFLAGS += -I$(ICONVDIR)/include

Re: [PATCH] imap-send: link against libcrypto for HMAC and others

[Jared Hance]

> 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?

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.

Re: [PATCH] imap-send: link against libcrypto for HMAC and others

[Junio C Hamano]

Jared Hance <jaredhance@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?
>
> 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.

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.

Re: [PATCH] imap-send: link against libcrypto for HMAC and others

[Anders Kaseorg]

On Sun, 5 Dec 2010, Junio C Hamano wrote:
> Anders, wouldn't this be a better fix for NO_OPENSSL build, than reverting
> a fix for an incorrect ld invocation?

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