On Mon, May 2, 2016 at 3:09 AM, Ruediger Pluem <rpl...@apache.org> wrote:
> > On 04/28/2016 10:19 AM, Ruediger Pluem wrote: > > > > On 04/27/2016 08:41 PM, wr...@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); }
