* [PATCH] imap-send: link against libcrypto for HMAC and others
@ 2010-11-24 13:55 Diego Elio Pettenò
2010-11-24 20:01 ` Junio C Hamano
0 siblings, 1 reply; 10+ messages in thread
From: Diego Elio Pettenò @ 2010-11-24 13:55 UTC (permalink / raw)
To: git
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] imap-send: link against libcrypto for HMAC and others
2010-11-24 13:55 [PATCH] imap-send: link against libcrypto for HMAC and others Diego Elio Pettenò
@ 2010-11-24 20:01 ` Junio C Hamano
2010-11-24 20:03 ` Diego Elio Pettenò
0 siblings, 1 reply; 10+ messages in thread
From: Junio C Hamano @ 2010-11-24 20:01 UTC (permalink / raw)
To: Diego Elio Pettenò; +Cc: git
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] imap-send: link against libcrypto for HMAC and others
2010-11-24 20:01 ` Junio C Hamano
@ 2010-11-24 20:03 ` Diego Elio Pettenò
2010-12-06 5:28 ` Anders Kaseorg
0 siblings, 1 reply; 10+ messages in thread
From: Diego Elio Pettenò @ 2010-11-24 20:03 UTC (permalink / raw)
To: git
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] imap-send: link against libcrypto for HMAC and others
2010-11-24 20:03 ` Diego Elio Pettenò
@ 2010-12-06 5:28 ` Anders Kaseorg
2010-12-06 5:39 ` Diego Elio Pettenò
0 siblings, 1 reply; 10+ messages in thread
From: Anders Kaseorg @ 2010-12-06 5:28 UTC (permalink / raw)
To: Diego Elio Pettenò, Junio C Hamano; +Cc: git
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] imap-send: link against libcrypto for HMAC and others
2010-12-06 5:28 ` Anders Kaseorg
@ 2010-12-06 5:39 ` Diego Elio Pettenò
2010-12-06 6:06 ` Jonathan Nieder
2010-12-06 6:11 ` Junio C Hamano
0 siblings, 2 replies; 10+ messages in thread
From: Diego Elio Pettenò @ 2010-12-06 5:39 UTC (permalink / raw)
To: Anders Kaseorg; +Cc: Junio C Hamano, git
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/
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] imap-send: link against libcrypto for HMAC and others
2010-12-06 5:39 ` Diego Elio Pettenò
@ 2010-12-06 6:06 ` Jonathan Nieder
2010-12-06 6:11 ` Junio C Hamano
1 sibling, 0 replies; 10+ messages in thread
From: Jonathan Nieder @ 2010-12-06 6:06 UTC (permalink / raw)
To: Diego Elio Pettenò
Cc: Anders Kaseorg, Junio C Hamano, git, Brian Gernhardt
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] imap-send: link against libcrypto for HMAC and others
2010-12-06 5:39 ` Diego Elio Pettenò
2010-12-06 6:06 ` Jonathan Nieder
@ 2010-12-06 6:11 ` Junio C Hamano
2010-12-08 1:51 ` Jared Hance
2010-12-08 4:53 ` Anders Kaseorg
1 sibling, 2 replies; 10+ messages in thread
From: Junio C Hamano @ 2010-12-06 6:11 UTC (permalink / raw)
To: Diego Elio Pettenò; +Cc: Anders Kaseorg, git
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] imap-send: link against libcrypto for HMAC and others
2010-12-06 6:11 ` Junio C Hamano
@ 2010-12-08 1:51 ` Jared Hance
2010-12-08 2:12 ` Junio C Hamano
2010-12-08 4:53 ` Anders Kaseorg
1 sibling, 1 reply; 10+ messages in thread
From: Jared Hance @ 2010-12-08 1:51 UTC (permalink / raw)
To: git
> 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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] imap-send: link against libcrypto for HMAC and others
2010-12-08 1:51 ` Jared Hance
@ 2010-12-08 2:12 ` Junio C Hamano
0 siblings, 0 replies; 10+ messages in thread
From: Junio C Hamano @ 2010-12-08 2:12 UTC (permalink / raw)
To: Jared Hance; +Cc: Diego Elio Pettenò, Anders Kaseorg, git
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] imap-send: link against libcrypto for HMAC and others
2010-12-06 6:11 ` Junio C Hamano
2010-12-08 1:51 ` Jared Hance
@ 2010-12-08 4:53 ` Anders Kaseorg
1 sibling, 0 replies; 10+ messages in thread
From: Anders Kaseorg @ 2010-12-08 4:53 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Diego Elio Pettenò, git
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
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2010-12-08 4:58 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-11-24 13:55 [PATCH] imap-send: link against libcrypto for HMAC and others Diego Elio Pettenò
2010-11-24 20:01 ` Junio C Hamano
2010-11-24 20:03 ` Diego Elio Pettenò
2010-12-06 5:28 ` Anders Kaseorg
2010-12-06 5:39 ` Diego Elio Pettenò
2010-12-06 6:06 ` Jonathan Nieder
2010-12-06 6:11 ` Junio C Hamano
2010-12-08 1:51 ` Jared Hance
2010-12-08 2:12 ` Junio C Hamano
2010-12-08 4:53 ` Anders Kaseorg
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.