SSLSrvConfigRec shared

[Stefan Eissing <st...@greenbytes.de>]

mod_ssl's server_rec configurations (SSLSrvConfigRec) are shared between vhost and base server *iff* there are no SSL* directives used inside a VirtualHost. This is not really a good idea since mod_ssl modifies these recs in its post_config hook. This looks currently harmless, e.g. setting sc->vhost_id n times (but the vhost_id is wrong for all but the last). With adding certificate/keys in post-config (mod_md) this sharing can no longer happen.

To be precise: this is a side effect of a global "SSLEngine" config. The old-skool "SSLEngine on" in each vhost will cause every server_rec to have its own SSLSrvConfigRec instance and things work.

Now, I would like both cases to work. Does anyone have a recommendation? My current thoughts go like (pseudo code):

if (server != base_server && sslconf(server) == sslconf(base_server)) {
   newconf = conf_merge(new_server_conf(), sslconf(base_server));
   ap_set_module_config(server, newconf);
}

Is there some better way?

Cheers,

Stefan

Re: SSLSrvConfigRec shared

[Yann Ylavic <yl...@gmail.com>]

On Sat, Dec 23, 2017 at 3:53 PM, Stefan Eissing
<st...@greenbytes.de> wrote:
>
> I think this really is a bug in our runtime. The flag was invented
> just for precaution, in case a module would rely on the no-copy
> behaviour. That is really close to saying: we will not fix
> any more bugs in 2.4.x as someone could rely on it.

I would be fine with duplicating/merging every server config by
default, even when no module directive is specified, the risk is that
some (third party) merger functions might not do the right thing when
called in this situation, and could break.
Too much caution possibly...

Re: SSLSrvConfigRec shared

[Eric Covener <co...@gmail.com>]

> I think this really is a bug in our runtime. The flag was invented
> just for precaution, in case a module would rely on the no-copy
> behaviour. That is really close to saying: we will not fix
> any more bugs in 2.4.x as someone could rely on it.

I don't think this bit is fair. Every config section behaves the same
-- modules get a new configuration when their directives are used in
the section.

Re: SSLSrvConfigRec shared

[Stefan Eissing <st...@greenbytes.de>]

> Am 23.12.2017 um 12:34 schrieb Yann Ylavic <yl...@gmail.com>:
> 
> On Sat, Dec 23, 2017 at 9:00 AM, Nick Kew <ni...@apache.org> wrote:
>> On Sat, 2017-12-23 at 08:20 +0100, Stefan Eissing wrote:
>> 
>>>> Ugh.  Fine for trunk, but that's a lot of complexity to foist on
>>>> a stable branch.  A module would not only need to check MMN,
>>>> but also implement fallback behaviour if there are no flags.
>>>> So why not KISS and stick with that fallback for all 2.4?
>>> 
>>> Not sure, I parse that. Any module that does nothing continues to
>>> see the same behaviour than before. Where does the complexity
>>> arrive?
>> 
>> But a module cannot ever *use* it without checking MMN
>> *AND* implementing fallback behaviour for being loaded
>> into an httpd built with the old struct - and consequently
>> old API and ABI.
>> 
>> That's bad enough to work through once, let alone maintain longer-term!
>> 
>> Whereas the fallback, by definition, works in all cases.
> 
> Right, but hence nothing prevents the modules from implementing the
> fallback only, just as if "flags" were not in 2.4.x.
> It's not like depending on the MMN is mandatory.
> 
> It seems that no module really needed it until mod_md, or they already
> implemented the fallback.
> If we make it to 2.4.next for mod_md needs (which anyway requires
> mod_ssl 2.4.next), at least it simplifies the code in mod_ssl...

Any module that writes to its config records in a post_config hook
is in danger of having shared instances between vhosts. Only when
directives of the module in the vhost are used, do they have
a unique config record instance.

There is workaround code in mod_md and I was writing that for mod_ssl
as Yann came with this flag solution. So, current mod_ssl does not
have a workaround and will not work in case someone uses the 
SSLEngine *:443 and uses no SSL* directives in vhosts.

If "SSLEngine on" or other directives are used, that vhost has a
unique instance of its mod_ssl config record. Very subtle.

I think this really is a bug in our runtime. The flag was invented
just for precaution, in case a module would rely on the no-copy
behaviour. That is really close to saying: we will not fix
any more bugs in 2.4.x as someone could rely on it.

>> Agreed, post-config per-server stuff is clumsy: I have a distant
>> memory from 2.0 days of hacking some ugly workaround, though the
>> details elude me.  But wouldn't it make more sense to review that
>> in 2.5/trunk rather than the stable branch?
> 
> It shouldn't hurt in 2.4, IMHO.

