httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From William A Rowe Jr <wr...@rowe-clan.net>
Subject Re: svn commit: r1741310 - in /httpd/httpd/trunk: modules/http2/ server/ server/mpm/event/ server/mpm/motorz/ server/mpm/simple/ server/mpm/winnt/ server/mpm/worker/
Date Tue, 03 May 2016 15:51:35 GMT
On Mon, May 2, 2016 at 3:09 AM, Ruediger Pluem <rpluem@apache.org> wrote:

>
> On 04/28/2016 10:19 AM, Ruediger Pluem wrote:
> >
> > On 04/27/2016 08:41 PM, wrowe@apache.org wrote:
> >> Author: wrowe
> >> Date: Wed Apr 27 18:41:49 2016
> >> New Revision: 1741310
> >>
> >> URL: http://svn.apache.org/viewvc?rev=1741310&view=rev
> >> Log:
> >>
> >>   Ensure the useragent_ip is only used in the case
> >>   where it has been initialized, fall back on the
> >>   connection's remote_ip if the status is accidently
> >>   updated from an uninitialized request_rec.
> >>
> >> --- httpd/httpd/trunk/server/scoreboard.c (original)
> >> +++ httpd/httpd/trunk/server/scoreboard.c Wed Apr 27 18:41:49 2016
> >> @@ -501,7 +501,7 @@ static int update_child_status_internal(
> >>              copy_request(ws->request, sizeof(ws->request), r);
> >>          }
> >>
> >> -        if (r) {
> >> +        if (r && r->useragent_ip) {
> >>              if (!(val = ap_get_useragent_host(r, REMOTE_NOLOOKUP,
> NULL)))
> >>                  apr_cpystrn(ws->client, r->useragent_ip,
> sizeof(ws->client));
> >
> > Hm, wouldn't it be better to just encapsulate the above line in an if
> (r->useragent_ip)
> > or do we already know that ap_get_useragent_host(r, REMOTE_NOLOOKUP,
> NULL) cannot deliver
> > something meaningful if r->useragent_ip == NULL?
>

It *could* deliver; but if r->useragent_ip is unset, this is
a premature call to set up the score entry, the request
read hook hasn't completed and will fail to deliver.  But
I don't know what you mean by "the above line" - you
mean the apr_cpystrn?  As you point out, the call is
fairly useless without a completed request rec and
known useragent_ip in the first place (you want to
look up the hostname of null address - but then
guard against copying a NULL string once it has
failed ? :-)

By falling back on the conn-based lookup, in the code you
snipped... we are populating the conn-based hostname,
which may be useful datum elsewhere...

        [...]
        else if (c) {
            if (!(val = ap_get_remote_host(c,
c->base_server->lookup_defaults,
                                           REMOTE_NOLOOKUP, NULL)))
                apr_cpystrn(ws->client, c->client_ip, sizeof(ws->client));
            else
                apr_cpystrn(ws->client, val, sizeof(ws->client));

In all cases we should have the value for (c) where (r) is passed,
whether r was fully populated yet or not.

If we want to be more robust, have a look at the current 2.4.x
implementation of ap_get_useragent_host() in core.c...

    if (r->useragent_addr == conn->client_addr) {
        return ap_get_remote_host(conn, r->per_dir_config, type, str_is_ip);
    }

Perhaps this would be better...

    if (!r->useragent_addr || (r->useragent_addr == conn->client_addr)) {
        return ap_get_remote_host(conn, r->per_dir_config, type, str_is_ip);
    }

Mime
View raw message