Re: Unexpected Warnings from Macro Use in 2.4

[Eric Covener]

On Wed, Oct 5, 2016 at 8:48 AM, Nick Gearls  wrote:
> Nobody is interested in avoiding problems with this sanity check?
> Trivial to review, only a warning - but that could save hours to users
>
I didn't see it the first time, but I don't think mod_macro should
emit a warning for its own basic configuration because it happens to
overlap with the cores variable expansion. I don't like it for 2.4.

Re: mod_macro: Control on bad nesting

[Jim Jagielski]

'bool' ??

> On Oct 5, 2016, at 8:50 AM, Nick Gearls  wrote:
> 
> The proposed patch 
> (https://bz.apache.org/bugzilla/attachment.cgi?id=34012=diff) is fully 
> back-ward compatible and can save a lot of useless warnings in the log.
> Anyway to get somebody review it?
> 
> Thanks
> 
> On 06-07-2016 09:20, Nick Gearls wrote:
>> There's a patch (see https://bz.apache.org/bugzilla/show_bug.cgi?id=59660) 
>> containing both options (global directive + flag) for both warnings (nesting 
>> & empty arguments)
>> 
>>> Good point.
>>> A global option is not a good option as it would disable this very
>>> useful check for all macros.
>>> 
>>> What about ">> 
>>> On Fri, 3 Jun 2016 10:19:24 -0400, Eric Covener  wrote:
>>> 
>>> > What syntax would be the best one? maybe a (one character) /option after 
>>> > the
>>> > Macro keyword?
>>> > Ex: "" (and later ">> Kinda ugly and probably breaks the core from recognizing ">> Maybe a global directive to turn off the nesting check?   Or a
>>> convention for the macro name? Or a second tag like
>>>  (bad name) that is a very thin wrapper around
>>> existing stuff?
>> 
> 

Re: mod_macro: Control on bad nesting

[Nick Gearls]

The proposed patch 
(https://bz.apache.org/bugzilla/attachment.cgi?id=34012=diff) is 
fully back-ward compatible and can save a lot of useless warnings in the 
log.

Anyway to get somebody review it?

Thanks

On 06-07-2016 09:20, Nick Gearls wrote:
There's a patch (see 
https://bz.apache.org/bugzilla/show_bug.cgi?id=59660) containing both 
options (global directive + flag) for both warnings (nesting & empty 
arguments)



Good point.
A global option is not a good option as it would disable this very
useful check for all macros.

What about "On Fri, 3 Jun 2016 10:19:24 -0400, Eric Covener  
wrote:


> What syntax would be the best one? maybe a (one character) /option 
after the

> Macro keyword?
> Ex: "" (and later " (bad name) that is a very thin wrapper around
existing stuff?

Re: Unexpected Warnings from Macro Use in 2.4

[Nick Gearls]

Nobody is interested in avoiding problems with this sanity check?
Trivial to review, only a warning - but that could save hours to users


On 06-07-2016 09:25, Nick Gearls wrote:
There's a patch (see 
https://bz.apache.org/bugzilla/show_bug.cgi?id=58304) adding a warning 
in case of syntax clash with 'Define'


On 2/18/2015 6:12 AM, Tom Browder wrote:
> I have been using mod_macro for some time and always get the following
> types of messages on startup (using 2.4.12 now, but this behavior has
> been noticed since 2.4.7):
>
> [Wed Feb 18 13:54:55.019032 2015] [core:warn] [pid 970:tid
> 140069833443200] AH00111: Config variable ${PROJECT} is not defined
> [Wed Feb 18 13:54:55.019041 2015] [core:warn] [pid 970:tid
> 140069833443200] AH00111: Config variable ${TLD} is not defined
>
> For the example httpd instance only one macro is defined, used, and
> undefined like this:
>
> 
> 
> ServerName ${PROJECT}.${TLD}
> ServerAlias www.${PROJECT}.${TLD}
> DocumentRoot /home/web-sites/${PROJECT}.${TLD}/public
> 
> 
> Use VHOST_NONTLS mysite org
> UndefMacro VHOST_NONTLS
>
> The warnings I believe are spurious and should not be there. The
> virtual hosts work fine after startup. Apparently, the first time
> though the macro definitions are read and, since they are not defined,
> the warnings are produced. It seems to me that is a bug.
>
> Am I doing something wrong?

Re: 2.4.24 soon?

[Jim Jagielski]

I'd like to see us work on having a 2.4.24 out sometime this
month... Can we spend some time on existing backports and
seeing what of usefulness in trunk *can and should* be backported
to 2.4?

Thx!!

> On Sep 19, 2016, at 11:36 AM, Jim Jagielski  wrote:
> 
> 
>> On Aug 2, 2016, at 2:59 PM, Jacob Champion  wrote:
>> 
>> On 08/02/2016 11:12 AM, William A Rowe Jr wrote:
>>> One additional thought... On 2.2 and 2.4 I see this change as entirely
>>> opt-in, no disruption to a user performing a subversion upgrade. On
>>> 2.6/3.0 I'd want us to seriously consider changing the out-of-the-box
>>> default to strict parsing.
>> 
>> +1.
>> 
>> (I have no strong opinions on whether or not this should go into the next 
>> release, though.)
>> 
> 
> Any more thoughts related to this? I know that it is
> still being worked here and there, but knowing whether or
> not it will be folded in 2.4.24 might be incentive to
> finish polishing as it were.
> 

Re: Frequent wake-ups for mpm_event

[Luca Toscano]

Hi Stefan,

2016-09-26 14:26 GMT+02:00 Stefan Priebe - Profihost AG <
s.pri...@profihost.ag>:

> currently no deadlocks since V5 also no old httpd processes anymore.
>

if you still have patience and time, I'd like to ask you a question: have
you noticed any performance improvement after applying the patch? For
example (Yann correct me if I am wrong) I'd expect some reduction in system
CPU utilization since the number of wake ups / context switches has been
reduced dramatically. Moreover, it would be great to know some info about
the status of the worker threads over time from the scoreboard, since it
would be great not to introduce weird regressions with this patch :)

Thanks again!

Luca

Re: Random AH01842 errors in mod_session_crypto

[Yann Ylavic]

[Adding dev@apr, with a little abstract]

On Mon, Sep 12, 2016 at 10:31 AM, Ewald Dieterich  wrote:
>
> Looks like the problem is this:
>
> * In session_crypto_init() a crypto context is created from a global pool
> (server->pconf).
> * In encrypt_string() and decrypt_string() a key is created from the context
> via apr_crypto_passphrase() using the global pool for allocating memory for
> the key.
> * Multiple threads might access the global pool at the same time.

Unless given an already allocated key, apr_crypto_passphrase() and
apr_crypto_key() will create/allocate the key from the pool of an
apr_crypto_t (the *const* context), not the given pool.

So we tried to allocate the key before the call to apr_crypto_passphrase()...

On Tue, Oct 4, 2016 at 6:21 PM, Yann Ylavic  wrote:
> On Tue, Oct 4, 2016 at 5:29 PM, Graham Leggett 
> wrote:
>> On 4 Oct 2016, at 15:47, Paul Spangler 
>> wrote:
>>
>>> From my understanding, apr_crypto_key_t is an opaque struct
>>> defined separately by each crypto provider, so mod_session_crypto
>>> will not be able to do the sizeof.
>>
>> That's a sizeof a pointer to apr_crypto_key_t, not the sizeof
>> apr_crypto_key_t itself.
>
> I think Paul is correct, apr_crypto_passphrase() requires its given
> *(apr_crypto_key_t**)key to be not NULL, otherwise it will allocate
> one from its (providers's) array, which is not thread safe.
>
> How are we supposed to have a *key not NULL given apr_crypto_key_t is
> opaque?

... no way.

>>
>> Keys are read at server start and reused. Trying to regenerate the
>> key on every request has performance implications.

Looks like the heavy operation is creating the context (apr_crypto_t),
not the key, but anyway the keys need to be generated at run time,
taking the salt into account.

So I don't see the point of caching (and allocating) the keys in the
context while we could use the given runtime pool.

If the user wanted some cache s/he could still handle it based on
her/his own pool, but:

> It seems that apr_crypto_passphrase()'s **key is updated for each
> call, though...

so we can't really cache it anyway.

Hence I propose to remove the keys' cache in apr_crypto, and let the
things work with the given pool(s)...

Patch attached, WDYT?

Regards,
Yann.
Index: crypto/apr_crypto_commoncrypto.c
===
--- crypto/apr_crypto_commoncrypto.c	(revision 1763363)
+++ crypto/apr_crypto_commoncrypto.c	(working copy)
@@ -41,7 +41,6 @@ struct apr_crypto_t
 apr_pool_t *pool;
 const apr_crypto_driver_t *provider;
 apu_err_t *result;
-apr_array_header_t *keys;
 apr_hash_t *types;
 apr_hash_t *modes;
 apr_random_t *rng;
@@ -206,11 +205,6 @@ static apr_status_t crypto_make(apr_crypto_t **ff,
 return APR_ENOMEM;
 }
 
-f->keys = apr_array_make(pool, 10, sizeof(apr_crypto_key_t));
-if (!f->keys) {
-return APR_ENOMEM;
-}
-
 f->types = apr_hash_make(pool);
 if (!f->types) {
 return APR_ENOMEM;
@@ -388,7 +382,7 @@ static apr_status_t crypto_key(apr_crypto_key_t **
 apr_crypto_key_t *key = *k;
 
 if (!key) {
-*k = key = apr_array_push(f->keys);
+*k = key = apr_pcalloc(p, sizeof *key);
 }
 if (!key) {
 return APR_ENOMEM;
@@ -480,11 +474,11 @@ static apr_status_t crypto_passphrase(apr_crypto_k
 apr_crypto_key_t *key = *k;
 
 if (!key) {
-*k = key = apr_array_push(f->keys);
+*k = key = apr_pcalloc(p, sizeof *key);
+if (!key) {
+return APR_ENOMEM;
+}
 }
-if (!key) {
-return APR_ENOMEM;
-}
 
 key->f = f;
 key->provider = f->provider;
Index: crypto/apr_crypto_nss.c
===
--- crypto/apr_crypto_nss.c	(revision 1763363)
+++ crypto/apr_crypto_nss.c	(working copy)
@@ -50,7 +50,6 @@ struct apr_crypto_t {
 apr_pool_t *pool;
 const apr_crypto_driver_t *provider;
 apu_err_t *result;
-apr_array_header_t *keys;
 apr_crypto_config_t *config;
 apr_hash_t *types;
 apr_hash_t *modes;
@@ -266,6 +265,15 @@ static apr_status_t crypto_block_cleanup_helper(vo
 return crypto_block_cleanup(block);
 }
 
+static apr_status_t crypto_key_cleanup(void *data)
+{
+apr_crypto_key_t *key = data;
+if (key->symKey) {
+PK11_FreeSymKey(key->symKey);
+key->symKey = NULL;
+}
+return APR_SUCCESS;
+}
 /**
  * @brief Clean encryption / decryption context.
  * @note After cleanup, a context is free to be reused if necessary.
@@ -274,15 +282,6 @@ static apr_status_t crypto_block_cleanup_helper(vo
  */
 static apr_status_t crypto_cleanup(apr_crypto_t *f)
 {
-apr_crypto_key_t *key;
-if (f->keys) {
-while ((key = apr_array_pop(f->keys))) {
-if (key->symKey) {
-PK11_FreeSymKey(key->symKey);
-key->symKey =