I agree. I also think the bugfix without flag could have gone to 2.4.

Re: SSLSrvConfigRec shared

[Yann Ylavic <yl...@gmail.com>]

On Sat, Dec 23, 2017 at 9:00 AM, Nick Kew <ni...@apache.org> wrote:
> On Sat, 2017-12-23 at 08:20 +0100, Stefan Eissing wrote:
>
>> > Ugh.  Fine for trunk, but that's a lot of complexity to foist on
>> > a stable branch.  A module would not only need to check MMN,
>> > but also implement fallback behaviour if there are no flags.
>> > So why not KISS and stick with that fallback for all 2.4?
>>
>> Not sure, I parse that. Any module that does nothing continues to
>> see the same behaviour than before. Where does the complexity
>> arrive?
>
> But a module cannot ever *use* it without checking MMN
> *AND* implementing fallback behaviour for being loaded
> into an httpd built with the old struct - and consequently
> old API and ABI.
>
> That's bad enough to work through once, let alone maintain longer-term!
>
> Whereas the fallback, by definition, works in all cases.

Right, but hence nothing prevents the modules from implementing the
fallback only, just as if "flags" were not in 2.4.x.
It's not like depending on the MMN is mandatory.

It seems that no module really needed it until mod_md, or they already
implemented the fallback.
If we make it to 2.4.next for mod_md needs (which anyway requires
mod_ssl 2.4.next), at least it simplifies the code in mod_ssl...

>
> Agreed, post-config per-server stuff is clumsy: I have a distant
> memory from 2.0 days of hacking some ugly workaround, though the
> details elude me.  But wouldn't it make more sense to review that
> in 2.5/trunk rather than the stable branch?

It shouldn't hurt in 2.4, IMHO.


Regards,
Yann.

Re: SSLSrvConfigRec shared

[Nick Kew <ni...@apache.org>]

On Sat, 2017-12-23 at 08:20 +0100, Stefan Eissing wrote:

> > Ugh.  Fine for trunk, but that's a lot of complexity to foist on
> > a stable branch.  A module would not only need to check MMN,
> > but also implement fallback behaviour if there are no flags.
> > So why not KISS and stick with that fallback for all 2.4?
> 
> Not sure, I parse that. Any module that does nothing continues to see the same behaviour than before. Where does the complexity arrive?

But a module cannot ever *use* it without checking MMN
*AND* implementing fallback behaviour for being loaded
into an httpd built with the old struct - and consequently
old API and ABI.

That's bad enough to work through once, let alone maintain longer-term!

Whereas the fallback, by definition, works in all cases.

Agreed, post-config per-server stuff is clumsy: I have a distant
memory from 2.0 days of hacking some ugly workaround, though the
details elude me.  But wouldn't it make more sense to review that
in 2.5/trunk rather than the stable branch?

-- 
Nick Kew

Re: SSLSrvConfigRec shared

[Stefan Eissing <st...@greenbytes.de>]

> Am 22.12.2017 um 23:31 schrieb Nick Kew <ni...@apache.org>:
> 
> On Thu, 21 Sep 2017 08:11:17 -0400
> Eric Covener <co...@gmail.com> wrote:
> 
>> IIUC it should be safe to extend module_struct with a minor bump to
>> add 'int flags' to the bottom, but when you check the value you'd need
>> to check the MMN first. In the module you'd then just have some flags
>> or'ed together after register_hooks.
> 
> Sorry to come to this late: arises from reviewing STATUS.
> 
> Ugh.  Fine for trunk, but that's a lot of complexity to foist on
> a stable branch.  A module would not only need to check MMN,
> but also implement fallback behaviour if there are no flags.
> So why not KISS and stick with that fallback for all 2.4?

Not sure, I parse that. Any module that does nothing continues to see the same behaviour than before. Where does the complexity arrive?

-Stefan

> 
> -- 
> Nick Kew

Re: SSLSrvConfigRec shared

[Nick Kew <ni...@apache.org>]

On Thu, 21 Sep 2017 08:11:17 -0400
Eric Covener <co...@gmail.com> wrote:

> IIUC it should be safe to extend module_struct with a minor bump to
> add 'int flags' to the bottom, but when you check the value you'd need
> to check the MMN first. In the module you'd then just have some flags
> or'ed together after register_hooks.

Sorry to come to this late: arises from reviewing STATUS.

Ugh.  Fine for trunk, but that's a lot of complexity to foist on
a stable branch.  A module would not only need to check MMN,
but also implement fallback behaviour if there are no flags.
So why not KISS and stick with that fallback for all 2.4?

