You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by wr...@apache.org on 2022/01/13 03:28:31 UTC
svn commit: r1896976 - /httpd/httpd/branches/2.4.x/STATUS
Author: wrowe
Date: Thu Jan 13 03:28:31 2022
New Revision: 1896976
URL: http://svn.apache.org/viewvc?rev=1896976&view=rev
Log:
Propose pcre2 support for backport
Modified:
httpd/httpd/branches/2.4.x/STATUS
Modified: httpd/httpd/branches/2.4.x/STATUS
URL: http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/STATUS?rev=1896976&r1=1896975&r2=1896976&view=diff
==============================================================================
--- httpd/httpd/branches/2.4.x/STATUS (original)
+++ httpd/httpd/branches/2.4.x/STATUS Thu Jan 13 03:28:31 2022
@@ -150,6 +150,35 @@ PATCHES ACCEPTED TO BACKPORT FROM TRUNK:
PATCHES PROPOSED TO BACKPORT FROM TRUNK:
[ New proposals should be added at the end of the list ]
+ *) Support PCRE2 (10.x) in place of PCRE (8.x).
+ Submitted by: wrowe, Petr Pisar [ppisar redhat.com], rjung
+ trunk patches:
+ http://svn.apache.org/r1773454
+ http://svn.apache.org/r1773741
+ http://svn.apache.org/r1773742
+ http://svn.apache.org/r1773839
+ http://svn.apache.org/r1773870
+ http://svn.apache.org/r1773882
+ http://svn.apache.org/r1814662
+ http://svn.apache.org/r1881478
+ summarized/conflicts resolved:
+ https://gist.github.com/wrowe/73f655d13bbe0f12030aa4557e804d8a
+ +1: wrowe
+ wrowe notes that the current code drops optimizations, owing to the fact
+ that the ovector is a required allocation and is no longer allocated on
+ the stack, by design. The correct fix is an apr userdata allocation on
+ the appropriate pool, which would be thread-safe, but the actual API of
+ ap_regexec[_len]() offers us no pool and isn't suitable for httpd-2.4.x.
+ At this time, PCRE 8.45 is EOL and will not receive security updates,
+ and taking ovector and other arrays off the stack was in direct reaction
+ to the patterns of abuse of previous pcre exploits. So this patch doesn't
+ wait for httpd-2.4 to be retired, it will need to be adopted without the
+ existing optimiation.
+ jorton: Adding ap_pregexec/_len which pass a pool would also work
+ for internal users of this api; not sure if performance
+ impact is significant from using malloc here.
+
+
PATCHES/ISSUES THAT ARE BEING WORKED
[ New entries should be added at the START of the list ]
@@ -256,28 +285,6 @@ PATCHES/ISSUES THAT ARE BEING WORKED
make it nonblocking (by default)?
jim: Non-blocking seems the best way to handle...
- * Support PCRE2 (10.x) in place of PCRE (8.x).
- Submitted by: wrowe, Petr Pisar [ppisar redhat.com]
- trunk patches:
- http://svn.apache.org/r1773454
- http://svn.apache.org/r1773741
- http://svn.apache.org/r1773742
- http://svn.apache.org/r1773839
- http://svn.apache.org/r1773870
- http://svn.apache.org/r1773882
- wrowe notes that the current code is too inefficient, owing to the fact
- that the ovector is a required allocation and is no longer allocated on
- the stack, by design. The correct fix is an apr userdata allocation on
- the appropriate pool, which would be thread-safe, but the actual API of
- ap_regexec[_len]() offers us no pool. We cannot associate that pool with
- the ap_regex_t, because a single regex may be used by many threads in
- parallel and is not thread-safe beyond initialization.
- So the only fix allowing us to use PCRE 10 in httpd 2.4 would be to write
- this as a thread safe storage buffer for the majority of cases (<10 $args)
- and we don't have a portable tls mechanism to do so.
- jorton: Adding ap_pregexec/_len which pass a pool would also work
- for internal users of this api; not sure if performance
- impact is significant from using malloc here.
PATCHES/ISSUES THAT ARE STALLED
Re: svn commit: r1896976 - /httpd/httpd/branches/2.4.x/STATUS
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Wed, Jan 19, 2022 at 2:02 AM Ruediger Pluem <rp...@apache.org> wrote:
>
> On 1/18/22 11:03 PM, William A Rowe Jr wrote:
> > On Tue, Jan 18, 2022 at 2:20 PM Ruediger Pluem <rp...@apache.org> wrote:
> >>
> >> On 1/18/22 8:54 PM, Yann Ylavic wrote:
> >>> On Tue, Jan 18, 2022 at 7:48 PM William A Rowe Jr <wr...@rowe-clan.net> wrote:
> >>>>
> >>>> Hi Joe, Yann and company,
> >>>>
> >>>> please consider this, we will not build against PCRE2 without pcre2-config
> >>>> installed from a pcre2-dev package. We find PCRE1 with pcre-config and link
> >>>> to it, no hassle.
> >>>
> >>> My concern is about the defaut, what happens if no --with-pcre or
> >>> --with-pcre=yes is specified?
> >>>
> >>>>
> >>>> If someone went to the trouble of installing pcre2, wouldn't we want
> >>>> to pick that
> >>>> up, even across a patch release?
> >>>
> >>> I think that a lot of systems have both installed (including the -dev
> >>> packages for building) since pcre2 is the only one supported by some
> >>> projects now and at least httpd requires pcre1 (until the next
> >>> release) so it's likely there too already.
> >>> I haven't looked at the configure part of the patch yet, it's possible
> >>> that your proposed backport already picks up pcre1 when both are
> >>> available (which looks the most reasonable to me for 2.4.x), I'm just
> >>> lazily asking..
> >>
> >> I think the default is pcre2:
> >>
> >> https://gist.github.com/wrowe/73f655d13bbe0f12030aa4557e804d8a#file-httpd-2-4-x-pcre2-10-x-patch-L97-L99
> >>
> >> Regards
> >>
> >> Rüdiger
> >
> > You are right - pcre2-10.x is the new default of the proposed patch
> > and the behavior of trunk.
> >
> > I do understand that RHEL, Ubuntu, all the other packagers are
> > applying patches on patches even of this abandoned branch.
> >
> > However the author advises us, it's a bad idea to play with pcre
> > arrays on stack. The author advises us that there are known
> > vulnerabilities (known to him) that cannot be fixed on the 8.x branch,
> > owing to the underlying design. Whether we care, since the flaws are
> > mostly in the expressions under the control of the admin, and not the
> > untrusted input against the expressions, I don't know.
>
> Ok. This is a good point. This makes me think of the following:
>
> 1. We should switch the default to pcre2 latest when we have found a solution with pcre2 that
> avoids the constant memory allocations on heap on each regex execution (see discussion elsewhere on the thread).
> 2. We should clearly state in the CHANGES entry that we switch the default to pcre2 once we do as currently it states:
>
> *) Support pcre2 (10.x) library in place of the now end-of-life pcre (8.x)
> for regular expression evaluation. This depends on locating pcre2-config.
> [William Rowe, Petr Pisar <ppisar redhat.com>, Rainer Jung]
>
> Which suggests to the user that it *can* use pcre2 if pcre2-config is present, but not it *will* use it if pcre2-config
> is present.
The key question, do we do this on stack? The library author wisely advises
against our doing this again to avoid stack smashing vectors, and rightly so.
This is the point in time to avoid high-risk coding practices.
So if someone (not me, not this month, too buried) wants to do this... even to
take the suggestion of _ex() functions which have a threadkey tied to the passed
single-threaded pool, I'd be in favor, and it's worth delaying a pcre2-10.x and
openssl 3.0.0 compatible release for just a week or few to work that in.
Re: svn commit: r1896976 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Ruediger Pluem <rp...@apache.org>.
On 1/18/22 11:03 PM, William A Rowe Jr wrote:
> On Tue, Jan 18, 2022 at 2:20 PM Ruediger Pluem <rp...@apache.org> wrote:
>>
>> On 1/18/22 8:54 PM, Yann Ylavic wrote:
>>> On Tue, Jan 18, 2022 at 7:48 PM William A Rowe Jr <wr...@rowe-clan.net> wrote:
>>>>
>>>> Hi Joe, Yann and company,
>>>>
>>>> please consider this, we will not build against PCRE2 without pcre2-config
>>>> installed from a pcre2-dev package. We find PCRE1 with pcre-config and link
>>>> to it, no hassle.
>>>
>>> My concern is about the defaut, what happens if no --with-pcre or
>>> --with-pcre=yes is specified?
>>>
>>>>
>>>> If someone went to the trouble of installing pcre2, wouldn't we want
>>>> to pick that
>>>> up, even across a patch release?
>>>
>>> I think that a lot of systems have both installed (including the -dev
>>> packages for building) since pcre2 is the only one supported by some
>>> projects now and at least httpd requires pcre1 (until the next
>>> release) so it's likely there too already.
>>> I haven't looked at the configure part of the patch yet, it's possible
>>> that your proposed backport already picks up pcre1 when both are
>>> available (which looks the most reasonable to me for 2.4.x), I'm just
>>> lazily asking..
>>
>> I think the default is pcre2:
>>
>> https://gist.github.com/wrowe/73f655d13bbe0f12030aa4557e804d8a#file-httpd-2-4-x-pcre2-10-x-patch-L97-L99
>>
>> Regards
>>
>> Rüdiger
>
> You are right - pcre2-10.x is the new default of the proposed patch
> and the behavior of trunk.
>
> I do understand that RHEL, Ubuntu, all the other packagers are
> applying patches on patches even of this abandoned branch.
>
> However the author advises us, it's a bad idea to play with pcre
> arrays on stack. The author advises us that there are known
> vulnerabilities (known to him) that cannot be fixed on the 8.x branch,
> owing to the underlying design. Whether we care, since the flaws are
> mostly in the expressions under the control of the admin, and not the
> untrusted input against the expressions, I don't know.
Ok. This is a good point. This makes me think of the following:
1. We should switch the default to pcre2 latest when we have found a solution with pcre2 that
avoids the constant memory allocations on heap on each regex execution (see discussion elsewhere on the thread).
2. We should clearly state in the CHANGES entry that we switch the default to pcre2 once we do as currently it states:
*) Support pcre2 (10.x) library in place of the now end-of-life pcre (8.x)
for regular expression evaluation. This depends on locating pcre2-config.
[William Rowe, Petr Pisar <ppisar redhat.com>, Rainer Jung]
Which suggests to the user that it *can* use pcre2 if pcre2-config is present, but not it *will* use it if pcre2-config
is present.
Regards
Rüdiger
Re: svn commit: r1896976 - /httpd/httpd/branches/2.4.x/STATUS
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Tue, Jan 18, 2022 at 2:20 PM Ruediger Pluem <rp...@apache.org> wrote:
>
> On 1/18/22 8:54 PM, Yann Ylavic wrote:
> > On Tue, Jan 18, 2022 at 7:48 PM William A Rowe Jr <wr...@rowe-clan.net> wrote:
> >>
> >> Hi Joe, Yann and company,
> >>
> >> please consider this, we will not build against PCRE2 without pcre2-config
> >> installed from a pcre2-dev package. We find PCRE1 with pcre-config and link
> >> to it, no hassle.
> >
> > My concern is about the defaut, what happens if no --with-pcre or
> > --with-pcre=yes is specified?
> >
> >>
> >> If someone went to the trouble of installing pcre2, wouldn't we want
> >> to pick that
> >> up, even across a patch release?
> >
> > I think that a lot of systems have both installed (including the -dev
> > packages for building) since pcre2 is the only one supported by some
> > projects now and at least httpd requires pcre1 (until the next
> > release) so it's likely there too already.
> > I haven't looked at the configure part of the patch yet, it's possible
> > that your proposed backport already picks up pcre1 when both are
> > available (which looks the most reasonable to me for 2.4.x), I'm just
> > lazily asking..
>
> I think the default is pcre2:
>
> https://gist.github.com/wrowe/73f655d13bbe0f12030aa4557e804d8a#file-httpd-2-4-x-pcre2-10-x-patch-L97-L99
>
> Regards
>
> Rüdiger
You are right - pcre2-10.x is the new default of the proposed patch
and the behavior of trunk.
I do understand that RHEL, Ubuntu, all the other packagers are
applying patches on patches even of this abandoned branch.
However the author advises us, it's a bad idea to play with pcre
arrays on stack. The author advises us that there are known
vulnerabilities (known to him) that cannot be fixed on the 8.x branch,
owing to the underlying design. Whether we care, since the flaws are
mostly in the expressions under the control of the admin, and not the
untrusted input against the expressions, I don't know.
Re: svn commit: r1896976 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Ruediger Pluem <rp...@apache.org>.
On 1/18/22 8:54 PM, Yann Ylavic wrote:
> On Tue, Jan 18, 2022 at 7:48 PM William A Rowe Jr <wr...@rowe-clan.net> wrote:
>>
>> Hi Joe, Yann and company,
>>
>> please consider this, we will not build against PCRE2 without pcre2-config
>> installed from a pcre2-dev package. We find PCRE1 with pcre-config and link
>> to it, no hassle.
>
> My concern is about the defaut, what happens if no --with-pcre or
> --with-pcre=yes is specified?
>
>>
>> If someone went to the trouble of installing pcre2, wouldn't we want
>> to pick that
>> up, even across a patch release?
>
> I think that a lot of systems have both installed (including the -dev
> packages for building) since pcre2 is the only one supported by some
> projects now and at least httpd requires pcre1 (until the next
> release) so it's likely there too already.
> I haven't looked at the configure part of the patch yet, it's possible
> that your proposed backport already picks up pcre1 when both are
> available (which looks the most reasonable to me for 2.4.x), I'm just
> lazily asking..
I think the default is pcre2:
https://gist.github.com/wrowe/73f655d13bbe0f12030aa4557e804d8a#file-httpd-2-4-x-pcre2-10-x-patch-L97-L99
Regards
Rüdiger
Re: svn commit: r1896976 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Yann Ylavic <yl...@gmail.com>.
On Tue, Jan 18, 2022 at 7:48 PM William A Rowe Jr <wr...@rowe-clan.net> wrote:
>
> Hi Joe, Yann and company,
>
> please consider this, we will not build against PCRE2 without pcre2-config
> installed from a pcre2-dev package. We find PCRE1 with pcre-config and link
> to it, no hassle.
My concern is about the defaut, what happens if no --with-pcre or
--with-pcre=yes is specified?
>
> If someone went to the trouble of installing pcre2, wouldn't we want
> to pick that
> up, even across a patch release?
I think that a lot of systems have both installed (including the -dev
packages for building) since pcre2 is the only one supported by some
projects now and at least httpd requires pcre1 (until the next
release) so it's likely there too already.
I haven't looked at the configure part of the patch yet, it's possible
that your proposed backport already picks up pcre1 when both are
available (which looks the most reasonable to me for 2.4.x), I'm just
lazily asking..
Regards;
Yann.
Re: svn commit: r1896976 - /httpd/httpd/branches/2.4.x/STATUS
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Mon, Jan 17, 2022 at 6:38 PM Yann Ylavic <yl...@gmail.com> wrote:
>
> On Mon, Jan 17, 2022 at 4:05 PM Joe Orton <jo...@redhat.com> wrote:
> >
> > For 2.4.x I would argue it's better to keep a preference for 8.x over
> > 10.x so that users aren't switched from one to the other across an
> > upgrade - with some new performance trade-off we know about - without
> > changing the environment/configure line?
>
> +1, we should try to find PCRE1 first in 2.4.x and fall back to PCRE2
> only if PCRE1 is not there.
Hi Joe, Yann and company,
please consider this, we will not build against PCRE2 without pcre2-config
installed from a pcre2-dev package. We find PCRE1 with pcre-config and link
to it, no hassle.
If someone went to the trouble of installing pcre2, wouldn't we want
to pick that
up, even across a patch release?
Re: svn commit: r1896976 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Yann Ylavic <yl...@gmail.com>.
On Mon, Jan 17, 2022 at 4:05 PM Joe Orton <jo...@redhat.com> wrote:
>
> For 2.4.x I would argue it's better to keep a preference for 8.x over
> 10.x so that users aren't switched from one to the other across an
> upgrade - with some new performance trade-off we know about - without
> changing the environment/configure line?
+1, we should try to find PCRE1 first in 2.4.x and fall back to PCRE2
only if PCRE1 is not there.
Regards;
Yann.
Re: svn commit: r1896976 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Ruediger Pluem <rp...@apache.org>.
On 1/17/22 8:35 PM, William A Rowe Jr wrote:
>
>
> On Mon, Jan 17, 2022, 09:37 Ruediger Pluem <rpluem@apache.org <ma...@apache.org>> wrote:
>
>
>
> On 1/17/22 4:05 PM, Joe Orton wrote:
> > On Sun, Jan 16, 2022 at 03:35:15PM -0600, William A Rowe Jr wrote:
> >> 4 day ago, you all saw this. 6 years ago, you all started using this on trunk.
> >>
> >> Don't know what I can to do help this along and honor the library
> >> author's wishes for all of us to walk away from the dead fork, and use
> >> the maintained fork. It's whatever it is, I'm out of here and removing
> >> the backport application branch, whoever 3rd upvotes this be prepared
> >> to apply this for us all, thanks.
> >
> > I'm fine with PCRE 10.x as a trunk/2.5 feature. PCRE upstream have
> > maintained 8.x better than e.g. zlib upstream have done in recent years
> > (last zlib release in 2017). So I don't find the fact it's considered
> > EOL upstream presents any particular urgency, it's still supported
> > downstream on platforms people deploy to.
> >
> > For 2.4.x I would argue it's better to keep a preference for 8.x over
> > 10.x so that users aren't switched from one to the other across an
> > upgrade - with some new performance trade-off we know about - without
> > changing the environment/configure line?
>
> Sounds sensible for Linux to keep the default to 8.x if found where people
> can expect their distribution to maintain stuff provided that the distribution is still maintained.
> I am not so sure for other platforms especially Windows where I guess that people get stuff
> more often directly from upstream.
>
>
> Sensible? Did you read the memo at pcre.org <http://pcre.org>? There will be no more evaluations of security risks on the
> abandoned fork and we were told this back in May 2021.
>
> Do you still have the same posture? Some of us spent the last 5 years arguing for httpd.next, and I resigned over it. Your call,
Unless I missed something on the link you provided this just looks like an EOL of a version by the upstream project. Many Linux
distributions, especially the LTS ones contain such versions of various products and promise to maintain them for the distribution
until the support for the distribution version stops. e.g. RedHat 7 still ships openssl 1.0.2 which is not supported upstream any
longer, but still receives updates in RedHat 7 until its regular support ends in 2024.
So my posture is still the same. But I want to thank you for making this backport proposal for which I already voted as it brought
up to our attention that 2.4.x users which cannot / do not want to use the old PCRE version for various reasons (e.g. no one
providing support for it, for the particular platform they use, not wanting to use such LTS distribution provided versions, etc.)
had no choice to use the upstream supported version. Once the backport comes in they have that possibility which is great.
Regards
Rüdiger
Re: svn commit: r1896976 - /httpd/httpd/branches/2.4.x/STATUS
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Mon, Jan 17, 2022, 09:37 Ruediger Pluem <rp...@apache.org> wrote:
>
>
> On 1/17/22 4:05 PM, Joe Orton wrote:
> > On Sun, Jan 16, 2022 at 03:35:15PM -0600, William A Rowe Jr wrote:
> >> 4 day ago, you all saw this. 6 years ago, you all started using this on
> trunk.
> >>
> >> Don't know what I can to do help this along and honor the library
> >> author's wishes for all of us to walk away from the dead fork, and use
> >> the maintained fork. It's whatever it is, I'm out of here and removing
> >> the backport application branch, whoever 3rd upvotes this be prepared
> >> to apply this for us all, thanks.
> >
> > I'm fine with PCRE 10.x as a trunk/2.5 feature. PCRE upstream have
> > maintained 8.x better than e.g. zlib upstream have done in recent years
> > (last zlib release in 2017). So I don't find the fact it's considered
> > EOL upstream presents any particular urgency, it's still supported
> > downstream on platforms people deploy to.
> >
> > For 2.4.x I would argue it's better to keep a preference for 8.x over
> > 10.x so that users aren't switched from one to the other across an
> > upgrade - with some new performance trade-off we know about - without
> > changing the environment/configure line?
>
> Sounds sensible for Linux to keep the default to 8.x if found where people
> can expect their distribution to maintain stuff provided that the
> distribution is still maintained.
> I am not so sure for other platforms especially Windows where I guess that
> people get stuff
> more often directly from upstream.
>
Sensible? Did you read the memo at pcre.org? There will be no more
evaluations of security risks on the abandoned fork and we were told this
back in May 2021.
Do you still have the same posture? Some of us spent the last 5 years
arguing for httpd.next, and I resigned over it. Your call, you are PMC and
I choose not to be.
Re: svn commit: r1896976 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Ruediger Pluem <rp...@apache.org>.
On 1/17/22 4:05 PM, Joe Orton wrote:
> On Sun, Jan 16, 2022 at 03:35:15PM -0600, William A Rowe Jr wrote:
>> 4 day ago, you all saw this. 6 years ago, you all started using this on trunk.
>>
>> Don't know what I can to do help this along and honor the library
>> author's wishes for all of us to walk away from the dead fork, and use
>> the maintained fork. It's whatever it is, I'm out of here and removing
>> the backport application branch, whoever 3rd upvotes this be prepared
>> to apply this for us all, thanks.
>
> I'm fine with PCRE 10.x as a trunk/2.5 feature. PCRE upstream have
> maintained 8.x better than e.g. zlib upstream have done in recent years
> (last zlib release in 2017). So I don't find the fact it's considered
> EOL upstream presents any particular urgency, it's still supported
> downstream on platforms people deploy to.
>
> For 2.4.x I would argue it's better to keep a preference for 8.x over
> 10.x so that users aren't switched from one to the other across an
> upgrade - with some new performance trade-off we know about - without
> changing the environment/configure line?
Sounds sensible for Linux to keep the default to 8.x if found where people
can expect their distribution to maintain stuff provided that the distribution is still maintained.
I am not so sure for other platforms especially Windows where I guess that people get stuff
more often directly from upstream.
Regards
Rüdiger
Re: svn commit: r1896976 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Joe Orton <jo...@redhat.com>.
On Sun, Jan 16, 2022 at 03:35:15PM -0600, William A Rowe Jr wrote:
> 4 day ago, you all saw this. 6 years ago, you all started using this on trunk.
>
> Don't know what I can to do help this along and honor the library
> author's wishes for all of us to walk away from the dead fork, and use
> the maintained fork. It's whatever it is, I'm out of here and removing
> the backport application branch, whoever 3rd upvotes this be prepared
> to apply this for us all, thanks.
I'm fine with PCRE 10.x as a trunk/2.5 feature. PCRE upstream have
maintained 8.x better than e.g. zlib upstream have done in recent years
(last zlib release in 2017). So I don't find the fact it's considered
EOL upstream presents any particular urgency, it's still supported
downstream on platforms people deploy to.
For 2.4.x I would argue it's better to keep a preference for 8.x over
10.x so that users aren't switched from one to the other across an
upgrade - with some new performance trade-off we know about - without
changing the environment/configure line?
Regards, Joe
Re: svn commit: r1896976 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Christophe JAILLET <ch...@wanadoo.fr>.
Le 17/01/2022 à 14:16, Ruediger Pluem a écrit :
>
> On 1/17/22 1:38 PM, Eric Covener wrote:
>> On Mon, Jan 17, 2022 at 7:28 AM Ruediger Pluem <rp...@apache.org> wrote:
>>>
>>>
>>> On 1/16/22 10:35 PM, William A Rowe Jr wrote:
>>>> 4 day ago, you all saw this. 6 years ago, you all started using this on trunk.
>>>>
>>>> Don't know what I can to do help this along and honor the library
>>>> author's wishes for
>>>> all of us to walk away from the dead fork, and use the maintained
>>>> fork. It's whatever
>>>> it is, I'm out of here and removing the backport application branch,
>>>> whoever 3rd upvotes
>>>> this be prepared to apply this for us all, thanks.
>>> With regards to the de-optimization / stack usage probably stupid question:
>>> Can't we use the TLS features that several compilers offer?
>>> e.g. see at the end of the page at https://stackoverflow.com/questions/18298280/how-to-declare-a-variable-as-thread-local-portably
>>>
>>> If we have no thread_local we could fall back to the current unoptimized implementation?
>> I more identified with Joe's comment, if we had an _ex version we
>> could pass pools for our own usage.
>> If some other heavy regex hitter third-party module finds an issue
>> (like mod_security) it might degrade a little bit but they can always
>> take advantage of it too.
>>
>> Also, what about alloca()?
>>
>> Trivia but I should add that in an integration between two third-party
>> mods I recently ripped out apr_threadkey stuff due to a crash after
>> some OS updates that nobody can explain. Fortunately the problematic
>> API had been extended with userdata and it was no longer absolutely
>> necessary. It was our only use of these API's so I was glad to
>> simplify and just remove them and move on.
>> Net, I would be hesitant to introduce something too new.
> >From a quick glance apr_threadkey seems to use pthread_*specific for which I found postings that state that it is slow compared to
> the compiler implemented thread locals. OTOH __thread alike stuff does not seem to offer any cleanup at thread exit which would
> not allow us to free any resource that was created for that thread :-(. Hence I guess providing a pool to the call remains the
> only solution here.
Some time ago, I played with apr_threadkey for another use case related
to mod_unique_id (see r1887244+r1887245)
It has been reverted later because getting the thread ID was not
reliable on Windows.
Discussion in BZ 65159.
I thought it could be a solution for PREC10 (see r1889287) but it wasn't.
I also looked at thread locals but AFAIK, it is not used in APR and it
was raising a potential compatibility issue with build chain.
Just my 2c.
CJ
> Regards
>
> Rüdiger
>
Re: svn commit: r1896976 - /httpd/httpd/branches/2.4.x/STATUS
Posted by gs...@gluelogic.com.
On Mon, Jan 17, 2022 at 02:16:32PM +0100, Ruediger Pluem wrote:
>
>
> On 1/17/22 1:38 PM, Eric Covener wrote:
> > On Mon, Jan 17, 2022 at 7:28 AM Ruediger Pluem <rp...@apache.org> wrote:
> >>
> >>
> >>
> >> On 1/16/22 10:35 PM, William A Rowe Jr wrote:
> >>> 4 day ago, you all saw this. 6 years ago, you all started using this on trunk.
> >>>
> >>> Don't know what I can to do help this along and honor the library
> >>> author's wishes for
> >>> all of us to walk away from the dead fork, and use the maintained
> >>> fork. It's whatever
> >>> it is, I'm out of here and removing the backport application branch,
> >>> whoever 3rd upvotes
> >>> this be prepared to apply this for us all, thanks.
> >>
> >> With regards to the de-optimization / stack usage probably stupid question:
> >> Can't we use the TLS features that several compilers offer?
> >> e.g. see at the end of the page at https://stackoverflow.com/questions/18298280/how-to-declare-a-variable-as-thread-local-portably
> >>
> >> If we have no thread_local we could fall back to the current unoptimized implementation?
> >
> > I more identified with Joe's comment, if we had an _ex version we
> > could pass pools for our own usage.
> > If some other heavy regex hitter third-party module finds an issue
> > (like mod_security) it might degrade a little bit but they can always
> > take advantage of it too.
> >
> > Also, what about alloca()?
> >
> > Trivia but I should add that in an integration between two third-party
> > mods I recently ripped out apr_threadkey stuff due to a crash after
> > some OS updates that nobody can explain. Fortunately the problematic
> > API had been extended with userdata and it was no longer absolutely
> > necessary. It was our only use of these API's so I was glad to
> > simplify and just remove them and move on.
> > Net, I would be hesitant to introduce something too new.
>
> From a quick glance apr_threadkey seems to use pthread_*specific for which I found postings that state that it is slow compared to
> the compiler implemented thread locals. OTOH __thread alike stuff does not seem to offer any cleanup at thread exit which would
> not allow us to free any resource that was created for that thread :-(. Hence I guess providing a pool to the call remains the
> only solution here.
Maybe use thread-local storage to set a pointer to an apr_pool
associated with the thread, and set that only for short-lived
(or otherwise appropriately scoped) threads?
Cheers, Glenn
Re: svn commit: r1896976 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Ruediger Pluem <rp...@apache.org>.
On 1/17/22 1:38 PM, Eric Covener wrote:
> On Mon, Jan 17, 2022 at 7:28 AM Ruediger Pluem <rp...@apache.org> wrote:
>>
>>
>>
>> On 1/16/22 10:35 PM, William A Rowe Jr wrote:
>>> 4 day ago, you all saw this. 6 years ago, you all started using this on trunk.
>>>
>>> Don't know what I can to do help this along and honor the library
>>> author's wishes for
>>> all of us to walk away from the dead fork, and use the maintained
>>> fork. It's whatever
>>> it is, I'm out of here and removing the backport application branch,
>>> whoever 3rd upvotes
>>> this be prepared to apply this for us all, thanks.
>>
>> With regards to the de-optimization / stack usage probably stupid question:
>> Can't we use the TLS features that several compilers offer?
>> e.g. see at the end of the page at https://stackoverflow.com/questions/18298280/how-to-declare-a-variable-as-thread-local-portably
>>
>> If we have no thread_local we could fall back to the current unoptimized implementation?
>
> I more identified with Joe's comment, if we had an _ex version we
> could pass pools for our own usage.
> If some other heavy regex hitter third-party module finds an issue
> (like mod_security) it might degrade a little bit but they can always
> take advantage of it too.
>
> Also, what about alloca()?
>
> Trivia but I should add that in an integration between two third-party
> mods I recently ripped out apr_threadkey stuff due to a crash after
> some OS updates that nobody can explain. Fortunately the problematic
> API had been extended with userdata and it was no longer absolutely
> necessary. It was our only use of these API's so I was glad to
> simplify and just remove them and move on.
> Net, I would be hesitant to introduce something too new.
From a quick glance apr_threadkey seems to use pthread_*specific for which I found postings that state that it is slow compared to
the compiler implemented thread locals. OTOH __thread alike stuff does not seem to offer any cleanup at thread exit which would
not allow us to free any resource that was created for that thread :-(. Hence I guess providing a pool to the call remains the
only solution here.
Regards
Rüdiger
Re: svn commit: r1896976 - /httpd/httpd/branches/2.4.x/STATUS
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
Ixney on the alloca. That's the entire point of not moving array
allocations to the stack, because the author won't take responsibility for
potential underflows and overflows. Stack is the wrong place for these bits
On Mon, Jan 17, 2022, 06:38 Eric Covener <co...@gmail.com> wrote:
> On Mon, Jan 17, 2022 at 7:28 AM Ruediger Pluem <rp...@apache.org> wrote:
> >
> >
> >
> > On 1/16/22 10:35 PM, William A Rowe Jr wrote:
> > > 4 day ago, you all saw this. 6 years ago, you all started using this
> on trunk.
> > >
> > > Don't know what I can to do help this along and honor the library
> > > author's wishes for
> > > all of us to walk away from the dead fork, and use the maintained
> > > fork. It's whatever
> > > it is, I'm out of here and removing the backport application branch,
> > > whoever 3rd upvotes
> > > this be prepared to apply this for us all, thanks.
> >
> > With regards to the de-optimization / stack usage probably stupid
> question:
> > Can't we use the TLS features that several compilers offer?
> > e.g. see at the end of the page at
> https://stackoverflow.com/questions/18298280/how-to-declare-a-variable-as-thread-local-portably
> >
> > If we have no thread_local we could fall back to the current unoptimized
> implementation?
>
> I more identified with Joe's comment, if we had an _ex version we
> could pass pools for our own usage.
> If some other heavy regex hitter third-party module finds an issue
> (like mod_security) it might degrade a little bit but they can always
> take advantage of it too.
>
> Also, what about alloca()?
>
> Trivia but I should add that in an integration between two third-party
> mods I recently ripped out apr_threadkey stuff due to a crash after
> some OS updates that nobody can explain. Fortunately the problematic
> API had been extended with userdata and it was no longer absolutely
> necessary. It was our only use of these API's so I was glad to
> simplify and just remove them and move on.
> Net, I would be hesitant to introduce something too new.
>
Re: svn commit: r1896976 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Eric Covener <co...@gmail.com>.
On Mon, Jan 17, 2022 at 7:28 AM Ruediger Pluem <rp...@apache.org> wrote:
>
>
>
> On 1/16/22 10:35 PM, William A Rowe Jr wrote:
> > 4 day ago, you all saw this. 6 years ago, you all started using this on trunk.
> >
> > Don't know what I can to do help this along and honor the library
> > author's wishes for
> > all of us to walk away from the dead fork, and use the maintained
> > fork. It's whatever
> > it is, I'm out of here and removing the backport application branch,
> > whoever 3rd upvotes
> > this be prepared to apply this for us all, thanks.
>
> With regards to the de-optimization / stack usage probably stupid question:
> Can't we use the TLS features that several compilers offer?
> e.g. see at the end of the page at https://stackoverflow.com/questions/18298280/how-to-declare-a-variable-as-thread-local-portably
>
> If we have no thread_local we could fall back to the current unoptimized implementation?
I more identified with Joe's comment, if we had an _ex version we
could pass pools for our own usage.
If some other heavy regex hitter third-party module finds an issue
(like mod_security) it might degrade a little bit but they can always
take advantage of it too.
Also, what about alloca()?
Trivia but I should add that in an integration between two third-party
mods I recently ripped out apr_threadkey stuff due to a crash after
some OS updates that nobody can explain. Fortunately the problematic
API had been extended with userdata and it was no longer absolutely
necessary. It was our only use of these API's so I was glad to
simplify and just remove them and move on.
Net, I would be hesitant to introduce something too new.
Re: svn commit: r1896976 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Yann Ylavic <yl...@gmail.com>.
On Mon, Jan 17, 2022 at 10:26 PM Christophe JAILLET
<ch...@wanadoo.fr> wrote:
>
> Le 17/01/2022 à 13:27, Ruediger Pluem a écrit :
> >
> > With regards to the de-optimization / stack usage probably stupid question:
> > Can't we use the TLS features that several compilers offer?
> > e.g. see at the end of the page at https://stackoverflow.com/questions/18298280/how-to-declare-a-variable-as-thread-local-portably
> >
> > If we have no thread_local we could fall back to the current unoptimized implementation?
Looks good to me, I think the simpler way would be in APR though, see below.
>
> What about something as stupid as the attached patch?
> (just a POC to give the general idea)
I don't think we can override pcre2_{malloc,free}() for every module,
they may be using pcre2 outside of the httpd api/abi.
>
> Reserve some space on stack (in the spirit of the optimization for
> PCRE1) and implement a malloc/free that is clever enough to use this
> space instead a real malloc if pcre2 does not ask for too much space.
[]
>
> The space required by pcre2 is:
> offsetof(pcre2_match_data, ovector) + 2*oveccount*sizeof(PCRE2_SIZE)
Which may be too big for the stack already?
>
> PCRE2_SIZE ovector[131072]; /* Must be last in the structure */
That one clearly is, but AIUI it's just a variable length structure so
we wouldn't really reserve that size.
>
> So we should be able to compute a MAX_PCRE2_STACK_DATA (see patch) that
> is both large enough to store 10 or so matches, and not too big.
>
> Not sure it is a clean solution, but I guess that it could work.
I'd rather we use the efficient
_Thread_local/__thread/__declspec(thread) in APR (see attached patch,
apr-1.8 material?) to track the current apr_thread_t and return it
with e.g. apr_thread_current(). Then we could simply do things like
this for ap_regex:
#ifndef APR_HAS_THREAD_LOCAL
/* always allocate */
#else
struct match_data {
#ifdef HAVE_PCRE2
pcre2_match_data *data;
#else
int *data;
#endif
apr_size_t size;
};
struct match_data *matchdata = NULL;
apr_thread_t *thd = apr_thread_current();
apr_thread_data_get((void **)&matchdata, "apreg", thd);
if (!matchdata || matchdata->size < new_size) {
if (matchdata) {
#ifdef HAVE_PCRE2
pcre2_match_data_free(matchdata->data);
#else
free(matchdata->data);
#endif
}
else {
matchdata = calloc(1, sizeof(*matchdata);
apr_thread_data_set(matchdata, "ap_regex",
free_matchdata, thd);
}
matchdata->size *= 2;
if (matchdata->size < new_size) {
matchdata->size = new_size;
if (matchdata->size < POSIX_MALLOC_THRESHOLD) {
matchdata->size = POSIX_MALLOC_THRESHOLD;
}
}
#ifdef HAVE_PCRE2
matchdata->data = pcre2_match_data_create(matchdata->size);
#else
matchdata->data = malloc(matchdata->size * sizeof(int) * 3);
#endif
}
#ifdef HAVE_PCRE2
ovector = pcre2_get_ovector_pointer(matchdata->data);
#else
ovector = matchdata->data;
#endif
#endif /* APR_HAS_THREAD_LOCAL */
(We could still keep the stack for the PCRE1 small size case though
that might complicate the code unnecessarily since reusing the TLS
matchdata in this case should be as effective as an apr_hash lookup).
WDYT?
Regards;
Yann.
Re: svn commit: r1896976 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Christophe JAILLET <ch...@wanadoo.fr>.
Le 17/01/2022 à 13:27, Ruediger Pluem a écrit :
>
>
> On 1/16/22 10:35 PM, William A Rowe Jr wrote:
>> 4 day ago, you all saw this. 6 years ago, you all started using this on trunk.
>>
>> Don't know what I can to do help this along and honor the library
>> author's wishes for
>> all of us to walk away from the dead fork, and use the maintained
>> fork. It's whatever
>> it is, I'm out of here and removing the backport application branch,
>> whoever 3rd upvotes
>> this be prepared to apply this for us all, thanks.
>
> With regards to the de-optimization / stack usage probably stupid question:
> Can't we use the TLS features that several compilers offer?
> e.g. see at the end of the page at https://stackoverflow.com/questions/18298280/how-to-declare-a-variable-as-thread-local-portably
>
> If we have no thread_local we could fall back to the current unoptimized implementation?
>
> Regards
>
> Rüdiger
>
>
What about something as stupid as the attached patch?
(just a POC to give the general idea)
Reserve some space on stack (in the spirit of the optimization for
PCRE1) and implement a malloc/free that is clever enough to use this
space instead a real malloc if pcre2 does not ask for too much space.
The only difference with what is done with PCRE1 is that in this case we
use on the stack the exact space needed for for POSIX_MALLOC_THRESHOLD
(10) matches. In the attach patch, we reserve "some space" on the stack
and we expect that it is large enough to avoid a malloc.
The space required by pcre2 is:
offsetof(pcre2_match_data, ovector) + 2*oveccount*sizeof(PCRE2_SIZE)
With:
typedef struct pcre2_real_match_data {
pcre2_memctl memctl;
const pcre2_real_code *code; /* The pattern used for the match */
PCRE2_SPTR subject; /* The subject that was matched */
PCRE2_SPTR mark; /* Pointer to last mark */
PCRE2_SIZE leftchar; /* Offset to leftmost code unit */
PCRE2_SIZE rightchar; /* Offset to rightmost code unit */
PCRE2_SIZE startchar; /* Offset to starting code unit */
uint8_t matchedby; /* Type of match (normal, JIT, DFA) */
uint8_t flags; /* Various flags */
uint16_t oveccount; /* Number of pairs */
int rc; /* The return code from the match */
PCRE2_SIZE ovector[131072]; /* Must be last in the structure */
} pcre2_real_match_data;
(my understanding is that pcre2_real_match_data and pcre2_match_data are
the same)
So we should be able to compute a MAX_PCRE2_STACK_DATA (see patch) that
is both large enough to store 10 or so matches, and not too big.
Not sure it is a clean solution, but I guess that it could work.
BTW, does someone have some numbers for the cost of the malloc we try to
avoid? (read: are we over engineering the solution?)
CJ
Re: svn commit: r1896976 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Ruediger Pluem <rp...@apache.org>.
On 1/16/22 10:35 PM, William A Rowe Jr wrote:
> 4 day ago, you all saw this. 6 years ago, you all started using this on trunk.
>
> Don't know what I can to do help this along and honor the library
> author's wishes for
> all of us to walk away from the dead fork, and use the maintained
> fork. It's whatever
> it is, I'm out of here and removing the backport application branch,
> whoever 3rd upvotes
> this be prepared to apply this for us all, thanks.
With regards to the de-optimization / stack usage probably stupid question:
Can't we use the TLS features that several compilers offer?
e.g. see at the end of the page at https://stackoverflow.com/questions/18298280/how-to-declare-a-variable-as-thread-local-portably
If we have no thread_local we could fall back to the current unoptimized implementation?
Regards
Rüdiger
Re: svn commit: r1896976 - /httpd/httpd/branches/2.4.x/STATUS
Posted by William A Rowe Jr <wr...@apache.org>.
4 day ago, you all saw this. 6 years ago, you all started using this on trunk.
Don't know what I can to do help this along and honor the library
author's wishes for
all of us to walk away from the dead fork, and use the maintained
fork. It's whatever
it is, I'm out of here and removing the backport application branch,
whoever 3rd upvotes
this be prepared to apply this for us all, thanks.
On Wed, Jan 12, 2022 at 9:28 PM <wr...@apache.org> wrote:
>
> Author: wrowe
> Date: Thu Jan 13 03:28:31 2022
> New Revision: 1896976
>
> URL: http://svn.apache.org/viewvc?rev=1896976&view=rev
> Log:
> Propose pcre2 support for backport
>
> Modified:
> httpd/httpd/branches/2.4.x/STATUS
>
> Modified: httpd/httpd/branches/2.4.x/STATUS
> URL: http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/STATUS?rev=1896976&r1=1896975&r2=1896976&view=diff
> ==============================================================================
> --- httpd/httpd/branches/2.4.x/STATUS (original)
> +++ httpd/httpd/branches/2.4.x/STATUS Thu Jan 13 03:28:31 2022
> @@ -150,6 +150,35 @@ PATCHES ACCEPTED TO BACKPORT FROM TRUNK:
> PATCHES PROPOSED TO BACKPORT FROM TRUNK:
> [ New proposals should be added at the end of the list ]
>
> + *) Support PCRE2 (10.x) in place of PCRE (8.x).
> + Submitted by: wrowe, Petr Pisar [ppisar redhat.com], rjung
> + trunk patches:
> + http://svn.apache.org/r1773454
> + http://svn.apache.org/r1773741
> + http://svn.apache.org/r1773742
> + http://svn.apache.org/r1773839
> + http://svn.apache.org/r1773870
> + http://svn.apache.org/r1773882
> + http://svn.apache.org/r1814662
> + http://svn.apache.org/r1881478
> + summarized/conflicts resolved:
> + https://gist.github.com/wrowe/73f655d13bbe0f12030aa4557e804d8a
> + +1: wrowe
> + wrowe notes that the current code drops optimizations, owing to the fact
> + that the ovector is a required allocation and is no longer allocated on
> + the stack, by design. The correct fix is an apr userdata allocation on
> + the appropriate pool, which would be thread-safe, but the actual API of
> + ap_regexec[_len]() offers us no pool and isn't suitable for httpd-2.4.x.
> + At this time, PCRE 8.45 is EOL and will not receive security updates,
> + and taking ovector and other arrays off the stack was in direct reaction
> + to the patterns of abuse of previous pcre exploits. So this patch doesn't
> + wait for httpd-2.4 to be retired, it will need to be adopted without the
> + existing optimiation.
> + jorton: Adding ap_pregexec/_len which pass a pool would also work
> + for internal users of this api; not sure if performance
> + impact is significant from using malloc here.
> +
> +
> PATCHES/ISSUES THAT ARE BEING WORKED
> [ New entries should be added at the START of the list ]
>
> @@ -256,28 +285,6 @@ PATCHES/ISSUES THAT ARE BEING WORKED
> make it nonblocking (by default)?
> jim: Non-blocking seems the best way to handle...
>
> - * Support PCRE2 (10.x) in place of PCRE (8.x).
> - Submitted by: wrowe, Petr Pisar [ppisar redhat.com]
> - trunk patches:
> - http://svn.apache.org/r1773454
> - http://svn.apache.org/r1773741
> - http://svn.apache.org/r1773742
> - http://svn.apache.org/r1773839
> - http://svn.apache.org/r1773870
> - http://svn.apache.org/r1773882
> - wrowe notes that the current code is too inefficient, owing to the fact
> - that the ovector is a required allocation and is no longer allocated on
> - the stack, by design. The correct fix is an apr userdata allocation on
> - the appropriate pool, which would be thread-safe, but the actual API of
> - ap_regexec[_len]() offers us no pool. We cannot associate that pool with
> - the ap_regex_t, because a single regex may be used by many threads in
> - parallel and is not thread-safe beyond initialization.
> - So the only fix allowing us to use PCRE 10 in httpd 2.4 would be to write
> - this as a thread safe storage buffer for the majority of cases (<10 $args)
> - and we don't have a portable tls mechanism to do so.
> - jorton: Adding ap_pregexec/_len which pass a pool would also work
> - for internal users of this api; not sure if performance
> - impact is significant from using malloc here.
>
> PATCHES/ISSUES THAT ARE STALLED
>
>
>
Re: svn commit: r1896976 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Ruediger Pluem <rp...@apache.org>.
On 2/1/22 1:23 AM, Yann Ylavic wrote:
> On Mon, Jan 31, 2022 at 10:10 AM Ruediger Pluem <rp...@apache.org> wrote:
>>
>> BTW: Backporting r1897460 and r1897472 for child.c of the Windows MPM needs some close look as a number of changes
>> to child.c in trunk have not been backported yet:
>
> The gap is too big (for me) between the winnt MPMs on trunk and 2.4.x
> for backporting r1897460 and r1897472 easily.
> What I did in https://github.com/apache/httpd/pull/289 is to use
> ap_thread_current_create() directly in the worker_main() thread, it's
> simple, but it does not even compile because
> ap_thread_current_create() is not AP_DECLARE/exported anymore, it's
> been replaced by ap_thread_main_create() in the latest commits of the
> series (since it's how it's used everywhere else).
> So we could AP_DECLARE ap_thread_current_create() specifically for
> 2.4.x, or disable thread locals for pcre2 on Windows, or someone will
> have to propose a more complete backport with the below list first:
>
>> 1801146
>> 1801147
>> 1801635
>> 1801636
>> 1801637
>> 1801639
>> 1801640
>> 1801655
>> 1801656
>> 1801657
>> 1801710
>> 1801747
>> 1801748
>> 1801749
>> 1801753
I have done a quick check and they merge just fine (minus a log-message-tags conflict) from
a text perspective. But I have not the expertise to review these change nor can I compile
these. But I am happy to create a PR in Github if someone can review and compile this.
Regards
Rüdiger
Re: svn commit: r1896976 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Yann Ylavic <yl...@gmail.com>.
On Mon, Jan 31, 2022 at 10:10 AM Ruediger Pluem <rp...@apache.org> wrote:
>
> BTW: Backporting r1897460 and r1897472 for child.c of the Windows MPM needs some close look as a number of changes
> to child.c in trunk have not been backported yet:
The gap is too big (for me) between the winnt MPMs on trunk and 2.4.x
for backporting r1897460 and r1897472 easily.
What I did in https://github.com/apache/httpd/pull/289 is to use
ap_thread_current_create() directly in the worker_main() thread, it's
simple, but it does not even compile because
ap_thread_current_create() is not AP_DECLARE/exported anymore, it's
been replaced by ap_thread_main_create() in the latest commits of the
series (since it's how it's used everywhere else).
So we could AP_DECLARE ap_thread_current_create() specifically for
2.4.x, or disable thread locals for pcre2 on Windows, or someone will
have to propose a more complete backport with the below list first:
> 1801146
> 1801147
> 1801635
> 1801636
> 1801637
> 1801639
> 1801640
> 1801655
> 1801656
> 1801657
> 1801710
> 1801747
> 1801748
> 1801749
> 1801753
Regards;
Yann.
Re: svn commit: r1896976 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Eric Covener <co...@gmail.com>.
> makes sense as well (Add compiled and loaded PCRE version numbers to "httpd -V" output and to mod_info page).
>
> Question is if this should be a separate backport or merged with this backport. If it is a separate backport, we
> have an order dependency between these backports.
Based on your description, I think it's OK to do them together
Re: svn commit: r1896976 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Ruediger Pluem <rp...@apache.org>.
On 2/1/22 12:53 AM, Yann Ylavic wrote:
> On Mon, Jan 31, 2022 at 12:57 PM Ruediger Pluem <rp...@apache.org> wrote:
>>
>> On 1/31/22 10:10 AM, Ruediger Pluem wrote:
>>>
>>> Are we now finished with improving the PCRE2 capabilities with regards to allocation / performance?
>>>
>>> I see the following revisions that would need to be backported on top of the above patch:
>>>
>>> 1897240
>>> 1897241
>>> 1897244
>>> 1897248
>>> 1897250
>>> 1897260
>>> 1897261
>>> 1897263
>>> 1897386
>>> 1897459
>>> 1897460
>>> 1897461
>>> 1897462
>>> 1897472
>>> 1897543
>>>
>>>
>>> Did I miss any?
>
> r1897242 :)
>
>>
>> Looking further into this I guess backporting
>>
>> 1612934
>> 1612940
>> 1613189
>>
>> makes sense as well (Add compiled and loaded PCRE version numbers to "httpd -V" output and to mod_info page).
>>
>> Question is if this should be a separate backport or merged with this backport. If it is a separate backport, we
>> have an order dependency between these backports.
>
> I have created https://github.com/apache/httpd/pull/289 with all of
> those, the first commit being the current backport proposal (once it's
> backported I can still rebase).
Thanks. I will have a look.
> The whole works for me, I'll +1 the current proposal first (even
> though I'd prefer that we build with pcre1 by default if both are
> available) and hopefully we can have the second series voted in the
> same release..
As I expect that the TLS approach will be available in most cases I am fine with switching the default. We just should state
this clearly in the CHANGES entry. If someone experiences issues with PCRE2 he can still switch back to PCRE.
Regards
Rüdiger
Re: svn commit: r1896976 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Yann Ylavic <yl...@gmail.com>.
On Mon, Jan 31, 2022 at 12:57 PM Ruediger Pluem <rp...@apache.org> wrote:
>
> On 1/31/22 10:10 AM, Ruediger Pluem wrote:
> >
> > Are we now finished with improving the PCRE2 capabilities with regards to allocation / performance?
> >
> > I see the following revisions that would need to be backported on top of the above patch:
> >
> > 1897240
> > 1897241
> > 1897244
> > 1897248
> > 1897250
> > 1897260
> > 1897261
> > 1897263
> > 1897386
> > 1897459
> > 1897460
> > 1897461
> > 1897462
> > 1897472
> > 1897543
> >
> >
> > Did I miss any?
r1897242 :)
>
> Looking further into this I guess backporting
>
> 1612934
> 1612940
> 1613189
>
> makes sense as well (Add compiled and loaded PCRE version numbers to "httpd -V" output and to mod_info page).
>
> Question is if this should be a separate backport or merged with this backport. If it is a separate backport, we
> have an order dependency between these backports.
I have created https://github.com/apache/httpd/pull/289 with all of
those, the first commit being the current backport proposal (once it's
backported I can still rebase).
The whole works for me, I'll +1 the current proposal first (even
though I'd prefer that we build with pcre1 by default if both are
available) and hopefully we can have the second series voted in the
same release..
Regards;
Yann.
Re: svn commit: r1896976 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Ruediger Pluem <rp...@apache.org>.
On 1/31/22 10:10 AM, Ruediger Pluem wrote:
>
>
> On 1/13/22 4:28 AM, wrowe@apache.org wrote:
>> Author: wrowe
>> Date: Thu Jan 13 03:28:31 2022
>> New Revision: 1896976
>>
>> URL: http://svn.apache.org/viewvc?rev=1896976&view=rev
>> Log:
>> Propose pcre2 support for backport
>>
>> Modified:
>> httpd/httpd/branches/2.4.x/STATUS
>>
>> Modified: httpd/httpd/branches/2.4.x/STATUS
>> URL: http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/STATUS?rev=1896976&r1=1896975&r2=1896976&view=diff
>> ==============================================================================
>> --- httpd/httpd/branches/2.4.x/STATUS (original)
>> +++ httpd/httpd/branches/2.4.x/STATUS Thu Jan 13 03:28:31 2022
>> @@ -150,6 +150,35 @@ PATCHES ACCEPTED TO BACKPORT FROM TRUNK:
>> PATCHES PROPOSED TO BACKPORT FROM TRUNK:
>> [ New proposals should be added at the end of the list ]
>>
>> + *) Support PCRE2 (10.x) in place of PCRE (8.x).
>> + Submitted by: wrowe, Petr Pisar [ppisar redhat.com], rjung
>> + trunk patches:
>> + http://svn.apache.org/r1773454
>> + http://svn.apache.org/r1773741
>> + http://svn.apache.org/r1773742
>> + http://svn.apache.org/r1773839
>> + http://svn.apache.org/r1773870
>> + http://svn.apache.org/r1773882
>> + http://svn.apache.org/r1814662
>> + http://svn.apache.org/r1881478
>> + summarized/conflicts resolved:
>> + https://gist.github.com/wrowe/73f655d13bbe0f12030aa4557e804d8a
>> + +1: wrowe
>> + wrowe notes that the current code drops optimizations, owing to the fact
>> + that the ovector is a required allocation and is no longer allocated on
>> + the stack, by design. The correct fix is an apr userdata allocation on
>> + the appropriate pool, which would be thread-safe, but the actual API of
>> + ap_regexec[_len]() offers us no pool and isn't suitable for httpd-2.4.x.
>> + At this time, PCRE 8.45 is EOL and will not receive security updates,
>> + and taking ovector and other arrays off the stack was in direct reaction
>> + to the patterns of abuse of previous pcre exploits. So this patch doesn't
>> + wait for httpd-2.4 to be retired, it will need to be adopted without the
>> + existing optimiation.
>> + jorton: Adding ap_pregexec/_len which pass a pool would also work
>> + for internal users of this api; not sure if performance
>> + impact is significant from using malloc here.
>
>
> Are we now finished with improving the PCRE2 capabilities with regards to allocation / performance?
>
> I see the following revisions that would need to be backported on top of the above patch:
>
> 1897240
> 1897241
> 1897244
> 1897248
> 1897250
> 1897260
> 1897261
> 1897263
> 1897386
> 1897459
> 1897460
> 1897461
> 1897462
> 1897472
> 1897543
>
>
> Did I miss any?
Looking further into this I guess backporting
1612934
1612940
1613189
makes sense as well (Add compiled and loaded PCRE version numbers to "httpd -V" output and to mod_info page).
Question is if this should be a separate backport or merged with this backport. If it is a separate backport, we
have an order dependency between these backports.
Regards
Rüdiger
Re: svn commit: r1896976 - /httpd/httpd/branches/2.4.x/STATUS
Posted by Ruediger Pluem <rp...@apache.org>.
On 1/13/22 4:28 AM, wrowe@apache.org wrote:
> Author: wrowe
> Date: Thu Jan 13 03:28:31 2022
> New Revision: 1896976
>
> URL: http://svn.apache.org/viewvc?rev=1896976&view=rev
> Log:
> Propose pcre2 support for backport
>
> Modified:
> httpd/httpd/branches/2.4.x/STATUS
>
> Modified: httpd/httpd/branches/2.4.x/STATUS
> URL: http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/STATUS?rev=1896976&r1=1896975&r2=1896976&view=diff
> ==============================================================================
> --- httpd/httpd/branches/2.4.x/STATUS (original)
> +++ httpd/httpd/branches/2.4.x/STATUS Thu Jan 13 03:28:31 2022
> @@ -150,6 +150,35 @@ PATCHES ACCEPTED TO BACKPORT FROM TRUNK:
> PATCHES PROPOSED TO BACKPORT FROM TRUNK:
> [ New proposals should be added at the end of the list ]
>
> + *) Support PCRE2 (10.x) in place of PCRE (8.x).
> + Submitted by: wrowe, Petr Pisar [ppisar redhat.com], rjung
> + trunk patches:
> + http://svn.apache.org/r1773454
> + http://svn.apache.org/r1773741
> + http://svn.apache.org/r1773742
> + http://svn.apache.org/r1773839
> + http://svn.apache.org/r1773870
> + http://svn.apache.org/r1773882
> + http://svn.apache.org/r1814662
> + http://svn.apache.org/r1881478
> + summarized/conflicts resolved:
> + https://gist.github.com/wrowe/73f655d13bbe0f12030aa4557e804d8a
> + +1: wrowe
> + wrowe notes that the current code drops optimizations, owing to the fact
> + that the ovector is a required allocation and is no longer allocated on
> + the stack, by design. The correct fix is an apr userdata allocation on
> + the appropriate pool, which would be thread-safe, but the actual API of
> + ap_regexec[_len]() offers us no pool and isn't suitable for httpd-2.4.x.
> + At this time, PCRE 8.45 is EOL and will not receive security updates,
> + and taking ovector and other arrays off the stack was in direct reaction
> + to the patterns of abuse of previous pcre exploits. So this patch doesn't
> + wait for httpd-2.4 to be retired, it will need to be adopted without the
> + existing optimiation.
> + jorton: Adding ap_pregexec/_len which pass a pool would also work
> + for internal users of this api; not sure if performance
> + impact is significant from using malloc here.
Are we now finished with improving the PCRE2 capabilities with regards to allocation / performance?
I see the following revisions that would need to be backported on top of the above patch:
1897240
1897241
1897244
1897248
1897250
1897260
1897261
1897263
1897386
1897459
1897460
1897461
1897462
1897472
1897543
Did I miss any?
BTW: Backporting r1897460 and r1897472 for child.c of the Windows MPM needs some close look as a number of changes
to child.c in trunk have not been backported yet:
1801146
1801147
1801635
1801636
1801637
1801639
1801640
1801655
1801656
1801657
1801710
1801747
1801748
1801749
1801753
Regards
Rüdiger