httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Geoff Thorpe <ge...@geoffthorpe.net>
Subject Re: cvs commit: httpd-2.0/modules/ssl ...
Date Mon, 19 May 2003 19:31:00 GMT
Hey there,

On May 19, 2003 01:01 pm, William A. Rowe, Jr. wrote:
> >The AC_CHECK_FUNCS stuff is not really useful here because of the
> >overhauls made in acinclude.h (which was why they were removed in the
> >first place).
>
> Then what exactly did we replace the checks for SSL_set_cert_store
> and SSL_set_state with?  Apparently we didn't, and the last refactoring
> was most broken on that point.

The support I coded up used autoconfisms for testing the various things, 
AC_CHECK_HEADER(S), AC_CHECK_LIB, etc. The previous support simply 
scanned hard-coded paths and did various shell-based examinations to 
*find* header and lib paths. My reading at the time of those 
AC_CHECK_FUNCS macros in modules/ssl/config.m4 was that they existed 
merely to provide some verification that what had been "found" actually 
worked. If there was any other significance to them, it was too subtle 
for me. :-)

> >They actually risk to fail unnecessarily because of the
> >issues with dependencies on apr - see the APACHE_CHECK_SSL_TOOLKIT
> >implementation.
>
> How is that?

Because that's what was happening for me when I was writing the autoconf 
code to begin with. I had to start saving and restoring variables so that 
I could mangle them while testing (eg. pulling in the apr linker flags 
temporarily). Running further ssl tests outside that save/restore macro 
block before the rest of configuration process is complete seems more 
fragile to my eye - but your experience with this build system is more 
extensive than mine so I'll defer. I merely wanted to draw your attention 
to the changes I'd made before, as the revert and changes you made did 
not show whether or not the previous changes were on your radar.

> >I'd suggest that the ENGIEN test be inserted into
> >APACHE_CHECK_SSL_TOOLKIT macro,
>
> That could be done...

Cool. If nothing else, that'd probably keep things more self-contained and 
with less potential for surprises.

> >and it would probably make more sense to
> >test for the openssl/engine.h header rather than the ENGINE_init()
> >function (eg. future versions might redefine ENGINE_init as a
> >backwards-compatibility macro rather than an actual function, but that
> >would fail the current autoconf test).
>
> Sorry, but that is a no-go.  This has already come up on the curl list.
>
> The problem is that someone may have several flavors of SSL installed
> on a given box, and that we may pick up another installed header rather
> than the one *selected* by --with-ssl={/path}.  This can't occur as
> easily with tests against explicit -l{ib}s.

Well the previous --with-ssl=... logic was broken which is why I ended up 
hacking in this code to begin with. I'm fully aware of the need to 
support building against arbitrary SSL installations (and/or uninstalled 
build-trees). I'm not sure that AC_CHECK_FUNC() is more resistant against 
inadvertently linking with unintended libraries than AC_CHECK_HEADER() is 
with unintended headers. Anyway, like I say - I'm just highlighting a 
couple of things in case they'd previously gone unnoticed or 
unconsidered, the rest is your call.

> Because we add the proper -L{sslpath} and -l{libs} at the end of
> APACHE_CHECK_SSL_TOOLKIT()'s invocation, we certainly better
> be safe using AC_CHECK_FUNC() or our entire build is broken.

Well, the problem I was observing is that it's fine that "-lssl -lcrypto" 
has been added with any appropriate "-L<path>"s, but that doesn't mean 
the linker can resolve. If the subsequent system crud required to resolve 
is later brought in from apr or wherever, then things will just work at 
compile-time, but it doesn't mean that they'll work outside 
APACHE_CHECK_SSL_TOOLKIT but before the rest of configuration is 
complete. This is why there is saving and restoring of LIBS in the macro, 
it allows me to temporarily pull in the apr linker flags (-ldl -lsocket 
...) so things resolve.

> Just for your amusement, we choose ENGINE_init instead of some
> ENGINE_load_all_engines function because the other modules within
> engine support require -ldl or -ldld or whatnot to also link. 
> ENGINE_init was a far simpler function to test.

Well, if you want to stick with that, go ahead. You'll still need to check 
for the ENGINE header anyway because you need to know whether to include 
it or not. As I say, API compatibility doesn't require that ENGINE_init() 
remains a function, only that it remains defined in the headers. For the 
short term, you're safe one way or the other.

> Certainly an alternative, but not a really effective one due to the
> points I mentioned above.  This issue already came up with curl, which
> used the header trick before it broke enough users.

I'm rattling this off in a hurry before I leave, so I haven't had a chance 
to examine the issue you mentioned about curl - I'll try to do so later. 
However, I think "getting the wrong lib" is just as easy as "getting the 
wrong header", and expecting ENGINE_init() to be a function may be an 
unnecessary requirement. One thing that occurs to me from your comments 
is that a "Good Idea" would be to try to compare version information from 
the output of a linked executable with that found in the headers. This is 
autoconfery beyond my means however, though I'd try to help anyone who 
was better versed in M4.

> If an SSL toolkit comes up with an alternate function to ENGINE_init
> we can deal with it then.  Note that I'm not against also using
> HAVE_OPENSSL_ENGINE_H (which is the clean way of doing
> what you attempted above),

<doh> yes indeed.

> but we need to check ENGINE_init
> to avoid the situation where crufty headers are hanging out
> in /usr/local/include.

but crufty libs hang out in /usr/lib too ...

> The biggest problem with dropping SSL_set_state, SSL_set_cert_store,
> and now ENGINE_init is that all three may be introduced by tossing
> in the appropriate patches to openssl or sslc.  That is, we could be
> looking at either 0.9.6-engine or a vanilla flavor.

Absolutely, I'm all for autoconf tests for these things if required, but 
in the right place. However the version checks for openssl (at least) are 
currently at a point where the SSL ones *do* exist and the ENGINE_init() 
is 1-1 with the presence of openssl/engine.h. I can't speak for SSLC 
though.

> If we have really broken things to the point where AutoConf cannot
> use it's simplest macros, then we have a bigger issue to examine.

I had to pull in apr flags temporarily and restore LIBS later to do 
precisely what you're talking about. I wouldn't say that meant things 
were broken, but I think it's a good argument for putting any further 
ssl-related tests in the same save/restore block so they don't fall prey 
to the same problems (eg. if ENGINE_init() does exist but won't link 
because of -ldl, it'll fail outside the save/restore block but succeed 
inside it).

Cheers,
Geoff

-- 
Geoff Thorpe
geoff@geoffthorpe.net
http://www.geoffthorpe.net/


Mime
View raw message