-- 
Nick Kew

Re: SSLSrvConfigRec shared

[Stefan Eissing <st...@greenbytes.de>]

posticipate - realizing, while one writes a reply, that Yann has probably already implemented it.

X-)

> Am 22.09.2017 um 14:48 schrieb Eric Covener <co...@gmail.com>:
> 
> Whoops I see you already folllowed it up.
> 
> On Fri, Sep 22, 2017 at 8:46 AM, Eric Covener <co...@gmail.com> wrote:
>> On Fri, Sep 22, 2017 at 8:11 AM, Yann Ylavic <yl...@gmail.com> wrote:
>>> On Thu, Sep 21, 2017 at 2:51 PM, Eric Covener <co...@gmail.com> wrote:
>>>> On Thu, Sep 21, 2017 at 8:44 AM, Yann Ylavic <yl...@gmail.com> wrote:
>>>>> On Thu, Sep 21, 2017 at 2:11 PM, Eric Covener <co...@gmail.com> wrote:
>>>>>> 
>>>>>> IIUC it should be safe to extend module_struct with a minor bump to
>>>>>> add 'int flags' to the bottom, but when you check the value you'd need
>>>>>> to check the MMN first. In the module you'd then just have some flags
>>>>>> or'ed together after register_hooks.
>>>>> 
>>>>> Something like the attached patch might do it (untested, no MMN minor bump).
>>>>> 
>>>>>> 
>>>>>> (hopefully someone will check my work)
>>>>> 
>>>>> Since modules (module_struct) are déclared globally, unspecified
>>>>> fields at the end of the struct should be initialized to zero, so it
>>>>> should be safe.
>>>> 
>>>> I was thinking about modules compiled against the previous definition
>>>> / out of tree.
>>> 
>>> Hmm, I'm not sure my commits address this.
>>> 
>>> The modules would be run by the latest core without being recompiled
>>> against it, that's the case?
>> 
>> Yes, I think it not yet handled because you're checking the cores MMN
>> # at compile time.
>> 
>> I think we need an accessor or macro to retrieve the flags that looks
>> at the module_struct being evaluated which I think also has their
>> compile-time MMN baked in.  Probably best to have this be a simple
>> function rather than a macro.
>> 
>> 
>> --
>> Eric Covener
>> covener@gmail.com
> 
> 
> 
> -- 
> Eric Covener
> covener@gmail.com

Re: SSLSrvConfigRec shared

[Yann Ylavic <yl...@gmail.com>]

I added the 'flags' getter in r1809311, much cleaner, thanks!

On Fri, Sep 22, 2017 at 2:48 PM, Eric Covener <co...@gmail.com> wrote:
> Whoops I see you already folllowed it up.
>
> On Fri, Sep 22, 2017 at 8:46 AM, Eric Covener <co...@gmail.com> wrote:
>> On Fri, Sep 22, 2017 at 8:11 AM, Yann Ylavic <yl...@gmail.com> wrote:
>>> On Thu, Sep 21, 2017 at 2:51 PM, Eric Covener <co...@gmail.com> wrote:
>>>> On Thu, Sep 21, 2017 at 8:44 AM, Yann Ylavic <yl...@gmail.com> wrote:
>>>>> On Thu, Sep 21, 2017 at 2:11 PM, Eric Covener <co...@gmail.com> wrote:
>>>>>>
>>>>>> IIUC it should be safe to extend module_struct with a minor bump to
>>>>>> add 'int flags' to the bottom, but when you check the value you'd need
>>>>>> to check the MMN first. In the module you'd then just have some flags
>>>>>> or'ed together after register_hooks.
>>>>>
>>>>> Something like the attached patch might do it (untested, no MMN minor bump).
>>>>>
>>>>>>
>>>>>> (hopefully someone will check my work)
>>>>>
>>>>> Since modules (module_struct) are déclared globally, unspecified
>>>>> fields at the end of the struct should be initialized to zero, so it
>>>>> should be safe.
>>>>
>>>> I was thinking about modules compiled against the previous definition
>>>> / out of tree.
>>>
>>> Hmm, I'm not sure my commits address this.
>>>
>>> The modules would be run by the latest core without being recompiled
>>> against it, that's the case?
>>
>> Yes, I think it not yet handled because you're checking the cores MMN
>> # at compile time.
>>
>> I think we need an accessor or macro to retrieve the flags that looks
>> at the module_struct being evaluated which I think also has their
>> compile-time MMN baked in.  Probably best to have this be a simple
>> function rather than a macro.
>>
>>
>> --
>> Eric Covener
>> covener@gmail.com
>
>
>
> --
> Eric Covener
> covener@gmail.com

