Re: svn commit: r1892874 - in /httpd/httpd/branches/2.4.x: ./ changes-entries/fix_uds_filename.txt modules/proxy/mod_proxy.c modules/proxy/proxy_util.c

[Ruediger Pluem]

On 9/3/21 6:52 PM, yla...@apache.org wrote:
> Author: ylavic
> Date: Fri Sep  3 16:52:38 2021
> New Revision: 1892874
> 
> URL: http://svn.apache.org/viewvc?rev=1892874=rev
> Log:
> Merge r1892814, r1892853 from trunk:
> 
> mod_proxy: Faster unix socket path parsing in the "proxy:" URL.
> 
> The actual r->filename format is "[proxy:]unix:path|url" for UDS, no need to
> strstr(,"unix:") since it's at the start of the string.
> 
> 
> mod_proxy: Follow up to r1892814.
> 
> Save some few cycles in ap_proxy_de_socketfy() too.
> 
> 
> Submitted by: ylavic
> Reviewed by: ylavic, covener, rpluem
> 
> Added:
> httpd/httpd/branches/2.4.x/changes-entries/fix_uds_filename.txt
>   - copied unchanged from r1892814, 
> httpd/httpd/trunk/changes-entries/fix_uds_filename.txt
> Modified:
> httpd/httpd/branches/2.4.x/   (props changed)
> httpd/httpd/branches/2.4.x/modules/proxy/mod_proxy.c
> httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c
> 
> Propchange: httpd/httpd/branches/2.4.x/
> --
>   Merged /httpd/httpd/trunk:r1892814,1892853
> 
> Modified: httpd/httpd/branches/2.4.x/modules/proxy/mod_proxy.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/modules/proxy/mod_proxy.c?rev=1892874=1892873=1892874=diff
> ==
> --- httpd/httpd/branches/2.4.x/modules/proxy/mod_proxy.c (original)
> +++ httpd/httpd/branches/2.4.x/modules/proxy/mod_proxy.c Fri Sep  3 16:52:38 
> 2021
> @@ -1975,7 +1975,7 @@ PROXY_DECLARE(const char *) ap_proxy_de_
>   * the UDS path... ignore it
>   */
>  if (!ap_cstr_casecmpn(url, "unix:", 5) &&
> -((ptr = ap_strchr_c(url, '|')) != NULL)) {
> +((ptr = ap_strchr_c(url + 5, '|')) != NULL)) {
>  /* move past the 'unix:...|' UDS path info */
>  const char *ret, *c;
>  
> 
> Modified: httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c?rev=1892874=1892873=1892874=diff
> ==
> --- httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c (original)
> +++ httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c Fri Sep  3 16:52:38 
> 2021
> @@ -2281,8 +2281,8 @@ static void fix_uds_filename(request_rec
>  if (!r || !r->filename) return;
>  
>  if (!strncmp(r->filename, "proxy:", 6) &&
> -(ptr2 = ap_strcasestr(r->filename, "unix:")) &&
> -(ptr = ap_strchr(ptr2, '|'))) {
> +!ap_cstr_casecmpn(r->filename + 6, "unix:", 5) &&
> +(ptr2 = r->filename + 6 + 5, ptr = ap_strchr(ptr2, '|'))) {

I know that I voted for it to be backported, but after rethinking this 
shouldn't this be

(ptr2 = r->filename + 6, ptr = ap_strchr(ptr2 + 5, '|')))

as ptr2 is later used in

rv = apr_uri_parse(r->pool, ptr2, );

With the old code and with my proposal above ptr2 points to the 'u' of 'unix:' 
with the current code it points to the char after
'unix:' likely //.

Regards

Rüdiger

Re: svn commit: r1892740 - in /httpd/httpd/trunk: changes-entries/ap_proxy_tunnel_run.txt modules/proxy/proxy_util.c

[Ruediger Pluem]

On 9/3/21 6:46 PM, Yann Ylavic wrote:
> On Fri, Sep 3, 2021 at 2:15 PM Ruediger Pluem  wrote:
>>
>> On 9/3/21 1:01 PM, Yann Ylavic wrote:
>>>

>>
>> Still confused.
> 
> Hope the above helps :/

I guess I get it know. Thanks for your detailed explanations.

Regards

Rüdiger

Re: svn commit: r1892740 - in /httpd/httpd/trunk: changes-entries/ap_proxy_tunnel_run.txt modules/proxy/proxy_util.c

[Yann Ylavic]

On Fri, Sep 3, 2021 at 2:15 PM Ruediger Pluem  wrote:
>
> On 9/3/21 1:01 PM, Yann Ylavic wrote:
> >
> > What I want to avoid in this loop is to handle all the POLLIN|POLLERR
> > or POLLOUT|POLLERR or POLLERR alone which depend on the system.
> > So it's written such that there is no "blank" pass, when no
> > POLLIN|POLLHUP or POLLOUT is returned (likely an error condition) we
> > will issue either a read or a write (in that order of preference) to
> > catch the error at the core filters' level.
> > This was missing the POLLOUT|POLLHUP case when the socket is shutdown
> > for read already, meaning that we will never get POLLIN anymore, and
> > this is fixed by this !(reqevents & APR_POLLIN) case to issue a write
> > (well a shutdown for write exactly here) when no read is to be issued
> > below.
> >
> > Does it make sense?
>
> Just to be sure. We need this because
>
> 1. We requested POLLOUT
> 2. We did not get POLLOUT back, but rtnevents might contain POLLHUP and or 
> POLLIN and hence the last condition existing before the
> patch (!(pfd->rtnevents & (APR_POLLIN | APR_POLLHUP))) would not trigger. The 
> new condition ensures in these cases that we flush
> stuff even if blocking and close down write.

We asked for POLLOUT (only) in reqevents but did not get it back in
rtnevents, instead we got POLLHUP|POLLPRI (in the case of PR 65521)
but it could also have been POLLERR or anything we didn't ask for.
In any case when we ask for POLLIN or POLLOUT or both and get back
none of them, the system tells us there is something "wrong" with the
socket (error, reset or EOF). There might be something wrong with
POLLIN or POLLOUT returned too but at least we know what to do when it
happens, here we have no clue based on the returned events only.

I think this can happen whether the connection was half shutdown or
not, at least POLLERR, see below.

>
> This means we could also use
>
> tc->other->down_in == 1
>
> instead of !(tc->pfd->reqevents & APR_POLLIN)
>
> to get the same results?

This is a more restrictive check that would not catch for instance
POLLERR while we asked for POLLOUT (only) on a full duplex connection
still.
We might have !(tc->pfd->reqevents & APR_POLLIN) because tc->other's
output filters have pending data (we stop reading tc in this case to
avoid yet more pending data and risk blocking), so it does not
necessarily mean that the connections are half closed (down_in/out).
Still if we get POLLERR we want to catch the error on write rather
than read, because read is shutdown or suspended.

>
> Or does it need to be !(tc->other->pfd->reqevents & APR_POLLIN) instead of 
> !(tc->pfd->reqevents & APR_POLLIN) ?

No, this is an event on tc (not tc->other), so we need to check
whether tc needs reading and/or writing because we will read from
and/or write to there in this pass.

>
> Still confused.

Hope the above helps :/


Thanks;
Yann.

Fixed: apache/httpd#1872 (candidate-2.4.49 - bbacd79)

[Travis CI]

Build Update for apache/httpd
-

Build: #1872
Status: Fixed

Duration: 16 mins and 57 secs
Commit: bbacd79 (candidate-2.4.49)
Author: Stefan Eissing
Message: Post  tag updates

git-svn-id: 
https://svn.apache.org/repos/asf/httpd/httpd/tags/candidate-2.4.49@1892866 
13f79535-47bb-0310-9956-ffa450edef68

View the changeset: 
https://github.com/apache/httpd/compare/a4dc599605d3...185b02d9fda7

View the full build log and details: 
https://app.travis-ci.com/github/apache/httpd/builds/236895546?utm_medium=notification_source=email


--

You can unsubscribe from build emails from the apache/httpd repository going to 
https://app.travis-ci.com/account/preferences/unsubscribe?repository=16806660_medium=notification_source=email.
Or unsubscribe from *all* email updating your settings at 
https://app.travis-ci.com/account/preferences/unsubscribe?utm_medium=notification_source=email.
Or configure specific recipients for build notifications in your .travis.yml 
file. See https://docs.travis-ci.com/user/notifications.

Re: release?

[ste...@eissing.org]

The v2 release scripts in ^/httpd/dev-tools do now work for me
to create the tarballs, checksums and signatures for a release
vote and push them to dist.apache.org.

The steps after a vote need some more work. I will do that in the
coming days. However, since we can do a vote now, this is not that
pressing.

The "announce.sh" part should stay usable as it is. I have no plans 
to update that one, since I have 0 experience what this really does 
and where any problems/room for improvements are.

Cheers,
Stefan


> Am 02.09.2021 um 16:53 schrieb Ruediger Pluem :
> 
> 
> 
> On 9/2/21 3:06 PM, Eric Covener wrote:
>> Since you are going through this I wanted to mention:
>> 
>> I think the public doc we have should mention everything that's done
>> during ther release, even the security stuff that is somewhat private.
>> The ASF-wide security policy is already public
>> (https://www.apache.org/security/committers.html) and this is just the
>> mechanics of it for us.
>> 
>> Anyone object?  This way we have one linear place to point to.
> 
> +1 Looks sensible. The details of an actual security issue should not be 
> public until we make it so, but the procedure we use can be.
> 
> Regards
> 
> Rüdiger
> 

Fixed: apache/httpd#1871 (candidate-2.4.49 - 5d8a82e)

[Travis CI]

Build Update for apache/httpd
-

Build: #1871
Status: Fixed

Duration: 16 mins and 56 secs
Commit: 5d8a82e (candidate-2.4.49)
Author: Stefan Eissing
Message: Tag branches/2.4.x@1892863 as 2.4.49

git-svn-id: 
https://svn.apache.org/repos/asf/httpd/httpd/tags/candidate-2.4.49@1892865 
13f79535-47bb-0310-9956-ffa450edef68

View the changeset: 
https://github.com/apache/httpd/compare/249100054faf...a4dc599605d3

View the full build log and details: 
https://app.travis-ci.com/github/apache/httpd/builds/236895228?utm_medium=notification_source=email


--

You can unsubscribe from build emails from the apache/httpd repository going to 
https://app.travis-ci.com/account/preferences/unsubscribe?repository=16806660_medium=notification_source=email.
Or unsubscribe from *all* email updating your settings at 
https://app.travis-ci.com/account/preferences/unsubscribe?utm_medium=notification_source=email.
Or configure specific recipients for build notifications in your .travis.yml 
file. See https://docs.travis-ci.com/user/notifications.

Re: svn commit: r1892740 - in /httpd/httpd/trunk: changes-entries/ap_proxy_tunnel_run.txt modules/proxy/proxy_util.c

[Ruediger Pluem]

On 9/3/21 1:01 PM, Yann Ylavic wrote:
> On Tue, Aug 31, 2021 at 9:29 AM Ruediger Pluem  wrote:
>>
>> On 8/30/21 8:04 PM, yla...@apache.org wrote:
>>>
>>> +/* We want to write if we asked for POLLOUT and got:
>>> + * - POLLOUT: the socket is ready for write;
>>> + * - !POLLIN: the socket is in error state (POLLERR) so we let
>>> + *   the user know by failing the write and log, OR the socket
>>> + *   is shutdown for read already (POLLHUP) so we have to
>>> + *   shutdown for write.
>>>   */
>>>  if ((tc->pfd->reqevents & APR_POLLOUT)
>>>  && ((pfd->rtnevents & APR_POLLOUT)
>>> +|| !(tc->pfd->reqevents & APR_POLLIN)
>>
>> Why should we write if we requested POLLOUT did not get POLLOUT back, but 
>> did not request for POLLIN?
> 
> What I want to avoid in this loop is to handle all the POLLIN|POLLERR
> or POLLOUT|POLLERR or POLLERR alone which depend on the system.
> So it's written such that there is no "blank" pass, when no
> POLLIN|POLLHUP or POLLOUT is returned (likely an error condition) we
> will issue either a read or a write (in that order of preference) to
> catch the error at the core filters' level.
> This was missing the POLLOUT|POLLHUP case when the socket is shutdown
> for read already, meaning that we will never get POLLIN anymore, and
> this is fixed by this !(reqevents & APR_POLLIN) case to issue a write
> (well a shutdown for write exactly here) when no read is to be issued
> below.
> 
> Does it make sense?

Just to be sure. We need this because

1. We requested POLLOUT
2. We did not get POLLOUT back, but rtnevents might contain POLLHUP and or 
POLLIN and hence the last condition existing before the
patch (!(pfd->rtnevents & (APR_POLLIN | APR_POLLHUP))) would not trigger. The 
new condition ensures in these cases that we flush
stuff even if blocking and close down write.

This means we could also use

tc->other->down_in == 1

instead of !(tc->pfd->reqevents & APR_POLLIN)

to get the same results?

Or does it need to be !(tc->other->pfd->reqevents & APR_POLLIN) instead of 
!(tc->pfd->reqevents & APR_POLLIN) ?

Still confused.

Regards

Rüdiger

Re: svn commit: r1892852 - in /httpd/dev-tools/v2: common-lib.sh make-candidate.sh make-tars.sh push-tars.sh

[ste...@eissing.org]

> Am 03.09.2021 um 13:46 schrieb Ruediger Pluem :
> 
> 
> 
> On 9/3/21 1:28 PM, ic...@apache.org wrote:
>> Author: icing
>> Date: Fri Sep  3 11:28:23 2021
>> New Revision: 1892852
>> 
>> URL: http://svn.apache.org/viewvc?rev=1892852=rev
>> Log:
>> * all scripts have a -h option
>> * make-tars.sh has option "-s user" to specify the pgp/gpg siging user
>> * push-tars.sh prepares the distribution of the tars for apache.org
>>   - creates/modifies CHANGES and Announcement
>>   - ask for confirmation before "svn commit" of changes
>>   - preps a voting email
>> 
>> 
>> Added:
>>httpd/dev-tools/v2/push-tars.sh
>> Modified:
>>httpd/dev-tools/v2/common-lib.sh
>>httpd/dev-tools/v2/make-candidate.sh
>>httpd/dev-tools/v2/make-tars.sh
>> 
> 
>> 
>> Modified: httpd/dev-tools/v2/make-tars.sh
>> URL: 
>> http://svn.apache.org/viewvc/httpd/dev-tools/v2/make-tars.sh?rev=1892852=1892851=1892852=diff
>> ==
>> --- httpd/dev-tools/v2/make-tars.sh (original)
>> +++ httpd/dev-tools/v2/make-tars.sh Fri Sep  3 11:28:23 2021
>> @@ -21,23 +21,34 @@ set -e
>> 
>> usage () {
>> cat <&2
>> -usage: $0 [version [signing-user]]
>> -  create the release tars from a SVN candidate. The candidate is exported
>> -  to the local dist directory from 'tags/candidate-$version', prepped
>> -  (buildconf, depending srclib, etc.) and then made into tarballs with
>> -  checksums and signatures.
>> -  Options:
>> +usage: $0 [options] [version]
>> +  create the release tars from a SVN candidate. The candidate is expected
>> +  to be found in SVN at 'tags/candidate-$version', as prepared by the
>> +  'make-candidate.sh' script.
>> +  If no version is given, the current version in the checkout is used.
>> +  Arguments:
>> versionas 'm.n.p(-suffix)?', the version to use.
>> -signing-user   the gpg2/gpg/pgp identity used for signing
>> +  Options:
>> +-h print usage information
>> +-s sign-user   use gpg/pgp key from 'sign-user'
>> EOF
>>   exit 1
>> }
>> 
>> source `dirname $0`/common-lib.sh
>> 
>> +signing_user=
>> +while getopts "hy" opt; do
> 
> Shouldn't this be hs instead of hy ?

Keen eyes! Yes, will fix, thanks.

>> +case $opt in
>> +h)  usage
>> +;;
>> +s)  signing_user="$OPTARG"
>> +;;
>> +esac
>> +done
>> +
>> detect_checkout
>> detect_version $1
>> -signing_user=$2
>> 
>> PROJECT=`basename $SVN_BASE`
>> SVN_DEST="tags/candidate-${VERSION}"
>> 
> 
> 
> Regards
> 
> Rüdiger

Re: svn commit: r1892852 - in /httpd/dev-tools/v2: common-lib.sh make-candidate.sh make-tars.sh push-tars.sh

[Ruediger Pluem]

On 9/3/21 1:28 PM, ic...@apache.org wrote:
> Author: icing
> Date: Fri Sep  3 11:28:23 2021
> New Revision: 1892852
> 
> URL: http://svn.apache.org/viewvc?rev=1892852=rev
> Log:
>  * all scripts have a -h option
>  * make-tars.sh has option "-s user" to specify the pgp/gpg siging user
>  * push-tars.sh prepares the distribution of the tars for apache.org
>- creates/modifies CHANGES and Announcement
>- ask for confirmation before "svn commit" of changes
>- preps a voting email
> 
> 
> Added:
> httpd/dev-tools/v2/push-tars.sh
> Modified:
> httpd/dev-tools/v2/common-lib.sh
> httpd/dev-tools/v2/make-candidate.sh
> httpd/dev-tools/v2/make-tars.sh
> 

> 
> Modified: httpd/dev-tools/v2/make-tars.sh
> URL: 
> http://svn.apache.org/viewvc/httpd/dev-tools/v2/make-tars.sh?rev=1892852=1892851=1892852=diff
> ==
> --- httpd/dev-tools/v2/make-tars.sh (original)
> +++ httpd/dev-tools/v2/make-tars.sh Fri Sep  3 11:28:23 2021
> @@ -21,23 +21,34 @@ set -e
>  
>  usage () {
>  cat <&2
> -usage: $0 [version [signing-user]]
> -  create the release tars from a SVN candidate. The candidate is exported
> -  to the local dist directory from 'tags/candidate-$version', prepped
> -  (buildconf, depending srclib, etc.) and then made into tarballs with
> -  checksums and signatures.
> -  Options:
> +usage: $0 [options] [version]
> +  create the release tars from a SVN candidate. The candidate is expected
> +  to be found in SVN at 'tags/candidate-$version', as prepared by the
> +  'make-candidate.sh' script.
> +  If no version is given, the current version in the checkout is used.
> +  Arguments:
>  versionas 'm.n.p(-suffix)?', the version to use.
> -signing-user   the gpg2/gpg/pgp identity used for signing
> +  Options:
> +-h print usage information
> +-s sign-user   use gpg/pgp key from 'sign-user'
>  EOF
>exit 1
>  }
>  
>  source `dirname $0`/common-lib.sh
>  
> +signing_user=
> +while getopts "hy" opt; do

Shouldn't this be hs instead of hy ?

> +case $opt in
> +h)  usage
> +;;
> +s)  signing_user="$OPTARG"
> +;;
> +esac
> +done
> +
>  detect_checkout
>  detect_version $1
> -signing_user=$2
>  
>  PROJECT=`basename $SVN_BASE`
>  SVN_DEST="tags/candidate-${VERSION}"
> 


Regards

Rüdiger

Re: svn commit: r1892740 - in /httpd/httpd/trunk: changes-entries/ap_proxy_tunnel_run.txt modules/proxy/proxy_util.c

[Yann Ylavic]

On Fri, Sep 3, 2021 at 1:01 PM Yann Ylavic  wrote:
>
> What I want to avoid in this loop is to handle all the POLLIN|POLLERR
> or POLLOUT|POLLERR or POLLERR alone which depend on the system.
> So it's written such that there is no "blank" pass, when no
> POLLIN|POLLHUP or POLLOUT is returned (likely an error condition) we
> will issue either a read or a write (in that order of preference) to
> catch the error at the core filters' level.
> This was missing the POLLOUT|POLLHUP case when the socket is shutdown

This was missing the POLLPRI|POLLHUP (PR 65521) ...

> for read already, meaning that we will never get POLLIN anymore, and
> this is fixed by this !(reqevents & APR_POLLIN) case to issue a write
> (well a shutdown for write exactly here) when no read is to be issued
> below.
>
> Does it make sense?
>
> Regards;
> Yann.

Re: svn commit: r1892740 - in /httpd/httpd/trunk: changes-entries/ap_proxy_tunnel_run.txt modules/proxy/proxy_util.c

[Yann Ylavic]

On Fri, Sep 3, 2021 at 12:58 PM Ruediger Pluem  wrote:
>
>
>
> On 9/3/21 12:27 PM, Yann Ylavic wrote:
> > On Tue, Aug 31, 2021 at 9:29 AM Ruediger Pluem  wrote:
> >>
> >> On 8/30/21 8:04 PM, yla...@apache.org wrote:
> >>> +
> >>> +/* Flush any pending input data now, we don't know 
> >>> when
> >>> + * the next POLLIN will trigger and retaining data 
> >>> might
> >>> + * deadlock the underlying protocol. We don't check 
> >>> for
> >>> + * pending data first with ap_filter_input_pending() 
> >>> since
> >>> + * the read from proxy_tunnel_forward() is 
> >>> nonblocking
> >>> + * anyway and returning OK if there's no data.
> >>> + */
> >>> +rc = proxy_tunnel_forward(tunnel, in);
> >>> +if (rc != OK) {
> >>> +return rc;
> >>> +}
> >>
> >> Don't we do all of this already a few lines above if 
> >> ap_filter_input_pending(in->c) == OK?
> >> Why doing it again?
> >
> > With whatever tc (client or origin connection), here on tc->rtnevents
> > & POLLOUT we have out == tc and in == tc->other, so we forward
> > potential pending input data from tc->other to tc (without waiting
> > tc->other to become readable).
> >
> > Below with the same tc (same loop), if tc->rtnevents & POLLIN we
> > forward from tc to tc->other, thus the opposite direction.
>
> I still don't get it. Starting at line 4941 in proxy_util.c we have
>
>
>
> /* Flush any pending input data now, we don't know when
>  * the next POLLIN will trigger and retaining data might
>  * block the protocol.
>  */
> if (ap_filter_input_pending(in->c) == OK) {
> rc = proxy_tunnel_forward(tunnel, in);
> if (rc != OK) {
> return rc;
> }
> }
>
> /* Flush any pending input data now, we don't know when
>  * the next POLLIN will trigger and retaining data might
>  * deadlock the underlying protocol. We don't check for
>  * pending data first with ap_filter_input_pending() since
>  * the read from proxy_tunnel_forward() is nonblocking
>  * anyway and returning OK if there's no data.
>  */
> rc = proxy_tunnel_forward(tunnel, in);
> if (rc != OK) {
> return rc;
> }
>
>
> We execute the same code
>
>
> rc = proxy_tunnel_forward(tunnel, in);
> if (rc != OK) {
> return rc;
> }
>
> twice if ap_filter_input_pending(in->c) == OK. Even the comments above these 
> two executions
> start with the same wording. I still ask me why we need to do it twice.

Argh sorry, I thought you were talking about the
proxy_tunnel_forward() in the POLLIN case, because I didn't see that I
had left the old code in (not visible in the diff..).

I think I got it in r1892851 :)

