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 2016/02/12 02:04:58 UTC
svn commit: r1729929 - in /httpd/httpd/trunk: CHANGES include/ap_mmn.h
include/http_core.h include/httpd.h server/core.c
Author: wrowe
Date: Fri Feb 12 01:04:58 2016
New Revision: 1729929
URL: http://svn.apache.org/viewvc?rev=1729929&view=rev
Log:
Introduce an ap_get_useragent_host() accessor to replace the old
ap_get_remote_host() in most applications, but preserve the original
behavior for all ap_get_remote_host() consumers (mostly, because we
don't have the request_rec in the first place, and also to avoid any
unintended consequences).
This accessor continues to store the remote_host of connection based
uesr agents within the conn_rec for optimization. Only where some
other module modifies the useragent_addr will we perform a per-request
query of the remote_host.
(Fixed compilation issues noted by Ranier, applies to 2.4.x trunk,
modulo CHANGES and ap_mmn.h)
Modified:
httpd/httpd/trunk/CHANGES
httpd/httpd/trunk/include/ap_mmn.h
httpd/httpd/trunk/include/http_core.h
httpd/httpd/trunk/include/httpd.h
httpd/httpd/trunk/server/core.c
Modified: httpd/httpd/trunk/CHANGES
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?rev=1729929&r1=1729928&r2=1729929&view=diff
==============================================================================
--- httpd/httpd/trunk/CHANGES [utf-8] (original)
+++ httpd/httpd/trunk/CHANGES [utf-8] Fri Feb 12 01:04:58 2016
@@ -1,6 +1,11 @@
-*- coding: utf-8 -*-
Changes with Apache 2.5.0
+ *) core: Track the useragent_host per-request when mod_remoteip or similar
+ modules track a per-request useragent_ip. Modules should be updated
+ to inquire for ap_get_useragent_host() in place of ap_get_remote_host().
+ [William Rowe]
+
*) mod_proxy: Play/restore the TLS-SNI on new backend connections which
had to be issued because the remote closed the previous/reusable one
during idle (keep-alive) time. [Yann Ylavic]
Modified: httpd/httpd/trunk/include/ap_mmn.h
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/include/ap_mmn.h?rev=1729929&r1=1729928&r2=1729929&view=diff
==============================================================================
--- httpd/httpd/trunk/include/ap_mmn.h (original)
+++ httpd/httpd/trunk/include/ap_mmn.h Fri Feb 12 01:04:58 2016
@@ -500,6 +500,8 @@
* 20150222.9 (2.5.0-dev) Add epxr_hander to core_dir_config.
* 20150222.10 (2.5.0-dev) Add new ap_update_child_status...() methods,
* add protocol to worker_score in scoreboard.h
+ * 20150222.11 (2.5.0-dev) Split useragent_host from the conn_rec into
+ * the request_rec, with ap_get_useragent_host()
*/
#define MODULE_MAGIC_COOKIE 0x41503235UL /* "AP25" */
@@ -507,7 +509,7 @@
#ifndef MODULE_MAGIC_NUMBER_MAJOR
#define MODULE_MAGIC_NUMBER_MAJOR 20150222
#endif
-#define MODULE_MAGIC_NUMBER_MINOR 10 /* 0...n */
+#define MODULE_MAGIC_NUMBER_MINOR 11 /* 0...n */
/**
* Determine if the server's current MODULE_MAGIC_NUMBER is at least a
Modified: httpd/httpd/trunk/include/http_core.h
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/include/http_core.h?rev=1729929&r1=1729928&r2=1729929&view=diff
==============================================================================
--- httpd/httpd/trunk/include/http_core.h (original)
+++ httpd/httpd/trunk/include/http_core.h Fri Feb 12 01:04:58 2016
@@ -159,6 +159,32 @@ AP_DECLARE(int) ap_allow_overrides(reque
AP_DECLARE(const char *) ap_document_root(request_rec *r);
/**
+ * Lookup the remote user agent's DNS name or IP address
+ * @ingroup get_remote_hostname
+ * @param req The current request
+ * @param type The type of lookup to perform. One of:
+ * <pre>
+ * REMOTE_HOST returns the hostname, or NULL if the hostname
+ * lookup fails. It will force a DNS lookup according to the
+ * HostnameLookups setting.
+ * REMOTE_NAME returns the hostname, or the dotted quad if the
+ * hostname lookup fails. It will force a DNS lookup according
+ * to the HostnameLookups setting.
+ * REMOTE_NOLOOKUP is like REMOTE_NAME except that a DNS lookup is
+ * never forced.
+ * REMOTE_DOUBLE_REV will always force a DNS lookup, and also force
+ * a double reverse lookup, regardless of the HostnameLookups
+ * setting. The result is the (double reverse checked)
+ * hostname, or NULL if any of the lookups fail.
+ * </pre>
+ * @param str_is_ip unless NULL is passed, this will be set to non-zero on
+ * output when an IP address string is returned
+ * @return The remote hostname (based on the request useragent_ip)
+ */
+AP_DECLARE(const char *) ap_get_useragent_host(request_rec *req, int type,
+ int *str_is_ip);
+
+/**
* Lookup the remote client's DNS name or IP address
* @ingroup get_remote_host
* @param conn The current connection
@@ -180,7 +206,7 @@ AP_DECLARE(const char *) ap_document_roo
* </pre>
* @param str_is_ip unless NULL is passed, this will be set to non-zero on output when an IP address
* string is returned
- * @return The remote hostname
+ * @return The remote hostname (based on the connection client_ip)
*/
AP_DECLARE(const char *) ap_get_remote_host(conn_rec *conn, void *dir_config, int type, int *str_is_ip);
Modified: httpd/httpd/trunk/include/httpd.h
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/include/httpd.h?rev=1729929&r1=1729928&r2=1729929&view=diff
==============================================================================
--- httpd/httpd/trunk/include/httpd.h (original)
+++ httpd/httpd/trunk/include/httpd.h Fri Feb 12 01:04:58 2016
@@ -1056,6 +1056,16 @@ struct request_rec {
apr_table_t *trailers_in;
/** MIME trailer environment from the response */
apr_table_t *trailers_out;
+
+ /** Originator's DNS name, if known. NULL if DNS hasn't been checked,
+ * "" if it has and no address was found. N.B. Only access this though
+ * ap_get_useragent_host() */
+ char *useragent_host;
+ /** have we done double-reverse DNS? -1 yes/failure, 0 not yet,
+ * 1 yes/success
+ * TODO: 2 bit signed bitfield when this structure is compacted
+ */
+ int double_reverse;
};
/**
Modified: httpd/httpd/trunk/server/core.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/core.c?rev=1729929&r1=1729928&r2=1729929&view=diff
==============================================================================
--- httpd/httpd/trunk/server/core.c (original)
+++ httpd/httpd/trunk/server/core.c Fri Feb 12 01:04:58 2016
@@ -887,35 +887,36 @@ char *ap_response_code_string(request_re
/* Code from Harald Hanche-Olsen <ha...@imf.unit.no> */
-static APR_INLINE void do_double_reverse (conn_rec *conn)
+static APR_INLINE int do_double_reverse (int double_reverse,
+ const char *remote_host,
+ apr_sockaddr_t *client_addr,
+ apr_pool_t *pool)
{
apr_sockaddr_t *sa;
apr_status_t rv;
- if (conn->double_reverse) {
+ if (double_reverse) {
/* already done */
- return;
+ return double_reverse;
}
- if (conn->remote_host == NULL || conn->remote_host[0] == '\0') {
+ if (remote_host == NULL || remote_host[0] == '\0') {
/* single reverse failed, so don't bother */
- conn->double_reverse = -1;
- return;
+ return -1;
}
- rv = apr_sockaddr_info_get(&sa, conn->remote_host, APR_UNSPEC, 0, 0, conn->pool);
+ rv = apr_sockaddr_info_get(&sa, remote_host, APR_UNSPEC, 0, 0, pool);
if (rv == APR_SUCCESS) {
while (sa) {
- if (apr_sockaddr_equal(sa, conn->client_addr)) {
- conn->double_reverse = 1;
- return;
+ if (apr_sockaddr_equal(sa, client_addr)) {
+ return 1;
}
sa = sa->next;
}
}
- conn->double_reverse = -1;
+ return -1;
}
AP_DECLARE(const char *) ap_get_remote_host(conn_rec *conn, void *dir_config,
@@ -953,7 +954,10 @@ AP_DECLARE(const char *) ap_get_remote_h
ap_str_tolower(conn->remote_host);
if (hostname_lookups == HOSTNAME_LOOKUP_DOUBLE) {
- do_double_reverse(conn);
+ conn->double_reverse = do_double_reverse(conn->double_reverse,
+ conn->remote_host,
+ conn->client_addr,
+ conn->pool);
if (conn->double_reverse != 1) {
conn->remote_host = NULL;
}
@@ -967,7 +971,9 @@ AP_DECLARE(const char *) ap_get_remote_h
}
if (type == REMOTE_DOUBLE_REV) {
- do_double_reverse(conn);
+ conn->double_reverse = do_double_reverse(conn->double_reverse,
+ conn->remote_host,
+ conn->client_addr, conn->pool);
if (conn->double_reverse == -1) {
return NULL;
}
@@ -991,6 +997,83 @@ AP_DECLARE(const char *) ap_get_remote_h
}
}
}
+
+AP_DECLARE(const char *) ap_get_useragent_host(request_rec *r,
+ int type, int *str_is_ip)
+{
+ conn_rec *conn = r->connection;
+ int hostname_lookups;
+ int ignored_str_is_ip;
+
+ if (r->useragent_addr == conn->client_addr) {
+ return ap_get_remote_host(conn, r->per_dir_config, type, str_is_ip);
+ }
+
+ if (!str_is_ip) { /* caller doesn't want to know */
+ str_is_ip = &ignored_str_is_ip;
+ }
+ *str_is_ip = 0;
+
+ hostname_lookups = ((core_dir_config *)
+ ap_get_core_module_config(r->per_dir_config))
+ ->hostname_lookups;
+ if (hostname_lookups == HOSTNAME_LOOKUP_UNSET) {
+ hostname_lookups = HOSTNAME_LOOKUP_OFF;
+ }
+
+ if (type != REMOTE_NOLOOKUP
+ && r->useragent_host == NULL
+ && (type == REMOTE_DOUBLE_REV
+ || hostname_lookups != HOSTNAME_LOOKUP_OFF)) {
+
+ if (apr_getnameinfo(&r->useragent_host, r->useragent_addr, 0)
+ == APR_SUCCESS) {
+ ap_str_tolower(r->useragent_host);
+
+ if (hostname_lookups == HOSTNAME_LOOKUP_DOUBLE) {
+ r->double_reverse = do_double_reverse(r->double_reverse,
+ r->useragent_host,
+ r->useragent_addr,
+ r->pool);
+ if (r->double_reverse != 1) {
+ r->useragent_host = NULL;
+ }
+ }
+ }
+
+ /* if failed, set it to the NULL string to indicate error */
+ if (r->useragent_host == NULL) {
+ r->useragent_host = "";
+ }
+ }
+
+ if (type == REMOTE_DOUBLE_REV) {
+ r->double_reverse = do_double_reverse(r->double_reverse,
+ r->useragent_host,
+ r->useragent_addr, r->pool);
+ if (r->double_reverse == -1) {
+ return NULL;
+ }
+ }
+
+ /*
+ * Return the desired information; either the remote DNS name, if found,
+ * or either NULL (if the hostname was requested) or the IP address
+ * (if any identifier was requested).
+ */
+ if (r->useragent_host != NULL && r->useragent_host[0] != '\0') {
+ return r->useragent_host;
+ }
+ else {
+ if (type == REMOTE_HOST || type == REMOTE_DOUBLE_REV) {
+ return NULL;
+ }
+ else {
+ *str_is_ip = 1;
+ return r->useragent_ip;
+ }
+ }
+}
/*
* Optional function coming from mod_ident, used for looking up ident user
Re: svn commit: r1729929 - in /httpd/httpd/trunk: CHANGES
include/ap_mmn.h include/http_core.h include/httpd.h server/core.c
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Feb 12, 2016 04:45, "Yann Ylavic" <yl...@gmail.com> wrote:
>
> On Fri, Feb 12, 2016 at 10:49 AM, William A Rowe Jr <wr...@rowe-clan.net>
wrote:
> > On Feb 12, 2016 2:24 AM, "Yann Ylavic" <yl...@gmail.com> wrote:
> >>
> >> I see this TODO about bitfield, but never seen it done in your
> >> commits, so I wonder where Rainer's warning comes from...
> >> Anyway, once backported (as you propose), we can hardly change it.
Well we simply can't reorder bitfields or add them in 2.4 anyways...
> > Precisely, it will NOT change in 2.4 and should not change, and is only
> > applicable to trunk. I should have omitted from the initial commit.
> >
> > If we define this as int :2 and add a second int :2, none of the c
standards
> > promise that the initial 2-bit value maintains its previous alignment.
The
> > alignment is undefined.
>
> Indeed, and as such we can't make any promise about bits
> alignment/order wrt to the underlying int, and the user can't expect
> any either.
Therefore this is problematic for 2.4, and I added a comment to STATUS that
we drop that note if backporting the patch. The note applies only to trunk.
> So I don't thing we should worry about compatibility of bitfields wrt
> added bits, the goal is to be able to add a new field without
> increasing the size of the struct (for something that requires only a
> few bits).
And that is the noble goal heading toward 2.next (3.0?) but it seems there
is no promise that adding one int foo:2 value following a previously
released int bar:2 member would preserve the bit alignment of bar across
all compilers? That suggests possibly breaking our ABI contract on a
maintenance release. So adding discrete bit fields seem very iffy, once we
tag a specific release branch. (I can see safe exceptions if adding a
group of small ints together in a maintenance release, but you would want
to bookend that with a member of known alignment, an int or * or such.)
> Aside note: with an int:2 bitfield, the possible values are 0 (00), 1
> (01), -2 (10) and -1 (11), that may be surprising, although that fits
> your description of double_reverse.
You are only looking at 2's compliment architectures, httpd isn't Intel
specific :) But we are promised 1..-1, while the fourth value is -2, or
-0, or 2, or NaN. Not something to rely on, but we don't need a fourth
value here anyways.
> IMHO signed bitfields should be avoided, and I don't know if compilers
> are kind enough (nor allowed) to pack successive signed/unsigned
> bitfields, probably yes but otherwise we may lose extensibility of
> "int double_reverse:2" though...
> So we may prefer "unsigned int double_reverse:2" (and change the
> description) so that we can later add a single bit that equals 1 and
> not -1.
We've had good success, they get rearranged and optimized only when it is
convenient for us to break ABI.
Unsigned bitfields are sometimes more compact and get us one more value (we
cannot reliably use a -2 value in a 2 bit int as I mentioned above) but
this particular use doesn't seem problematic to optimize later (in
trunk)... Let's leave it as a full int to avoid future ABI alignment issues
for any 2.4 backport?
Re: svn commit: r1729929 - in /httpd/httpd/trunk: CHANGES
include/ap_mmn.h include/http_core.h include/httpd.h server/core.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Fri, Feb 12, 2016 at 10:49 AM, William A Rowe Jr <wr...@rowe-clan.net> wrote:
> On Feb 12, 2016 2:24 AM, "Yann Ylavic" <yl...@gmail.com> wrote:
>>
>> I see this TODO about bitfield, but never seen it done in your
>> commits, so I wonder where Rainer's warning comes from...
>> Anyway, once backported (as you propose), we can hardly change it.
>
> Precisely, it will NOT change in 2.4 and should not change, and is only
> applicable to trunk. I should have omitted from the initial commit.
>
> If we define this as int :2 and add a second int :2, none of the c standards
> promise that the initial 2-bit value maintains its previous alignment. The
> alignment is undefined.
Indeed, and as such we can't make any promise about bits
alignment/order wrt to the underlying int, and the user can't expect
any either.
As you noticed, it's not possible to get the address of a bitfield (it
makes no sense), and we don't union the bitfields with a true int, so
the user would have to do things like "int *bitmask = (int *)&((char
*)r)[APR_OFFSETOF(r, useragent_host) + sizeof(r->useragent_host)]" to
start playing with the bits, and IMO that deserves a breakage if we
add a new bitfield!
So I don't thing we should worry about compatibility of bitfields wrt
added bits, the goal is to be able to add a new field without
increasing the size of the struct (for something that requires only a
few bits).
Aside note: with an int:2 bitfield, the possible values are 0 (00), 1
(01), -2 (10) and -1 (11), that may be surprising, although that fits
your description of double_reverse.
IMHO signed bitfields should be avoided, and I don't know if compilers
are kind enough (nor allowed) to pack successive signed/unsigned
bitfields, probably yes but otherwise we may lose extensibility of
"int double_reverse:2" though...
So we may prefer "unsigned int double_reverse:2" (and change the
description) so that we can later add a single bit that equals 1 and
not -1.
Regards,
Yann.
Re: svn commit: r1729929 - in /httpd/httpd/trunk: CHANGES
include/ap_mmn.h include/http_core.h include/httpd.h server/core.c
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Feb 12, 2016 2:24 AM, "Yann Ylavic" <yl...@gmail.com> wrote:
>
> On Fri, Feb 12, 2016 at 2:04 AM, <wr...@apache.org> wrote:
> > Author: wrowe
> > Date: Fri Feb 12 01:04:58 2016
> > New Revision: 1729929
> >
> > + /** Originator's DNS name, if known. NULL if DNS hasn't been
checked,
> > + * "" if it has and no address was found. N.B. Only access this
though
> > + * ap_get_useragent_host() */
> > + char *useragent_host;
>
> const char * ?
> Christophe has made some changes recently to add const to r->protocol
> (trunk only, can't be backported...), I think we shoudn't introduce
> new char* in "public" structs.
While I would generally agree, the note states you must not access this
member directly, so it seems irrelevant. But such a change should be
harmless.
> > + /** have we done double-reverse DNS? -1 yes/failure, 0 not yet,
> > + * 1 yes/success
> > + * TODO: 2 bit signed bitfield when this structure is compacted
> > + */
> > + int double_reverse;
> > };
>
> I see this TODO about bitfield, but never seen it done in your
> commits, so I wonder where Rainer's warning comes from...
> Anyway, once backported (as you propose), we can hardly change it.
Precisely, it will NOT change in 2.4 and should not change, and is only
applicable to trunk. I should have omitted from the initial commit.
If we define this as int :2 and add a second int :2, none of the c
standards promise that the initial 2-bit value maintains its previous
alignment. The alignment is undefined.
When we refactor trunk structures in anticipation of 2.6/3.0, that will be
the time to permanently park short bit values adjacent to one another.
If backported, we should redact the comment.
Cheers,
Bill
Re: svn commit: r1729929 - in /httpd/httpd/trunk: CHANGES
include/ap_mmn.h include/http_core.h include/httpd.h server/core.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Fri, Feb 12, 2016 at 2:04 AM, <wr...@apache.org> wrote:
> Author: wrowe
> Date: Fri Feb 12 01:04:58 2016
> New Revision: 1729929
>
[]
> Modified: httpd/httpd/trunk/include/httpd.h
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/include/httpd.h?rev=1729929&r1=1729928&r2=1729929&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/include/httpd.h (original)
> +++ httpd/httpd/trunk/include/httpd.h Fri Feb 12 01:04:58 2016
> @@ -1056,6 +1056,16 @@ struct request_rec {
> apr_table_t *trailers_in;
> /** MIME trailer environment from the response */
> apr_table_t *trailers_out;
> +
> + /** Originator's DNS name, if known. NULL if DNS hasn't been checked,
> + * "" if it has and no address was found. N.B. Only access this though
> + * ap_get_useragent_host() */
> + char *useragent_host;
const char * ?
Christophe has made some changes recently to add const to r->protocol
(trunk only, can't be backported...), I think we shoudn't introduce
new char* in "public" structs.
> + /** have we done double-reverse DNS? -1 yes/failure, 0 not yet,
> + * 1 yes/success
> + * TODO: 2 bit signed bitfield when this structure is compacted
> + */
> + int double_reverse;
> };
I see this TODO about bitfield, but never seen it done in your
commits, so I wonder where Rainer's warning comes from...
Anyway, once backported (as you propose), we can hardly change it.
Regards,
Yann.