Re: SSLSrvConfigRec shared

[Eric Covener <co...@gmail.com>]

Whoops I see you already folllowed it up.

On Fri, Sep 22, 2017 at 8:46 AM, Eric Covener <co...@gmail.com> wrote:
> On Fri, Sep 22, 2017 at 8:11 AM, Yann Ylavic <yl...@gmail.com> wrote:
>> On Thu, Sep 21, 2017 at 2:51 PM, Eric Covener <co...@gmail.com> wrote:
>>> On Thu, Sep 21, 2017 at 8:44 AM, Yann Ylavic <yl...@gmail.com> wrote:
>>>> On Thu, Sep 21, 2017 at 2:11 PM, Eric Covener <co...@gmail.com> wrote:
>>>>>
>>>>> IIUC it should be safe to extend module_struct with a minor bump to
>>>>> add 'int flags' to the bottom, but when you check the value you'd need
>>>>> to check the MMN first. In the module you'd then just have some flags
>>>>> or'ed together after register_hooks.
>>>>
>>>> Something like the attached patch might do it (untested, no MMN minor bump).
>>>>
>>>>>
>>>>> (hopefully someone will check my work)
>>>>
>>>> Since modules (module_struct) are déclared globally, unspecified
>>>> fields at the end of the struct should be initialized to zero, so it
>>>> should be safe.
>>>
>>> I was thinking about modules compiled against the previous definition
>>> / out of tree.
>>
>> Hmm, I'm not sure my commits address this.
>>
>> The modules would be run by the latest core without being recompiled
>> against it, that's the case?
>
> Yes, I think it not yet handled because you're checking the cores MMN
> # at compile time.
>
> I think we need an accessor or macro to retrieve the flags that looks
> at the module_struct being evaluated which I think also has their
> compile-time MMN baked in.  Probably best to have this be a simple
> function rather than a macro.
>
>
> --
> Eric Covener
> covener@gmail.com



-- 
Eric Covener
covener@gmail.com

Re: SSLSrvConfigRec shared

[Eric Covener <co...@gmail.com>]

On Fri, Sep 22, 2017 at 8:11 AM, Yann Ylavic <yl...@gmail.com> wrote:
> On Thu, Sep 21, 2017 at 2:51 PM, Eric Covener <co...@gmail.com> wrote:
>> On Thu, Sep 21, 2017 at 8:44 AM, Yann Ylavic <yl...@gmail.com> wrote:
>>> On Thu, Sep 21, 2017 at 2:11 PM, Eric Covener <co...@gmail.com> wrote:
>>>>
>>>> IIUC it should be safe to extend module_struct with a minor bump to
>>>> add 'int flags' to the bottom, but when you check the value you'd need
>>>> to check the MMN first. In the module you'd then just have some flags
>>>> or'ed together after register_hooks.
>>>
>>> Something like the attached patch might do it (untested, no MMN minor bump).
>>>
>>>>
>>>> (hopefully someone will check my work)
>>>
>>> Since modules (module_struct) are déclared globally, unspecified
>>> fields at the end of the struct should be initialized to zero, so it
>>> should be safe.
>>
>> I was thinking about modules compiled against the previous definition
>> / out of tree.
>
> Hmm, I'm not sure my commits address this.
>
> The modules would be run by the latest core without being recompiled
> against it, that's the case?

Yes, I think it not yet handled because you're checking the cores MMN
# at compile time.

I think we need an accessor or macro to retrieve the flags that looks
at the module_struct being evaluated which I think also has their
compile-time MMN baked in.  Probably best to have this be a simple
function rather than a macro.


-- 
Eric Covener
covener@gmail.com

Re: SSLSrvConfigRec shared

[Yann Ylavic <yl...@gmail.com>]

On Thu, Sep 21, 2017 at 2:51 PM, Eric Covener <co...@gmail.com> wrote:
> On Thu, Sep 21, 2017 at 8:44 AM, Yann Ylavic <yl...@gmail.com> wrote:
>> On Thu, Sep 21, 2017 at 2:11 PM, Eric Covener <co...@gmail.com> wrote:
>>>
>>> IIUC it should be safe to extend module_struct with a minor bump to
>>> add 'int flags' to the bottom, but when you check the value you'd need
>>> to check the MMN first. In the module you'd then just have some flags
>>> or'ed together after register_hooks.
>>
>> Something like the attached patch might do it (untested, no MMN minor bump).
>>
>>>
>>> (hopefully someone will check my work)
>>
>> Since modules (module_struct) are déclared globally, unspecified
>> fields at the end of the struct should be initialized to zero, so it
>> should be safe.
>
> I was thinking about modules compiled against the previous definition
> / out of tree.

