# [PATCH] imap-send: link against libcrypto for HMAC and others 10 messages from 2010-11-24 to 2010-12-08. Participants: Diego Elio Pettenò, Junio C Hamano, Anders Kaseorg, Jonathan Nieder, Jared Hance. Thread: https://gitlist.dev/t/25831 ## Diego Elio Pettenò, 2010-11-24 13:55 Subject: [PATCH] imap-send: link against libcrypto for HMAC and others Message-ID: <1290606916-14550-1-git-send-email-flameeyes@gmail.com> URL: https://gitlist.dev/e/1290606916-14550-1-git-send-email-flameeyes%40gmail.com ``` 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 ``` ## Junio C Hamano, 2010-11-24 20:01 Subject: Re: [PATCH] imap-send: link against libcrypto for HMAC and others Message-ID: <7vwro2sd83.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vwro2sd83.fsf%40alter.siamese.dyndns.org In-Reply-To: <1290606916-14550-1-git-send-email-flameeyes@gmail.com> ``` Diego Elio Pettenò 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 ``` ## Diego Elio Pettenò, 2010-11-24 20:03 Subject: [PATCH] imap-send: link against libcrypto for HMAC and others Message-ID: <1290629033-20566-1-git-send-email-flameeyes@gmail.com> URL: https://gitlist.dev/e/1290629033-20566-1-git-send-email-flameeyes%40gmail.com In-Reply-To: <7vwro2sd83.fsf@alter.siamese.dyndns.org> ``` 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ò --- 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 ``` ## Anders Kaseorg, 2010-12-06 05:28 Subject: Re: [PATCH] imap-send: link against libcrypto for HMAC and others Message-ID: <1291613304.3339.12.camel@fixed-disk> URL: https://gitlist.dev/e/1291613304.3339.12.camel%40fixed-disk In-Reply-To: <1290629033-20566-1-git-send-email-flameeyes@gmail.com> ``` 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 ``` ## Diego Elio Pettenò, 2010-12-06 05:39 Subject: Re: [PATCH] imap-send: link against libcrypto for HMAC and others Message-ID: <1291613978.4756.157.camel@yamato.local> URL: https://gitlist.dev/e/1291613978.4756.157.camel%40yamato.local In-Reply-To: <1291613304.3339.12.camel@fixed-disk> ``` 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/ ``` ## Jonathan Nieder, 2010-12-06 06:06 Subject: Re: [PATCH] imap-send: link against libcrypto for HMAC and others Message-ID: <20101206060628.GA30188@burratino> URL: https://gitlist.dev/e/20101206060628.GA30188%40burratino In-Reply-To: <1291613978.4756.157.camel@yamato.local> ``` 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. ``` ## Junio C Hamano, 2010-12-06 06:11 Subject: Re: [PATCH] imap-send: link against libcrypto for HMAC and others Message-ID: <7v39qbtoq9.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7v39qbtoq9.fsf%40alter.siamese.dyndns.org In-Reply-To: <1291613978.4756.157.camel@yamato.local> ``` Diego Elio Pettenò 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 ``` ## Jared Hance, 2010-12-08 01:51 Subject: Re: [PATCH] imap-send: link against libcrypto for HMAC and others Message-ID: <20101208015144.GA4868@localhost.localdomain> URL: https://gitlist.dev/e/20101208015144.GA4868%40localhost.localdomain In-Reply-To: <7v39qbtoq9.fsf@alter.siamese.dyndns.org> ``` > 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. ``` ## Junio C Hamano, 2010-12-08 02:12 Subject: Re: [PATCH] imap-send: link against libcrypto for HMAC and others Message-ID: <7vd3pdm2rz.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vd3pdm2rz.fsf%40alter.siamese.dyndns.org In-Reply-To: <20101208015144.GA4868@localhost.localdomain> ``` Jared Hance 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. ``` ## Anders Kaseorg, 2010-12-08 04:53 Subject: Re: [PATCH] imap-send: link against libcrypto for HMAC and others Message-ID: URL: https://gitlist.dev/e/alpine.DEB.2.02.1012072347560.6372%40dr-wily.mit.edu In-Reply-To: <7v39qbtoq9.fsf@alter.siamese.dyndns.org> ``` 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 ```