Hi,
On June 18, 2003 02:28 am, Justin Erenkrantz wrote:
> Finally got a chance to dig some more and found that if I add:
>
> CRYPTO_set_id_callback(openssl_id);
>
> to ssl_hook_pre_config before the ENGINE_load_builtin_engines and where
> openssl_id is defined as:
>
> static unsigned long openssl_id(void)
> {
> /* FIXME: This is lame and not portable. -aaron */
> return (unsigned long) apr_os_thread_current();
> }
>
> (The above is used in flood - hence Aaron's comment.)
>
> The SEGVs go away if we do this. Does this make sense given the
> internals of OpenSSL?
I'm not the best person to talk about with respect to locking in openssl
(I more or less detest threads, an Alan Cox quote about state-machines
springs to mind...). That said, :-), I took another look at the backtrace
you posted and it certainly seems odd. Digging ... the implementation of
CRYPTO_thread_id() is;
unsigned long CRYPTO_thread_id(void)
{
unsigned long ret=0;
if (id_callback == NULL)
{
#ifdef OPENSSL_SYS_WIN16
ret=(unsigned long)GetCurrentTask();
#elif defined(OPENSSL_SYS_WIN32)
ret=(unsigned long)GetCurrentThreadId();
#elif defined(GETPID_IS_MEANINGLESS)
ret=1L;
#else
ret=(unsigned long)getpid();
#endif
}
else
ret=id_callback();
return(ret);
}
The segfault is (or appears to be, stack smashing possibilities aside)
occuring in a function called from this, and the address is less than the
other openssl library functions in the trace, but not by much. Which begs
the question - is id_callback set to something (and if so, what?) or does
the address correspond to getpid()? Unfortunately, I think the easiest
way to see this would be to hack the above function (and perhaps others
in crypto/cryptlib.c that deal with id_callback) to watch what is going
on.
> On this same tangent, do we need to be doing all of the CRYPTO_<lock>
> stuff? I don't believe we are doing that. And, I know in flood, we had
> lots of problems until we called them. So, I think mod_ssl should be
> passing the lock structures - especially for worker MPM builds...
Whenever I build Apache, I typically build a version of openssl configured
with "no-threads" so this sort of issue is implicitly ruled out. It also
removes a few meaningless function calls given that (multi-forked,
traditional) Apache doesn't need any thread-safe locking. However I know
that isn't the answer to your question in general, as many people will be
using the openssl libs bundled with their system and/or trying to use
Apache2 in a threaded setup.
> While I've got someone's attention, I have some real issues in that
> both of the OpenSSL lock implementations (CRYPTO_set_locking_callback
> and CRYPTO_set_dynlock_*) require global variables to implement. I
> don't think I can say 'ick' loud enough.
>
> What would the possibility/feasibility of passing application-specific
> opaque values through the CRYPTO_lock callbacks? Say
>
> static void openssl_lock(int mode, int n, void *app, const char *file,
> int line)
>
> static CRYPTO_dynlock_value *ssl_dyn_create(void *app, const char*
> file, int line)
>
> We can have app passed into the CRYPTO_set functions - in the case of
> httpd, it would be the pools for ssl_dyn_create *or* the global array
> of locks that openssl_lock would require. You don't know how much
> happier that would make me if we could do that. globals are just so
> icky. -- justin
For this and the previous question, the short answer is "maybe, but it
doesn't solve the problem of working with existing versions".
In both cases, ie. the segfault you're seeing *and* the issue about
attaching an opaque pointer to caller-provide locking callbacks, it would
make sense to take that conversation over to openssl-dev. In particular,
we should get any appropriate details into the request-tracking system.
BTW: Once we're over there, Richard Levitte may be able to provide better
comments than I on this locking stuff, particularly as the dynlock stuff
was (IIRC) his creation.
Cheers,
Geoff
--
Geoff Thorpe
[EMAIL PROTECTED]
http://www.geoffthorpe.net/