Hmm, I'm not sure my commits address this.

The modules would be run by the latest core without being recompiled
against it, that's the case?

Re: SSLSrvConfigRec shared

[Stefan Eissing <st...@greenbytes.de>]

The patches look great! Will test on next occasion! Thanks! :)

> Am 22.09.2017 um 14:02 schrieb Yann Ylavic <yl...@gmail.com>:
> 
> On Thu, Sep 21, 2017 at 2:54 PM, Yann Ylavic <yl...@gmail.com> wrote:
>> On Thu, Sep 21, 2017 at 2:51 PM, Eric Covener <co...@gmail.com> wrote:
>>> On Thu, Sep 21, 2017 at 8:44 AM, Yann Ylavic <yl...@gmail.com> wrote:
>>>> On Thu, Sep 21, 2017 at 2:11 PM, Eric Covener <co...@gmail.com> wrote:
>>>>> 
>>>>> IIUC it should be safe to extend module_struct with a minor bump to
>>>>> add 'int flags' to the bottom, but when you check the value you'd need
>>>>> to check the MMN first. In the module you'd then just have some flags
>>>>> or'ed together after register_hooks.
>>>> 
>>>> Something like the attached patch might do it (untested, no MMN minor bump).
>>>> 
>>>>> 
>>>>> (hopefully someone will check my work)
>>>> 
>>>> Since modules (module_struct) are déclared globally, unspecified
>>>> fields at the end of the struct should be initialized to zero, so it
>>>> should be safe.
>>> 
>>> I was thinking about modules compiled against the previous definition
>>> / out of tree.
>> 
>> Right, it's missing some #ifdefs MMN too :)
> 
> r1809302 and r1809303, does it work for you Stefan?
> 
> We have to change the AP_MODULE_HAS_FLAGS macro with the appropriate
> MMN if we'd backport...

Re: SSLSrvConfigRec shared

[Yann Ylavic <yl...@gmail.com>]

On Thu, Sep 21, 2017 at 2:54 PM, Yann Ylavic <yl...@gmail.com> wrote:
> On Thu, Sep 21, 2017 at 2:51 PM, Eric Covener <co...@gmail.com> wrote:
>> On Thu, Sep 21, 2017 at 8:44 AM, Yann Ylavic <yl...@gmail.com> wrote:
>>> On Thu, Sep 21, 2017 at 2:11 PM, Eric Covener <co...@gmail.com> wrote:
>>>>
>>>> IIUC it should be safe to extend module_struct with a minor bump to
>>>> add 'int flags' to the bottom, but when you check the value you'd need
>>>> to check the MMN first. In the module you'd then just have some flags
>>>> or'ed together after register_hooks.
>>>
>>> Something like the attached patch might do it (untested, no MMN minor bump).
>>>
>>>>
>>>> (hopefully someone will check my work)
>>>
>>> Since modules (module_struct) are déclared globally, unspecified
>>> fields at the end of the struct should be initialized to zero, so it
>>> should be safe.
>>
>> I was thinking about modules compiled against the previous definition
>> / out of tree.
>
> Right, it's missing some #ifdefs MMN too :)

r1809302 and r1809303, does it work for you Stefan?

We have to change the AP_MODULE_HAS_FLAGS macro with the appropriate
MMN if we'd backport...

Re: SSLSrvConfigRec shared

[Yann Ylavic <yl...@gmail.com>]

On Thu, Sep 21, 2017 at 2:51 PM, Eric Covener <co...@gmail.com> wrote:
> On Thu, Sep 21, 2017 at 8:44 AM, Yann Ylavic <yl...@gmail.com> wrote:
>> On Thu, Sep 21, 2017 at 2:11 PM, Eric Covener <co...@gmail.com> wrote:
>>>
>>> IIUC it should be safe to extend module_struct with a minor bump to
>>> add 'int flags' to the bottom, but when you check the value you'd need
>>> to check the MMN first. In the module you'd then just have some flags
>>> or'ed together after register_hooks.
>>
>> Something like the attached patch might do it (untested, no MMN minor bump).
>>
>>>
>>> (hopefully someone will check my work)
>>
>> Since modules (module_struct) are déclared globally, unspecified
>> fields at the end of the struct should be initialized to zero, so it
>> should be safe.
>
> I was thinking about modules compiled against the previous definition
> / out of tree.

Right, it's missing some #ifdefs MMN too :)

Re: SSLSrvConfigRec shared

[Eric Covener <co...@gmail.com>]