Thanks!

Re: svn commit: r1892740 - in /httpd/httpd/trunk: changes-entries/ap_proxy_tunnel_run.txt modules/proxy/proxy_util.c

[Yann Ylavic]

On Tue, Aug 31, 2021 at 9:29 AM Ruediger Pluem  wrote:
>
> On 8/30/21 8:04 PM, yla...@apache.org wrote:
> >
> > +/* We want to write if we asked for POLLOUT and got:
> > + * - POLLOUT: the socket is ready for write;
> > + * - !POLLIN: the socket is in error state (POLLERR) so we let
> > + *   the user know by failing the write and log, OR the socket
> > + *   is shutdown for read already (POLLHUP) so we have to
> > + *   shutdown for write.
> >   */
> >  if ((tc->pfd->reqevents & APR_POLLOUT)
> >  && ((pfd->rtnevents & APR_POLLOUT)
> > +|| !(tc->pfd->reqevents & APR_POLLIN)
>
> Why should we write if we requested POLLOUT did not get POLLOUT back, but did 
> not request for POLLIN?

What I want to avoid in this loop is to handle all the POLLIN|POLLERR
or POLLOUT|POLLERR or POLLERR alone which depend on the system.
So it's written such that there is no "blank" pass, when no
POLLIN|POLLHUP or POLLOUT is returned (likely an error condition) we
will issue either a read or a write (in that order of preference) to
catch the error at the core filters' level.
This was missing the POLLOUT|POLLHUP case when the socket is shutdown
for read already, meaning that we will never get POLLIN anymore, and
this is fixed by this !(reqevents & APR_POLLIN) case to issue a write
(well a shutdown for write exactly here) when no read is to be issued
below.

Does it make sense?

Regards;
Yann.

Re: svn commit: r1892740 - in /httpd/httpd/trunk: changes-entries/ap_proxy_tunnel_run.txt modules/proxy/proxy_util.c

[Ruediger Pluem]

On 9/3/21 12:27 PM, Yann Ylavic wrote:
> On Tue, Aug 31, 2021 at 9:29 AM Ruediger Pluem  wrote:
>>
>> On 8/30/21 8:04 PM, yla...@apache.org wrote:
>>> +
>>> +/* Flush any pending input data now, we don't know when
>>> + * the next POLLIN will trigger and retaining data 
>>> might
>>> + * deadlock the underlying protocol. We don't check for
>>> + * pending data first with ap_filter_input_pending() 
>>> since
>>> + * the read from proxy_tunnel_forward() is nonblocking
>>> + * anyway and returning OK if there's no data.
>>> + */
>>> +rc = proxy_tunnel_forward(tunnel, in);
>>> +if (rc != OK) {
>>> +return rc;
>>> +}
>>
>> Don't we do all of this already a few lines above if 
>> ap_filter_input_pending(in->c) == OK?
>> Why doing it again?
> 
> With whatever tc (client or origin connection), here on tc->rtnevents
> & POLLOUT we have out == tc and in == tc->other, so we forward
> potential pending input data from tc->other to tc (without waiting
> tc->other to become readable).
> 
> Below with the same tc (same loop), if tc->rtnevents & POLLIN we
> forward from tc to tc->other, thus the opposite direction.

I still don't get it. Starting at line 4941 in proxy_util.c we have



/* Flush any pending input data now, we don't know when
 * the next POLLIN will trigger and retaining data might
 * block the protocol.
 */
if (ap_filter_input_pending(in->c) == OK) {
rc = proxy_tunnel_forward(tunnel, in);
if (rc != OK) {
return rc;
}
}

/* Flush any pending input data now, we don't know when
 * the next POLLIN will trigger and retaining data might
 * deadlock the underlying protocol. We don't check for
 * pending data first with ap_filter_input_pending() since
 * the read from proxy_tunnel_forward() is nonblocking
 * anyway and returning OK if there's no data.
 */
rc = proxy_tunnel_forward(tunnel, in);
if (rc != OK) {
return rc;
}


We execute the same code


rc = proxy_tunnel_forward(tunnel, in);
if (rc != OK) {
return rc;
}

twice if ap_filter_input_pending(in->c) == OK. Even the comments above these 
two executions
start with the same wording. I still ask me why we need to do it twice.

Regards

Rüdiger

Re: Broken: apache/httpd#1862 (2.4.x - b15b9a5)

[ste...@eissing.org]

> Am 03.09.2021 um 11:57 schrieb Ruediger Pluem :
> 
> 
> 
> On 9/3/21 11:17 AM, Travis CI wrote:
>> apache
>> 
>> /
>> 
>> httpd
>> 
>> 
>> 
>> branch icon2.4.x 
>> 
>> build has failed
>> Build #1862 was broken 
>> 
>> arrow to build time
>> clock icon26 mins and 38 secs
>> 
> 
> It is a read after free in mod_http2. Do we miss a backport?

Yes. It was a "wait and see what travis says" and then never picked up.

> ==64184==ERROR: AddressSanitizer: heap-use-after-free on address 
> 0x62500dcca9a0 at pc 0x7fe9704ad024 bp 0x7fe952100d30 sp
> 0x7fe952100d20
> 
> READ of size 4 at 0x62500dcca9a0 thread T54
> 
>#0 0x7fe9704ad023 in mst_check_data_for 
> /home/travis/build/apache/httpd/modules/http2/h2_mplx.c:618
> 
>#1 0x7fe9704b1807 in s_task_done 
> /home/travis/build/apache/httpd/modules/http2/h2_mplx.c:811
> 
>#2 0x7fe9704b1807 in h2_mplx_s_task_done 
> /home/travis/build/apache/httpd/modules/http2/h2_mplx.c:842
> 
>#3 0x7fe9704f0cd7 in slot_run 
> /home/travis/build/apache/httpd/modules/http2/h2_workers.c:245
> 
>#4 0x7fe974fba47a in dummy_worker threadproc/unix/thread.c:147
> 
>#5 0x7fe974f2d608 in start_thread 
> (/lib/x86_64-linux-gnu/libpthread.so.0+0x9608)
> 
>#6 0x7fe974e54292 in __clone (/lib/x86_64-linux-gnu/libc.so.6+0x122292)
> 
> 0x62500dcca9a0 is located 160 bytes inside of 8192-byte region 
> [0x62500dcca900,0x62500dccc900)
> 
> freed by thread T59 here:
> 
>#0 0x7fe9751ed7cf in __interceptor_free 
> (/lib/x86_64-linux-gnu/libasan.so.5+0x10d7cf)
> 
>#1 0x7fe974f97926 in allocator_free memory/unix/apr_pools.c:492
> 
>#2 0x7fe974f97926 in apr_pool_destroy memory/unix/apr_pools.c:1026
> 
>#3 0x7fe9704ab589 in m_stream_destroy_iter 
> /home/travis/build/apache/httpd/modules/http2/h2_mplx.c:316
> 
>#4 0x7fe974f7a025 in apr_hash_do tables/apr_hash.c:542
> 
>#5 0x7fe9704ea299 in h2_ihash_iter 
> /home/travis/build/apache/httpd/modules/http2/h2_util.c:283
> 
>#6 0x7fe9704b415f in m_purge_streams 
> /home/travis/build/apache/httpd/modules/http2/h2_mplx.c:324
> 
>#7 0x7fe9704b415f in m_purge_streams 
> /home/travis/build/apache/httpd/modules/http2/h2_mplx.c:320
> 
>#8 0x7fe9704b415f in h2_mplx_m_dispatch_master_events 
> /home/travis/build/apache/httpd/modules/http2/h2_mplx.c:1098
> 
>#9 0x7fe9704cd643 in dispatch_master 
> /home/travis/build/apache/httpd/modules/http2/h2_session.c:2078
> 
>#10 0x7fe9704cd643 in h2_session_process 
> /home/travis/build/apache/httpd/modules/http2/h2_session.c:2278
> 
>#11 0x7fe97049336c in h2_conn_run 
> /home/travis/build/apache/httpd/modules/http2/h2_conn.c:214
> 
>#12 0x7fe9704a7ace in h2_h2_process_conn 
> /home/travis/build/apache/httpd/modules/http2/h2_h2.c:631
> 
>#13 0x7fe9704a7ace in h2_h2_process_conn 
> /home/travis/build/apache/httpd/modules/http2/h2_h2.c:549
> 
>#14 0x56201065b6be in ap_run_process_connection 
> /home/travis/build/apache/httpd/server/connection.c:42
> 
>#15 0x56201069b6ef in process_socket 
> /home/travis/build/apache/httpd/server/mpm/event/event.c:1053
> 
>#16 0x56201069d367 in worker_thread 
> /home/travis/build/apache/httpd/server/mpm/event/event.c:2142
> 
>#17 0x7fe974fba47a in dummy_worker threadproc/unix/thread.c:147
> 
>#18 0x7fe974f2d608 in start_thread 
> (/lib/x86_64-linux-gnu/libpthread.so.0+0x9608)
> 
> 
> 
> Regards
> 
> Rüdiger

Re: svn commit: r1892740 - in /httpd/httpd/trunk: changes-entries/ap_proxy_tunnel_run.txt modules/proxy/proxy_util.c

[Yann Ylavic]

On Tue, Aug 31, 2021 at 9:29 AM Ruediger Pluem  wrote:
>
> On 8/30/21 8:04 PM, yla...@apache.org wrote:
> > +
> > +/* Flush any pending input data now, we don't know when
> > + * the next POLLIN will trigger and retaining data 
> > might
> > + * deadlock the underlying protocol. We don't check for
> > + * pending data first with ap_filter_input_pending() 
> > since
> > + * the read from proxy_tunnel_forward() is nonblocking
> > + * anyway and returning OK if there's no data.
> > + */
> > +rc = proxy_tunnel_forward(tunnel, in);
> > +if (rc != OK) {
> > +return rc;
> > +}
>
> Don't we do all of this already a few lines above if 
> ap_filter_input_pending(in->c) == OK?
> Why doing it again?

With whatever tc (client or origin connection), here on tc->rtnevents
& POLLOUT we have out == tc and in == tc->other, so we forward
potential pending input data from tc->other to tc (without waiting
tc->other to become readable).

Below with the same tc (same loop), if tc->rtnevents & POLLIN we
forward from tc to tc->other, thus the opposite direction.


Regards;
Yann.

Re: Broken: apache/httpd#1862 (2.4.x - b15b9a5)

[Ruediger Pluem]

On 9/3/21 11:17 AM, Travis CI wrote:
> apache
> 
> /
> 
> httpd
> 
> 
> 
> branch icon2.4.x 
> 
> build has failed
> Build #1862 was broken 
> 
> arrow to build time
> clock icon26 mins and 38 secs
> 

It is a read after free in mod_http2. Do we miss a backport?

==64184==ERROR: AddressSanitizer: heap-use-after-free on address 0x62500dcca9a0 
at pc 0x7fe9704ad024 bp 0x7fe952100d30 sp
0x7fe952100d20

READ of size 4 at 0x62500dcca9a0 thread T54

#0 0x7fe9704ad023 in mst_check_data_for 
/home/travis/build/apache/httpd/modules/http2/h2_mplx.c:618

#1 0x7fe9704b1807 in s_task_done 
/home/travis/build/apache/httpd/modules/http2/h2_mplx.c:811

#2 0x7fe9704b1807 in h2_mplx_s_task_done 
/home/travis/build/apache/httpd/modules/http2/h2_mplx.c:842

#3 0x7fe9704f0cd7 in slot_run 
/home/travis/build/apache/httpd/modules/http2/h2_workers.c:245

#4 0x7fe974fba47a in dummy_worker threadproc/unix/thread.c:147

#5 0x7fe974f2d608 in start_thread 
(/lib/x86_64-linux-gnu/libpthread.so.0+0x9608)

#6 0x7fe974e54292 in __clone (/lib/x86_64-linux-gnu/libc.so.6+0x122292)

0x62500dcca9a0 is located 160 bytes inside of 8192-byte region 
[0x62500dcca900,0x62500dccc900)

freed by thread T59 here:

#0 0x7fe9751ed7cf in __interceptor_free 
(/lib/x86_64-linux-gnu/libasan.so.5+0x10d7cf)

#1 0x7fe974f97926 in allocator_free memory/unix/apr_pools.c:492

#2 0x7fe974f97926 in apr_pool_destroy memory/unix/apr_pools.c:1026

#3 0x7fe9704ab589 in m_stream_destroy_iter 
/home/travis/build/apache/httpd/modules/http2/h2_mplx.c:316

#4 0x7fe974f7a025 in apr_hash_do tables/apr_hash.c:542

#5 0x7fe9704ea299 in h2_ihash_iter 
/home/travis/build/apache/httpd/modules/http2/h2_util.c:283

#6 0x7fe9704b415f in m_purge_streams 
/home/travis/build/apache/httpd/modules/http2/h2_mplx.c:324

#7 0x7fe9704b415f in m_purge_streams 
/home/travis/build/apache/httpd/modules/http2/h2_mplx.c:320

#8 0x7fe9704b415f in h2_mplx_m_dispatch_master_events 
/home/travis/build/apache/httpd/modules/http2/h2_mplx.c:1098

#9 0x7fe9704cd643 in dispatch_master 
/home/travis/build/apache/httpd/modules/http2/h2_session.c:2078

#10 0x7fe9704cd643 in h2_session_process 
/home/travis/build/apache/httpd/modules/http2/h2_session.c:2278

#11 0x7fe97049336c in h2_conn_run 
/home/travis/build/apache/httpd/modules/http2/h2_conn.c:214

#12 0x7fe9704a7ace in h2_h2_process_conn 
/home/travis/build/apache/httpd/modules/http2/h2_h2.c:631

#13 0x7fe9704a7ace in h2_h2_process_conn 
/home/travis/build/apache/httpd/modules/http2/h2_h2.c:549

#14 0x56201065b6be in ap_run_process_connection 
/home/travis/build/apache/httpd/server/connection.c:42

#15 0x56201069b6ef in process_socket 
/home/travis/build/apache/httpd/server/mpm/event/event.c:1053

#16 0x56201069d367 in worker_thread 
/home/travis/build/apache/httpd/server/mpm/event/event.c:2142

#17 0x7fe974fba47a in dummy_worker threadproc/unix/thread.c:147

#18 0x7fe974f2d608 in start_thread 
(/lib/x86_64-linux-gnu/libpthread.so.0+0x9608)



Regards

Rüdiger

Broken: apache/httpd#1862 (2.4.x - b15b9a5)

[Travis CI]

Build Update for apache/httpd
-

Build: #1862
Status: Broken

Duration: 26 mins and 38 secs
Commit: b15b9a5 (2.4.x)
Author: Nilgun Belma Buguner
Message: update transformations. 

git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/branches/2.4.x@1892842 
13f79535-47bb-0310-9956-ffa450edef68

View the changeset: 
https://github.com/apache/httpd/compare/65fcb0194c86...b15b9a590288

View the full build log and details: 
https://app.travis-ci.com/github/apache/httpd/builds/236874643?utm_medium=notification_source=email


--

You can unsubscribe from build emails from the apache/httpd repository going to 
https://app.travis-ci.com/account/preferences/unsubscribe?repository=16806660_medium=notification_source=email.
Or unsubscribe from *all* email updating your settings at 
https://app.travis-ci.com/account/preferences/unsubscribe?utm_medium=notification_source=email.
Or configure specific recipients for build notifications in your .travis.yml 
file. See https://docs.travis-ci.com/user/notifications.

Re: Failed: apache/httpd#1856 (candidate-2.4.49 - cd5b61d)

[Joe Orton]

I think this is fixed with both r1892841, r1892845 merged but I am still 
slightly unsure about when things appear as "tags" in Travis and when as 
"branches", so it might need further tweaks.

Regards, Joe

Re: Failed: apache/httpd#1856 (candidate-2.4.49 - cd5b61d)

[Joe Orton]

On Thu, Sep 02, 2021 at 01:38:55PM -0400, Eric Covener wrote:
> I think there are some jobs that are excluded if not "2.4.x", so it's
> doing some trunk-only stuff.

Yup, we need to adjust the conditions.  I did r1892841 but this is 
wrong, it needs to catch branches, gimme a minute while a burn a few 
thousand hours of CPU time testing this...

Re: svn commit: r1892841 - /httpd/httpd/trunk/.travis.yml

[ste...@eissing.org]

Thanks, Joe! \o/

> Am 03.09.2021 um 09:54 schrieb jor...@apache.org:
> 
> Author: jorton
> Date: Fri Sep  3 07:54:03 2021
> New Revision: 1892841
> 
> URL: http://svn.apache.org/viewvc?rev=1892841=rev
> Log:
> Adjust Travis conditions for candidate-2.4.x tags.
> 
> Modified:
>httpd/httpd/trunk/.travis.yml
> 
> Modified: httpd/httpd/trunk/.travis.yml
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/.travis.yml?rev=1892841=1892840=1892841=diff
> ==
> --- httpd/httpd/trunk/.travis.yml (original)
> +++ httpd/httpd/trunk/.travis.yml Fri Sep  3 07:54:03 2021
> @@ -38,8 +38,8 @@ env:
> # definitions to either:
> #  condition_24x_only => run the job only for 2.4.x
> #  condition_not_24x  => run the job everywhere EXCEPT 2.4.x
> -_cond1: _24x_only (branch is present AND branch = 2.4.x) OR (tag 
> is present AND tag ~= /^2.4/)
> -_cond2: _not_24x (branch is not present OR branch != 2.4.x) AND 
> (tag is not present OR tag !~ /^2.4/)
> +_cond1: _24x_only (branch is present AND branch = 2.4.x) OR (tag 
> is present AND (tag ~= /^2.4/ OR tag ~= /^candidate-2.4/))
> +_cond2: _not_24x (branch is not present OR branch != 2.4.x) AND 
> (tag is not present OR (tag !~ /^2.4/ AND tag !~ /^candidate-2.4/))
> 
> jobs:
>   include:
> 
>