Re: AW: SSL_VERSION_LIBRARY
On Thu, Sep 13, 2007 at 09:08:26AM -0500, William Rowe wrote: Joe Orton wrote: On Mon, Sep 10, 2007 at 09:47:24PM +0200, Ruediger Pluem wrote: On 09/10/2007 08:40 AM, Plüm wrote: That was the goal of my diagnostic patch: Finding out if we have a pool issue. Looks like we have. I guess the right fix is as you say to use the parent pool (process scope). Not 100% sure regarding the correct pool, but would that be the correct fix That's not really thread-safe, and it ought to be, though we might get away with it since it's called during startup. But rather than guessing pools, actually caching the stuff once at startup is probably cleanest, I've reviewed, I'd be entirely happy with the short-and-sweet hack on 2.0 and 2.2 if you would like this to become the new logic for trunk. Would that appeal? I guess that's fine. It should at least be explicitly documented in the 2.0/2.2 backports as here be dragons. Regards, joe
Re: AW: SSL_VERSION_LIBRARY
Joe Orton wrote: On Mon, Sep 10, 2007 at 09:47:24PM +0200, Ruediger Pluem wrote: On 09/10/2007 08:40 AM, Plüm wrote: That was the goal of my diagnostic patch: Finding out if we have a pool issue. Looks like we have. I guess the right fix is as you say to use the parent pool (process scope). Not 100% sure regarding the correct pool, but would that be the correct fix That's not really thread-safe, and it ought to be, though we might get away with it since it's called during startup. But rather than guessing pools, actually caching the stuff once at startup is probably cleanest, I've reviewed, I'd be entirely happy with the short-and-sweet hack on 2.0 and 2.2 if you would like this to become the new logic for trunk. Would that appeal? Bill
Re: AW: SSL_VERSION_LIBRARY
William A. Rowe, Jr. wrote:
William A. Rowe, Jr. wrote:
Looking at the scope of all these static calls, I really believe the
patch is this simple (process-pool survives the entire httpd);
Sorry - scratch that. I wasn't counting the frequency of pstrdup calls.
Just begging for optimization :)
Without fretting the optimizations, try this for the cleanest patch I could
think of (remember, these are static internal functions)
Index: modules/ssl/ssl_engine_vars.c
===
--- modules/ssl/ssl_engine_vars.c (revision 574494)
+++ modules/ssl/ssl_engine_vars.c (working copy)
@@ -49,7 +49,7 @@
static char *ssl_var_lookup_ssl_cert_verify(apr_pool_t *p, conn_rec *c);
static char *ssl_var_lookup_ssl_cipher(apr_pool_t *p, conn_rec *c, char *var);
static void ssl_var_lookup_ssl_cipher_bits(SSL *ssl, int *usekeysize, int *algkeysize);
-static char *ssl_var_lookup_ssl_version(apr_pool_t *p, char *var);
+static char *ssl_var_lookup_ssl_version(apr_pool_t *pp, apr_pool_t *p, char *var);
static char *ssl_var_lookup_ssl_compress_meth(SSL *ssl);
static int ssl_is_https(conn_rec *c)
@@ -190,7 +190,7 @@
*/
if (result == NULL) {
if (strlen(var) 12 strcEQn(var, SSL_VERSION_, 12))
-result = ssl_var_lookup_ssl_version(p, var+12);
+result = ssl_var_lookup_ssl_version(s-process-pool, p, var+12);
else if (strcEQ(var, SERVER_SOFTWARE))
result = ap_get_server_banner();
else if (strcEQ(var, API_VERSION)) {
@@ -262,7 +262,8 @@
ssl = sslconn-ssl;
if (strlen(var) 8 strcEQn(var, VERSION_, 8)) {
-result = ssl_var_lookup_ssl_version(p, var+8);
+result = ssl_var_lookup_ssl_version(c-base_server-process-pool,
+p, var+8);
}
else if (ssl != NULL strcEQ(var, PROTOCOL)) {
result = (char *)SSL_get_version(ssl);
@@ -633,7 +634,7 @@
return;
}
-static char *ssl_var_lookup_ssl_version(apr_pool_t *p, char *var)
+static char *ssl_var_lookup_ssl_version(apr_pool_t *pp, apr_pool_t *p, char *var)
{
static const char interface[] = mod_ssl/ MOD_SSL_VERSION;
static char library_interface[] = SSL_LIBRARY_TEXT;
@@ -642,7 +643,7 @@
if (!library) {
char *cp, *cp2;
-library = apr_pstrdup(p, SSL_LIBRARY_DYNTEXT);
+library = apr_pstrdup(pp, SSL_LIBRARY_DYNTEXT);
if ((cp = strchr(library, ' ')) != NULL) {
*cp = '/';
if ((cp2 = strchr(cp, ' ')) != NULL)
Re: AW: SSL_VERSION_LIBRARY
-Ursprüngliche Nachricht- Von: William A. Rowe, Jr. [mailto:[EMAIL PROTECTED] Gesendet: Mittwoch, 12. September 2007 09:47 An: dev@httpd.apache.org Betreff: Re: AW: SSL_VERSION_LIBRARY William A. Rowe, Jr. wrote: William A. Rowe, Jr. wrote: Looking at the scope of all these static calls, I really believe the patch is this simple (process-pool survives the entire httpd); Sorry - scratch that. I wasn't counting the frequency of pstrdup calls. Just begging for optimization :) Without fretting the optimizations, try this for the cleanest patch I could think of (remember, these are static internal functions) Also looks good for me. Thanks for working this out. Mind to attach this patch to PR43334 (https://issues.apache.org/bugzilla/show_bug.cgi?id=43334) so that people there can test? Regards Rüdiger
Re: AW: SSL_VERSION_LIBRARY
I have tested the patch and it works fine. Thanks a lot. On 12/09/07 13:14, Plüm wrote: -Ursprüngliche Nachricht- Von: William A. Rowe, Jr. [mailto:[EMAIL PROTECTED] Gesendet: Mittwoch, 12. September 2007 09:47 An: dev@httpd.apache.org Betreff: Re: AW: SSL_VERSION_LIBRARY William A. Rowe, Jr. wrote: William A. Rowe, Jr. wrote: Looking at the scope of all these static calls, I really believe the patch is this simple (process-pool survives the entire httpd); Sorry - scratch that. I wasn't counting the frequency of pstrdup calls. Just begging for optimization :) Without fretting the optimizations, try this for the cleanest patch I could think of (remember, these are static internal functions) Also looks good for me. Thanks for working this out. Mind to attach this patch to PR43334 (https://issues.apache.org/bugzilla/show_bug.cgi?id=43334) so that people there can test? Regards Rüdiger -- Dr. Zvi Har'El mailto:[EMAIL PROTECTED]Department of Mathematics tel:+972-54-4227607 Technion - Israel Institute of Technology fax:+972-4-8293388 http://www.math.technion.ac.il/~rl/Haifa 32000, ISRAEL If you can't say somethin' nice, don't say nothin' at all. -- Thumper (1942)
Re: AW: SSL_VERSION_LIBRARY
Plüm wrote: Also looks good for me. Thanks for working this out. Mind to attach this patch to PR43334 (https://issues.apache.org/bugzilla/show_bug.cgi?id=43334) so that people there can test? Better yet, committed the patch to trunk and pointed the url @ the commit.
Re: AW: SSL_VERSION_LIBRARY
On Sep 12, 2007, at 3:47 AM, William A. Rowe, Jr. wrote: William A. Rowe, Jr. wrote: William A. Rowe, Jr. wrote: Looking at the scope of all these static calls, I really believe the patch is this simple (process-pool survives the entire httpd); Sorry - scratch that. I wasn't counting the frequency of pstrdup calls. Just begging for optimization :) Without fretting the optimizations, try this for the cleanest patch I could think of (remember, these are static internal functions) +1
Re: AW: SSL_VERSION_LIBRARY
On Mon, Sep 10, 2007 at 09:47:24PM +0200, Ruediger Pluem wrote:
On 09/10/2007 08:40 AM, Plüm wrote:
That was the goal of my diagnostic patch: Finding out if we have a pool
issue. Looks like we have. I guess the right fix is as you say
to use the parent pool (process scope).
Not 100% sure regarding the correct pool, but would that be the correct fix
That's not really thread-safe, and it ought to be, though we might get
away with it since it's called during startup. But rather than guessing
pools, actually caching the stuff once at startup is probably cleanest,
e.g.:
Index: ssl_private.h
===
--- ssl_private.h (revision 574523)
+++ ssl_private.h (working copy)
@@ -680,7 +680,7 @@
void ssl_log_ssl_error(const char *, int, int, server_rec *);
/** Variables */
-void ssl_var_register(void);
+void ssl_var_register(apr_pool_t *p);
char*ssl_var_lookup(apr_pool_t *, server_rec *, conn_rec *,
request_rec *, char *);
apr_array_header_t *ssl_ext_list(apr_pool_t *p, conn_rec *c, int peer, const
char *extension);
Index: ssl_engine_vars.c
===
--- ssl_engine_vars.c (revision 574523)
+++ ssl_engine_vars.c (working copy)
@@ -58,12 +58,32 @@
return sslconn sslconn-ssl;
}
-void ssl_var_register(void)
+static const char var_interface[] = mod_ssl/ MOD_SSL_VERSION;
+static char var_library_interface[] = SSL_LIBRARY_TEXT;
+static char *var_library = NULL;
+
+void ssl_var_register(apr_pool_t *p)
{
+char *cp, *cp2;
+
APR_REGISTER_OPTIONAL_FN(ssl_is_https);
APR_REGISTER_OPTIONAL_FN(ssl_var_lookup);
APR_REGISTER_OPTIONAL_FN(ssl_ext_list);
-return;
+
+/* Perform once-per-process library version determination: */
+var_library = apr_pstrdup(p, SSL_LIBRARY_DYNTEXT);
+
+if ((cp = strchr(var_library, ' ')) != NULL) {
+*cp = '/';
+if ((cp2 = strchr(cp, ' ')) != NULL)
+*cp2 = NUL;
+}
+
+if ((cp = strchr(var_library_interface, ' ')) != NULL) {
+*cp = '/';
+if ((cp2 = strchr(cp, ' ')) != NULL)
+*cp2 = NUL;
+}
}
/* This function must remain safe to use for a non-SSL connection. */
@@ -635,34 +655,16 @@
static char *ssl_var_lookup_ssl_version(apr_pool_t *p, char *var)
{
-static const char interface[] = mod_ssl/ MOD_SSL_VERSION;
-static char library_interface[] = SSL_LIBRARY_TEXT;
-static char *library = NULL;
char *result;
-if (!library) {
-char *cp, *cp2;
-library = apr_pstrdup(p, SSL_LIBRARY_DYNTEXT);
-if ((cp = strchr(library, ' ')) != NULL) {
-*cp = '/';
-if ((cp2 = strchr(cp, ' ')) != NULL)
-*cp2 = NUL;
-}
-if ((cp = strchr(library_interface, ' ')) != NULL) {
-*cp = '/';
-if ((cp2 = strchr(cp, ' ')) != NULL)
-*cp2 = NUL;
-}
-}
-
if (strEQ(var, INTERFACE)) {
-result = apr_pstrdup(p, interface);
+result = apr_pstrdup(p, var_interface);
}
else if (strEQ(var, LIBRARY_INTERFACE)) {
-result = apr_pstrdup(p, library_interface);
+result = apr_pstrdup(p, var_library_interface);
}
else if (strEQ(var, LIBRARY)) {
-result = apr_pstrdup(p, library);
+result = apr_pstrdup(p, var_library);
}
else {
result = NULL;
Index: mod_ssl.c
===
--- mod_ssl.c (revision 574523)
+++ mod_ssl.c (working copy)
@@ -482,7 +482,7 @@
ap_hook_insert_filter (ssl_hook_Insert_Filter, NULL,NULL, APR_HOOK_MIDDLE);
/*ap_hook_handler (ssl_hook_Upgrade, NULL,NULL,
APR_HOOK_MIDDLE); */
-ssl_var_register();
+ssl_var_register(p);
APR_REGISTER_OPTIONAL_FN(ssl_proxy_enable);
APR_REGISTER_OPTIONAL_FN(ssl_engine_disable);
Re: AW: SSL_VERSION_LIBRARY
-Ursprüngliche Nachricht- Von: Joe Orton Gesendet: Dienstag, 11. September 2007 11:35 An: dev@httpd.apache.org Betreff: Re: AW: SSL_VERSION_LIBRARY On Mon, Sep 10, 2007 at 09:47:24PM +0200, Ruediger Pluem wrote: On 09/10/2007 08:40 AM, Plüm wrote: That was the goal of my diagnostic patch: Finding out if we have a pool issue. Looks like we have. I guess the right fix is as you say to use the parent pool (process scope). Not 100% sure regarding the correct pool, but would that be the correct fix That's not really thread-safe, and it ought to be, though we might get away with it since it's called during startup. But rather than guessing pools, actually caching the stuff once at startup is probably cleanest, e.g.: Looks good for me. Thanks for working this out. I assume the pool given to ssl_register_hooks is a very long living pool that lasts as long as the process lives. Mind to attach this patch to PR43334 (https://issues.apache.org/bugzilla/show_bug.cgi?id=43334)? Regards Rüdiger
Re: AW: SSL_VERSION_LIBRARY
What with this and the Win32/apr issues, seems to me that we should consider a 2.2.7 out soonish :)
Re: AW: SSL_VERSION_LIBRARY
Jim Jagielski wrote: What with this and the Win32/apr issues, seems to me that we should consider a 2.2.7 out soonish :) I was about to suggest the same :) With Win32/APR there isn't a fix. Not yet at least, Tom Donovan and I are going back and forth with ideas that break the fewest binaries (it's not going to be workable to require users to obtain new copies). So some fix that is entirely made by a minor change to mod_fcgid itself, and not the individually compiled fcgid's, is more appropriate. But I've sort of put that aside to look at the mod_perl issues that some user had reported. Will have some update shortly this week. The SSL_VERISON_LIBRARY is thankfully quite fixable, something in, say patches/apply_to_2.2.6 would certainly go a long ways to helping in the very short term. Bill
Re: AW: SSL_VERSION_LIBRARY
Looking at the scope of all these static calls, I really believe the
patch is this simple (process-pool survives the entire httpd);
Index: ssl_engine_vars.c
===
--- ssl_engine_vars.c (revision 574494)
+++ ssl_engine_vars.c (working copy)
@@ -190,7 +190,7 @@
*/
if (result == NULL) {
if (strlen(var) 12 strcEQn(var, SSL_VERSION_, 12))
-result = ssl_var_lookup_ssl_version(p, var+12);
+result = ssl_var_lookup_ssl_version(s-process-pool, var+12);
else if (strcEQ(var, SERVER_SOFTWARE))
result = ap_get_server_banner();
else if (strcEQ(var, API_VERSION)) {
@@ -262,7 +262,8 @@
ssl = sslconn-ssl;
if (strlen(var) 8 strcEQn(var, VERSION_, 8)) {
-result = ssl_var_lookup_ssl_version(p, var+8);
+result = ssl_var_lookup_ssl_version(c-base_server-process-pool,
+var+8);
}
else if (ssl != NULL strcEQ(var, PROTOCOL)) {
result = (char *)SSL_get_version(ssl);
Re: AW: SSL_VERSION_LIBRARY
William A. Rowe, Jr. wrote: Looking at the scope of all these static calls, I really believe the patch is this simple (process-pool survives the entire httpd); Sorry - scratch that. I wasn't counting the frequency of pstrdup calls. Just begging for optimization :)
AW: SSL_VERSION_LIBRARY
-Ursprüngliche Nachricht- Von: William A. Rowe, Jr. Gesendet: Montag, 10. September 2007 07:50 An: dev@httpd.apache.org Betreff: Re: SSL_VERSION_LIBRARY Zvi Har'El wrote: This looks similar to PR 43334 (https://issues.apache.org/bugzilla/show_bug.cgi?id=43334). Could you please test my diagnostic patch from there? Yup - that patch would solve it since we don't reinit static char*library to null on every unload/reload cycle. The fix is either to do it always (as this patch does) - or since it might outlive p, delve down to parent pool (process scope) and allocate from there. That was the goal of my diagnostic patch: Finding out if we have a pool issue. Looks like we have. I guess the right fix is as you say to use the parent pool (process scope). Regards Rüdiger
Re: AW: SSL_VERSION_LIBRARY
On 09/10/2007 08:40 AM, Plüm wrote:
-Ursprüngliche Nachricht-
Von: William A. Rowe, Jr.
Gesendet: Montag, 10. September 2007 07:50
An: dev@httpd.apache.org
Betreff: Re: SSL_VERSION_LIBRARY
Zvi Har'El wrote:
This looks similar to PR 43334
(https://issues.apache.org/bugzilla/show_bug.cgi?id=43334).
Could you please test my diagnostic patch from there?
Yup - that patch would solve it since we don't reinit static
char*library
to null on every unload/reload cycle.
The fix is either to do it always (as this patch does) - or since
it might outlive p, delve down to parent pool (process scope) and
allocate from there.
That was the goal of my diagnostic patch: Finding out if we have a pool
issue. Looks like we have. I guess the right fix is as you say
to use the parent pool (process scope).
Not 100% sure regarding the correct pool, but would that be the correct fix
Index: modules/ssl/ssl_engine_vars.c
===
--- modules/ssl/ssl_engine_vars.c (Revision 573732)
+++ modules/ssl/ssl_engine_vars.c (Arbeitskopie)
@@ -642,7 +642,13 @@
if (!library) {
char *cp, *cp2;
-library = apr_pstrdup(p, SSL_LIBRARY_DYNTEXT);
+apr_pool_t *lp1, *lp2;
+
+lp1 = p;
+while ((lp2 = apr_pool_parent_get(lp1)) != NULL) {
+lp1 = lp2;
+}
+library = apr_pstrdup(lp1, SSL_LIBRARY_DYNTEXT);
if ((cp = strchr(library, ' ')) != NULL) {
*cp = '/';
if ((cp2 = strchr(cp, ' ')) != NULL)
?
or is the following already sufficient?
Index: modules/ssl/ssl_engine_vars.c
===
--- modules/ssl/ssl_engine_vars.c (Revision 573732)
+++ modules/ssl/ssl_engine_vars.c (Arbeitskopie)
@@ -642,7 +642,13 @@
if (!library) {
char *cp, *cp2;
-library = apr_pstrdup(p, SSL_LIBRARY_DYNTEXT);
+apr_pool_t *lp;
+
+lp = apr_pool_parent_get(p);
+if (!lp) {
+lp = p;
+}
+library = apr_pstrdup(lp, SSL_LIBRARY_DYNTEXT);
if ((cp = strchr(library, ' ')) != NULL) {
*cp = '/';
if ((cp2 = strchr(cp, ' ')) != NULL)
Regards
Rüdiger