On Thu, Sep 21, 2017 at 8:44 AM, Yann Ylavic <yl...@gmail.com> wrote:
> On Thu, Sep 21, 2017 at 2:11 PM, Eric Covener <co...@gmail.com> wrote:
>>
>> IIUC it should be safe to extend module_struct with a minor bump to
>> add 'int flags' to the bottom, but when you check the value you'd need
>> to check the MMN first. In the module you'd then just have some flags
>> or'ed together after register_hooks.
>
> Something like the attached patch might do it (untested, no MMN minor bump).
>
>>
>> (hopefully someone will check my work)
>
> Since modules (module_struct) are déclared globally, unspecified
> fields at the end of the struct should be initialized to zero, so it
> should be safe.

I was thinking about modules compiled against the previous definition
/ out of tree.


-- 
Eric Covener
covener@gmail.com

Re: SSLSrvConfigRec shared

[Yann Ylavic <yl...@gmail.com>]

On Thu, Sep 21, 2017 at 2:11 PM, Eric Covener <co...@gmail.com> wrote:
>
> IIUC it should be safe to extend module_struct with a minor bump to
> add 'int flags' to the bottom, but when you check the value you'd need
> to check the MMN first. In the module you'd then just have some flags
> or'ed together after register_hooks.

Something like the attached patch might do it (untested, no MMN minor bump).

>
> (hopefully someone will check my work)

Since modules (module_struct) are déclared globally, unspecified
fields at the end of the struct should be initialized to zero, so it
should be safe.

Re: SSLSrvConfigRec shared

[Eric Covener <co...@gmail.com>]

On Thu, Sep 21, 2017 at 7:42 AM, Stefan Eissing
<st...@greenbytes.de> wrote:
>
>> Am 21.09.2017 um 13:35 schrieb Eric Covener <co...@gmail.com>:
>>
>> On Thu, Sep 21, 2017 at 7:00 AM, Yann Ylavic <yl...@gmail.com> wrote:
>>> On Thu, Sep 21, 2017 at 11:48 AM, Stefan Eissing
>>> <st...@greenbytes.de> wrote:
>>>>
>>>>> Am 21.09.2017 um 11:37 schrieb Yann Ylavic <yl...@gmail.com>:
>>>>>
>>>>> If the module defines its own server_config_create() which allocates
>>>>> one, each vhost will have its own, and the module's
>>>>> server_config_merge() can do whatever needs to for the members of the
>>>>> config (pointer copy, shallow/deep copy, ...).
>>>>
>>>> Yes, but only *iff* there is every a directive of that module used in
>>>> a VirtualHost.
>>>
>>> OK, I see know, thanks.
>>>
>>> I'd call that a premature optimization though, even if it matured for decades :)
>>> Only the module knows what to do when merging, so I think the core
>>> config should still call config_create() and config_merge() for those,
>>> precisely because post_config() is always called.
>>> Modules also know how to merge configs that do nothing (usually), with
>>> all those _set members all over the place, so it should work (untested
>>> ;) even if it may consume more (not that much I think) initial memory
>>> for large confs.
>>
>> Maybe less intrusive for a module to make a copy of its config if it
>> detects base/vh are the same and needs them to differ?
>
> That is why I "fixed" this for mod_ssl only. But mod_md has similar code now.
>
> Do we have some sort of bitfield of flags for each module? That would be great
> as we could make an opt-in change that way.
>

IIUC it should be safe to extend module_struct with a minor bump to
add 'int flags' to the bottom, but when you check the value you'd need
to check the MMN first. In the module you'd then just have some flags
or'ed together after register_hooks.

(hopefully someone will check my work)

-- 
Eric Covener
covener@gmail.com

Re: SSLSrvConfigRec shared

[Stefan Eissing <st...@greenbytes.de>]

> Am 21.09.2017 um 13:35 schrieb Eric Covener <co...@gmail.com>:
> 
> On Thu, Sep 21, 2017 at 7:00 AM, Yann Ylavic <yl...@gmail.com> wrote:
>> On Thu, Sep 21, 2017 at 11:48 AM, Stefan Eissing
>> <st...@greenbytes.de> wrote:
>>> 
>>>> Am 21.09.2017 um 11:37 schrieb Yann Ylavic <yl...@gmail.com>:
>>>> 
>>>> If the module defines its own server_config_create() which allocates
>>>> one, each vhost will have its own, and the module's
>>>> server_config_merge() can do whatever needs to for the members of the
>>>> config (pointer copy, shallow/deep copy, ...).
>>> 
>>> Yes, but only *iff* there is every a directive of that module used in
>>> a VirtualHost.
>> 
>> OK, I see know, thanks.
>> 
>> I'd call that a premature optimization though, even if it matured for decades :)
>> Only the module knows what to do when merging, so I think the core
>> config should still call config_create() and config_merge() for those,
>> precisely because post_config() is always called.
>> Modules also know how to merge configs that do nothing (usually), with
>> all those _set members all over the place, so it should work (untested
>> ;) even if it may consume more (not that much I think) initial memory
>> for large confs.
> 
> Maybe less intrusive for a module to make a copy of its config if it
> detects base/vh are the same and needs them to differ?

