{"thread":{"id":"25831","subject":"[PATCH] imap-send: link against libcrypto for HMAC and others","startedAt":"2010-11-24T13:55:16Z","lastAt":"2010-12-08T04:53:40Z","messageCount":10,"participants":["Diego Elio Pettenò","Junio C Hamano","Anders Kaseorg","Jonathan Nieder","Jared Hance"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"156482","messageId":"1290606916-14550-1-git-send-email-flameeyes@gmail.com","threadId":"25831","inReplyTo":null,"subject":"[PATCH] imap-send: link against libcrypto for HMAC and others","fromName":"Diego Elio Pettenò","fromEmail":"flameeyes@gmail.com","sentAt":"2010-11-24T13:55:16Z","receivedAt":"2010-11-24T13:55:16Z","isPatch":true,"sender":{"key":"flameeyes@gmail.com","avatar":"https://avatars.githubusercontent.com/u/74834?v=4"},"body":"When using stricter linkers, such as GNU gold or Darwin ld, transitive\ndependencies are not counted towards symbol resolution. If we don't link\nimap-send to libcrypto, we'll have undefined references to the HMAC_*,\nEVP_* and ERR_* functions families.\n---\n Makefile |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 1f1ce04..c8377d4 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1927,7 +1927,7 @@ git-%$X: %.o $(GITLIBS)\n \n git-imap-send$X: imap-send.o $(GITLIBS)\n \t$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \\\n-\t\t$(LIBS) $(OPENSSL_LINK) $(OPENSSL_LIBSSL)\n+\t\t$(LIBS) $(OPENSSL_LINK) $(OPENSSL_LIBSSL) $(LIB_4_CRYPTO)\n \n git-http-fetch$X: revision.o http.o http-walker.o http-fetch.o $(GITLIBS)\n \t$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \\\n-- \n1.7.3.2\n"},{"id":"156494","messageId":"7vwro2sd83.fsf@alter.siamese.dyndns.org","threadId":"25831","inReplyTo":"1290606916-14550-1-git-send-email-flameeyes@gmail.com","subject":"Re: [PATCH] imap-send: link against libcrypto for HMAC and others","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-11-24T20:01:16Z","receivedAt":"2010-11-24T20:01:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Diego Elio Pettenò <flameeyes@gmail.com> writes:\n\n> When using stricter linkers, such as GNU gold or Darwin ld, transitive\n> dependencies are not counted towards symbol resolution. If we don't link\n> imap-send to libcrypto, we'll have undefined references to the HMAC_*,\n> EVP_* and ERR_* functions families.\n> ---\n\nThanks.  Sign-off, please.\n\n>  Makefile |    2 +-\n>  1 files changed, 1 insertions(+), 1 deletions(-)\n>\n> diff --git a/Makefile b/Makefile\n> index 1f1ce04..c8377d4 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -1927,7 +1927,7 @@ git-%$X: %.o $(GITLIBS)\n>  \n>  git-imap-send$X: imap-send.o $(GITLIBS)\n>  \t$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \\\n> -\t\t$(LIBS) $(OPENSSL_LINK) $(OPENSSL_LIBSSL)\n> +\t\t$(LIBS) $(OPENSSL_LINK) $(OPENSSL_LIBSSL) $(LIB_4_CRYPTO)\n>  \n>  git-http-fetch$X: revision.o http.o http-walker.o http-fetch.o $(GITLIBS)\n>  \t$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \\\n> -- \n> 1.7.3.2\n"},{"id":"156495","messageId":"1290629033-20566-1-git-send-email-flameeyes@gmail.com","threadId":"25831","inReplyTo":"7vwro2sd83.fsf@alter.siamese.dyndns.org","subject":"[PATCH] imap-send: link against libcrypto for HMAC and others","fromName":"Diego Elio Pettenò","fromEmail":"flameeyes@gmail.com","sentAt":"2010-11-24T20:03:53Z","receivedAt":"2010-11-24T20:03:53Z","isPatch":true,"sender":{"key":"flameeyes@gmail.com","avatar":"https://avatars.githubusercontent.com/u/74834?v=4"},"body":"When using stricter linkers, such as GNU gold or Darwin ld, transitive\ndependencies are not counted towards symbol resolution. If we don't link\nimap-send to libcrypto, we'll have undefined references to the HMAC_*,\nEVP_* and ERR_* functions families.\n\nSigned-off-by: Diego Elio Pettenò <flameeyes@gmail.com>\n---\n Makefile |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 1f1ce04..c8377d4 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1927,7 +1927,7 @@ git-%$X: %.o $(GITLIBS)\n \n git-imap-send$X: imap-send.o $(GITLIBS)\n \t$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \\\n-\t\t$(LIBS) $(OPENSSL_LINK) $(OPENSSL_LIBSSL)\n+\t\t$(LIBS) $(OPENSSL_LINK) $(OPENSSL_LIBSSL) $(LIB_4_CRYPTO)\n \n git-http-fetch$X: revision.o http.o http-walker.o http-fetch.o $(GITLIBS)\n \t$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \\\n-- \n1.7.3.2\n"},{"id":"157381","messageId":"1291613304.3339.12.camel@fixed-disk","threadId":"25831","inReplyTo":"1290629033-20566-1-git-send-email-flameeyes@gmail.com","subject":"Re: [PATCH] imap-send: link against libcrypto for HMAC and others","fromName":"Anders Kaseorg","fromEmail":"andersk@mit.edu","sentAt":"2010-12-06T05:28:24Z","receivedAt":"2010-12-06T05:28:24Z","isPatch":true,"sender":{"key":"andersk@mit.edu","avatar":"https://avatars.githubusercontent.com/u/26471?v=4"},"body":"On Wed, 2010-11-24 at 21:03 +0100, Diego Elio Pettenò wrote:\n> When using stricter linkers, such as GNU gold or Darwin ld, transitive\n> dependencies are not counted towards symbol resolution. If we don't\n> link imap-send to libcrypto, we'll have undefined references to the\n> HMAC_*, EVP_* and ERR_* functions families.\n> […]\n>  git-imap-send$X: imap-send.o $(GITLIBS)\n>  \t$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \\\n> -\t\t$(LIBS) $(OPENSSL_LINK) $(OPENSSL_LIBSSL)\n> +\t\t$(LIBS) $(OPENSSL_LINK) $(OPENSSL_LIBSSL) $(LIB_4_CRYPTO)\n\nThis broke the build with NO_OPENSSL=1, so Debian will need to revert\nit:\n\n    CC imap-send.o\n    LINK git-imap-send\n/usr/bin/ld: cannot find -lcrypto\ncollect2: ld returned 1 exit status\nmake[1]: *** [git-imap-send] Error 1\n\nAlso, the Makefile already has a NEEDS_CRYPTO_WITH_SSL flag that’s\nautomatically set on Darwin, Windows, and MinGW.  We shouldn’t have two\nmechanisms for addressing the same problem; maybe we just need to enable\nthe existing flag on more (or all) platforms?\n\nAnders\n"},{"id":"157382","messageId":"1291613978.4756.157.camel@yamato.local","threadId":"25831","inReplyTo":"1291613304.3339.12.camel@fixed-disk","subject":"Re: [PATCH] imap-send: link against libcrypto for HMAC and others","fromName":"Diego Elio Pettenò","fromEmail":"flameeyes@gmail.com","sentAt":"2010-12-06T05:39:38Z","receivedAt":"2010-12-06T05:39:38Z","isPatch":true,"sender":{"key":"flameeyes@gmail.com","avatar":"https://avatars.githubusercontent.com/u/74834?v=4"},"body":"Il giorno lun, 06/12/2010 alle 00.28 -0500, Anders Kaseorg ha scritto:\n> This broke the build with NO_OPENSSL=1, so Debian will need to revert\n> it:\n\nI'll try a NO_OPENSSL build later on today and see to get it fixed.\n\n> Also, the Makefile already has a NEEDS_CRYPTO_WITH_SSL flag that’s\n> automatically set on Darwin, Windows, and MinGW.  We shouldn’t have two\n> mechanisms for addressing the same problem; maybe we just need to enable\n> the existing flag on more (or all) platforms?\n\nNo, these should be different issues; you may have a libssl (which uses\nlibcrypto) requiring libcrypto to be linked in, even if you only use\ninterfaces from libssl (and that's what NEEDS_CRYPTO_WITH_SSL seem to be\ndesigned to deal with), but in this case what you have is rather\nimap-send using the libcrypto interfaces _as well as_ the libssl\ninterfaces.\n\nI have blogged a detailed analysis of the problem, if you wish to see\nthe details:\n\nhttp://blog.flameeyes.eu/2010/11/26/it-s-not-all-gold-that-shines-why-underlinking-is-a-bad-thing\n\n-- \nDiego Elio Pettenò — “Flameeyes”\nhttp://blog.flameeyes.eu/\n\nIf you found a .asc file in this mail and know not what it is,\nit's a GnuPG digital signature: http://www.gnupg.org/\n"},{"id":"157383","messageId":"20101206060628.GA30188@burratino","threadId":"25831","inReplyTo":"1291613978.4756.157.camel@yamato.local","subject":"Re: [PATCH] imap-send: link against libcrypto for HMAC and others","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-12-06T06:06:28Z","receivedAt":"2010-12-06T06:06:28Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Diego Elio Pettenò wrote:\n> Il giorno lun, 06/12/2010 alle 00.28 -0500, Anders Kaseorg ha scritto:\n\n>> Also, the Makefile already has a NEEDS_CRYPTO_WITH_SSL flag that’s\n>> automatically set on Darwin, Windows, and MinGW.  We shouldn’t have two\n>> mechanisms for addressing the same problem; maybe we just need to enable\n>> the existing flag on more (or all) platforms?\n>\n> No, these should be different issues\n\nAre you sure?  The change description for v1.6.5-rc1~23 (Makefile: Add\nNEEDS_CRYPTO_WITH_SSL, 2009-10-08) says:\n\n    The Makefile comment for NEEDS_SSL_WITH_CRYPTO says to define it \"if\n    you need -lcrypto with -lssl (Darwin).\"  However, what it actually\n    does is add -lssl when you use -lcrypto and not the other way around.\n    However, libcrypto contains a majority of the ERR_* functions from\n    OpenSSL (at least on OS X) so we need it both ways.\n\nand grepping that version reveals the same hit I think your patch\ndeals with:\n\n $ git grep -F -e ERR_ v1.6.5-rc1~23\n v1.6.5-rc1~23:imap-send.c:      fprintf(stderr, \"%s: %s\\n\", func, ERR_error_string(ERR_get_error(), NULL));\n\nI am guessing the reality is closer to:\n\n    Previously we relied on -lssl pulling in libcrypto on most\n    platforms.  That has at least three problems:\n\n     (1) it is not resiliant against future changes in libssl.\n         The DT_NEEDED entries in a library are generally\n         considered an implementation detail that is allowed to\n         change.\n\n     (2) it does not work on all platforms; for example, ld on\n         Mac OS X and ld --no-copy-dt-needed-entries on Linux\n         do not permit that trick.\n\n     (3) it is tricky.  A simple rule is \"explicitly link to\n         all libraries whos symbols you use directly\".\n\n    To fix this, eliminate the NEEDS_CRYPTO_WITH_SSL knob and\n    always treat it as true.  NEEDS_SSL_WITH_CRYPTO can stay\n    because there really are differences between platforms\n    for that one.\n\nWarning: I could be totally wrong. :)  I have not verified any\nof these claims.\n"},{"id":"157384","messageId":"7v39qbtoq9.fsf@alter.siamese.dyndns.org","threadId":"25831","inReplyTo":"1291613978.4756.157.camel@yamato.local","subject":"Re: [PATCH] imap-send: link against libcrypto for HMAC and others","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-12-06T06:11:26Z","receivedAt":"2010-12-06T06:11:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Diego Elio Pettenò <flameeyes@gmail.com> writes:\n\n> Il giorno lun, 06/12/2010 alle 00.28 -0500, Anders Kaseorg ha scritto:\n>> This broke the build with NO_OPENSSL=1, so Debian will need to revert\n>> it:\n>\n> I'll try a NO_OPENSSL build later on today and see to get it fixed.\n>\n>> Also, the Makefile already has a NEEDS_CRYPTO_WITH_SSL flag that’s\n>> automatically set on Darwin, Windows, and MinGW.  We shouldn’t have two\n>> mechanisms for addressing the same problem; maybe we just need to enable\n>> the existing flag on more (or all) platforms?\n>\n> No, these should be different issues; you may have a libssl (which uses\n> libcrypto) requiring libcrypto to be linked in, even if you only use\n> interfaces from libssl (and that's what NEEDS_CRYPTO_WITH_SSL seem to be\n> designed to deal with), but in this case what you have is rather\n> imap-send using the libcrypto interfaces _as well as_ the libssl\n> interfaces.\n\nYou are both correct; the point of NO_OPENSSL is not to link with anything\nfrom openssl suite, so we need a separate mechanism to address this.\n\nAnders, wouldn't this be a better fix for NO_OPENSSL build, than reverting\na fix for an incorrect ld invocation?\n\n Makefile |    4 ++++\n 1 files changed, 4 insertions(+), 0 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 7a5fb69..b20ae1a 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1296,11 +1296,15 @@ else\n \tBLK_SHA1 = 1\n \tOPENSSL_LIBSSL =\n endif\n+ifdef NO_OPENSSL\n+\tLIB_4_CRYPTO =\n+else\n ifdef NEEDS_SSL_WITH_CRYPTO\n \tLIB_4_CRYPTO = $(OPENSSL_LINK) -lcrypto -lssl\n else\n \tLIB_4_CRYPTO = $(OPENSSL_LINK) -lcrypto\n endif\n+endif\n ifdef NEEDS_LIBICONV\n \tifdef ICONVDIR\n \t\tBASIC_CFLAGS += -I$(ICONVDIR)/include\n"},{"id":"157565","messageId":"20101208015144.GA4868@localhost.localdomain","threadId":"25831","inReplyTo":"7v39qbtoq9.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] imap-send: link against libcrypto for HMAC and others","fromName":"Jared Hance","fromEmail":"jaredhance@gmail.com","sentAt":"2010-12-08T01:51:44Z","receivedAt":"2010-12-08T01:51:44Z","isPatch":true,"sender":{"key":"jaredhance@gmail.com","avatar":"https://avatars.githubusercontent.com/u/170192?v=4"},"body":"> You are both correct; the point of NO_OPENSSL is not to link with anything\n> from openssl suite, so we need a separate mechanism to address this.\n> \n> Anders, wouldn't this be a better fix for NO_OPENSSL build, than reverting\n> a fix for an incorrect ld invocation?\n\nCould we get this fixup patch into master? Leaving the original patch\nin without it doesn't seem like a good idea when it breaks the build.\n"},{"id":"157567","messageId":"7vd3pdm2rz.fsf@alter.siamese.dyndns.org","threadId":"25831","inReplyTo":"20101208015144.GA4868@localhost.localdomain","subject":"Re: [PATCH] imap-send: link against libcrypto for HMAC and others","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-12-08T02:12:00Z","receivedAt":"2010-12-08T02:12:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jared Hance <jaredhance@gmail.com> writes:\n\n>> You are both correct; the point of NO_OPENSSL is not to link with anything\n>> from openssl suite, so we need a separate mechanism to address this.\n>> \n>> Anders, wouldn't this be a better fix for NO_OPENSSL build, than reverting\n>> a fix for an incorrect ld invocation?\n>\n> Could we get this fixup patch into master? Leaving the original patch\n> in without it doesn't seem like a good idea when it breaks the build.\n\nThat depends on what Diego and Anders would say/report, and that is why\ntheir address were on To/Cc in the message you are replying to.  Why did\nyou cull Cc from your message?\n\nOne thing I do not like about it is that the \"fixup\" is ugly.  It makes\npeople expect to add $(LIB_4_CRYPTO) on their linker command line to get a\ncorrect linking with -lcrypto library, when all it does is _not_ to link\nwith -lcrypto at all, so whatever program that uses it needs to know about\nNO_OPENSSL and refrain from using the symbols from that library.\n"},{"id":"157578","messageId":"alpine.DEB.2.02.1012072347560.6372@dr-wily.mit.edu","threadId":"25831","inReplyTo":"7v39qbtoq9.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] imap-send: link against libcrypto for HMAC and others","fromName":"Anders Kaseorg","fromEmail":"andersk@mit.edu","sentAt":"2010-12-08T04:53:40Z","receivedAt":"2010-12-08T04:53:40Z","isPatch":true,"sender":{"key":"andersk@mit.edu","avatar":"https://avatars.githubusercontent.com/u/26471?v=4"},"body":"On Sun, 5 Dec 2010, Junio C Hamano wrote:\n> Anders, wouldn't this be a better fix for NO_OPENSSL build, than reverting\n> a fix for an incorrect ld invocation?\n\nIt works for me.  I agree it isn’t beautiful but I don’t think I have a \nbetter idea at this time.\n\n(It’s worth pointing out that $(OPENSSL_LINK) is duplicated in the \ngit-imap-send build command, once directly and once indirectly via \n$(LIB_4_CRYPTO), but I assume this is intentional for clarity.)\n\nAnders\n"}]}