That is why I "fixed" this for mod_ssl only. But mod_md has similar code now.

Do we have some sort of bitfield of flags for each module? That would be great
as we could make an opt-in change that way.

-Stefan

Re: SSLSrvConfigRec shared

[Eric Covener <co...@gmail.com>]

On Thu, Sep 21, 2017 at 7:00 AM, Yann Ylavic <yl...@gmail.com> wrote:
> On Thu, Sep 21, 2017 at 11:48 AM, Stefan Eissing
> <st...@greenbytes.de> wrote:
>>
>>> Am 21.09.2017 um 11:37 schrieb Yann Ylavic <yl...@gmail.com>:
>>>
>>> If the module defines its own server_config_create() which allocates
>>> one, each vhost will have its own, and the module's
>>> server_config_merge() can do whatever needs to for the members of the
>>> config (pointer copy, shallow/deep copy, ...).
>>
>> Yes, but only *iff* there is every a directive of that module used in
>> a VirtualHost.
>
> OK, I see know, thanks.
>
> I'd call that a premature optimization though, even if it matured for decades :)
> Only the module knows what to do when merging, so I think the core
> config should still call config_create() and config_merge() for those,
> precisely because post_config() is always called.
> Modules also know how to merge configs that do nothing (usually), with
> all those _set members all over the place, so it should work (untested
> ;) even if it may consume more (not that much I think) initial memory
> for large confs.

Maybe less intrusive for a module to make a copy of its config if it
detects base/vh are the same and needs them to differ?

Re: SSLSrvConfigRec shared

[Yann Ylavic <yl...@gmail.com>]

On Thu, Sep 21, 2017 at 11:48 AM, Stefan Eissing
<st...@greenbytes.de> wrote:
>
>> Am 21.09.2017 um 11:37 schrieb Yann Ylavic <yl...@gmail.com>:
>>
>> If the module defines its own server_config_create() which allocates
>> one, each vhost will have its own, and the module's
>> server_config_merge() can do whatever needs to for the members of the
>> config (pointer copy, shallow/deep copy, ...).
>
> Yes, but only *iff* there is every a directive of that module used in
> a VirtualHost.

OK, I see know, thanks.

I'd call that a premature optimization though, even if it matured for decades :)
Only the module knows what to do when merging, so I think the core
config should still call config_create() and config_merge() for those,
precisely because post_config() is always called.
Modules also know how to merge configs that do nothing (usually), with
all those _set members all over the place, so it should work (untested
;) even if it may consume more (not that much I think) initial memory
for large confs.

>
> The assumption here is that module configs at that point-in-time are
> read-only. Which is not really true since several modules make changes
> in the post_config phase that happens afterwards.

Agreed.

>
> My fix for this in mod_ssl is part of
>   http://svn.apache.org/viewvc?view=revision&revision=1809037
> which calls ssl_config_server_uniq() once at the post_init phase
> for each server.

Or maybe do the right thing (IMHO) in core.

>>>
>>> Now, are you speaking of changing that for all modules? Add a flag to "struct module"
>>> or solve it in mod_ssl post_config?
>>
>> Of course not, create/merge process per module can already do that, up
>> to the module to do what it needs (correctly).

Finally, maybe :)
It could be "suprising" for any module.

Let's see what others think...

Re: SSLSrvConfigRec shared

[Stefan Eissing <st...@greenbytes.de>]

> Am 21.09.2017 um 11:37 schrieb Yann Ylavic <yl...@gmail.com>:
> 
> Hi Stefan,
> 
> On Wed, Sep 20, 2017 at 2:06 PM, Stefan Eissing
> <st...@greenbytes.de> wrote:
>> 
>>> Am 20.09.2017 um 12:33 schrieb Yann Ylavic <yl...@gmail.com>:
>>> 
>>> On Wed, Sep 20, 2017 at 12:09 PM, Stefan Eissing
>>> <st...@greenbytes.de> wrote:
>>>> 
>>>> Is there some better way?
>>> 
>>> I would go with the usual/unconditional per server config (and hence
>>> merging), trade simplicity vs a few memory space...
>> 
>> Not sure I get your dift here.
> 
> I think you lost me too :)
> 
> Let's try to get in sync...

Sure! :D

>> 
>> server/config.c calls merge_server_configs() for each non-base server_rec
>> and that one copies the config pointer from base if the vhost has none.
>> if there is one, it merges.
> 
> If the module defines its own server_config_create() which allocates
> one, each vhost will have its own, and the module's
> server_config_merge() can do whatever needs to for the members of the
> config (pointer copy, shallow/deep copy, ...).

Yes, but only *iff* there is every a directive of that module used in
a VirtualHost.

> I didn't re-looked at the mod_ssl config code lately, I remember some
> special treatment for main server config (re process->pool), but it
> seems to me that each vhost has its own config, and that the merge
> process issues copies (shallow?).

mod_ssl does nothing special here, just normal module stuff.

The special thing happens in server/config.c: merge_server_configs() 
that goes over all modules and all virtual servers and either merges
the modules config *or* just assigns the pointer from the
base_server's one.

The assumption here is that module configs at that point-in-time are
read-only. Which is not really true since several modules make changes
in the post_config phase that happens afterwards.

My fix for this in mod_ssl is part of 
  http://svn.apache.org/viewvc?view=revision&revision=1809037
which calls ssl_config_server_uniq() once at the post_init phase
for each server.

Cheers,

Stefan

> What I don't remember is some modifications of the server configs in
> place in post_config, if that's the case it should indeed be preceded
> by a deep copy sowehow...
> 
>> 
>> Now, are you speaking of changing that for all modules? Add a flag to "struct module"
>> or solve it in mod_ssl post_config?
> 
> Of course not, create/merge process per module can already do that, up
> to the module to do what it needs (correctly).
> 
> 
> Regards,
> Yann.

Re: SSLSrvConfigRec shared

[Yann Ylavic <yl...@gmail.com>]

Hi Stefan,

On Wed, Sep 20, 2017 at 2:06 PM, Stefan Eissing
<st...@greenbytes.de> wrote:
>
>> Am 20.09.2017 um 12:33 schrieb Yann Ylavic <yl...@gmail.com>:
>>
>> On Wed, Sep 20, 2017 at 12:09 PM, Stefan Eissing
>> <st...@greenbytes.de> wrote:
>>>
>>> Is there some better way?
>>
>> I would go with the usual/unconditional per server config (and hence
>> merging), trade simplicity vs a few memory space...
>
> Not sure I get your dift here.

I think you lost me too :)

Let's try to get in sync...

>
> server/config.c calls merge_server_configs() for each non-base server_rec
> and that one copies the config pointer from base if the vhost has none.
> if there is one, it merges.

If the module defines its own server_config_create() which allocates
one, each vhost will have its own, and the module's
server_config_merge() can do whatever needs to for the members of the
config (pointer copy, shallow/deep copy, ...).

I didn't re-looked at the mod_ssl config code lately, I remember some
special treatment for main server config (re process->pool), but it
seems to me that each vhost has its own config, and that the merge
process issues copies (shallow?).

What I don't remember is some modifications of the server configs in
place in post_config, if that's the case it should indeed be preceded
by a deep copy sowehow...

>
> Now, are you speaking of changing that for all modules? Add a flag to "struct module"
> or solve it in mod_ssl post_config?

Of course not, create/merge process per module can already do that, up
to the module to do what it needs (correctly).


Regards,
Yann.

Re: SSLSrvConfigRec shared

[Stefan Eissing <st...@greenbytes.de>]

> Am 20.09.2017 um 12:33 schrieb Yann Ylavic <yl...@gmail.com>:
> 
> On Wed, Sep 20, 2017 at 12:09 PM, Stefan Eissing
> <st...@greenbytes.de> wrote:
>> 
>> Is there some better way?
> 
> I would go with the usual/unconditional per server config (and hence
> merging), trade simplicity vs a few memory space...

Not sure I get your dift here.

server/config.c calls merge_server_configs() for each non-base server_rec
and that one copies the config pointer from base if the vhost has none. 
if there is one, it merges.

Now, are you speaking of changing that for all modules? Add a flag to "struct module"
or solve it in mod_ssl post_config?

Cheers,
Stefan


> Regards,
> Yann.

Re: SSLSrvConfigRec shared

[Yann Ylavic <yl...@gmail.com>]

On Wed, Sep 20, 2017 at 12:09 PM, Stefan Eissing
<st...@greenbytes.de> wrote:
>
> Is there some better way?

I would go with the usual/unconditional per server config (and hence
merging), trade simplicity vs a few memory space...

Regards,
Yann.