You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@apr.apache.org by wr...@apache.org on 2007/12/07 19:47:29 UTC
svn commit: r602176 - /apr/apr/trunk/network_io/unix/sockaddr.c
Author: wrowe
Date: Fri Dec 7 10:47:29 2007
New Revision: 602176
URL: http://svn.apache.org/viewvc?rev=602176&view=rev
Log:
Where hostname is provided in ipv4 numeric form, as we've
foolishly cooerced all of our IPV4_MAPPED_IPV6 addresses,
we'll need to accept this as a socket lookup call!
Unfortunately, we failed to resolve, for example, 127.0.0.1
for INET6 addressing, where we would resolve ::ffff:127.0.0.1
But the AI_V4MAPPED flag will let us resolve this address after
attempting to resolve the IPV6 notation.
Feedback and careful review is desired before this is applied
to branches 1.2 and 0.9. Thanks.
Modified:
apr/apr/trunk/network_io/unix/sockaddr.c
Modified: apr/apr/trunk/network_io/unix/sockaddr.c
URL: http://svn.apache.org/viewvc/apr/apr/trunk/network_io/unix/sockaddr.c?rev=602176&r1=602175&r2=602176&view=diff
==============================================================================
--- apr/apr/trunk/network_io/unix/sockaddr.c (original)
+++ apr/apr/trunk/network_io/unix/sockaddr.c Fri Dec 7 10:47:29 2007
@@ -344,6 +344,11 @@
servname = apr_itoa(p, port);
#endif /* OSF1 */
}
+#if APR_HAVE_IPV6 && defined(AI_V4MAPPED)
+ else if (family == APR_INET6) {
+ hints.ai_flags |= AI_V4MAPPED;
+ }
+#endif
error = getaddrinfo(hostname, servname, &hints, &ai_list);
#ifdef HAVE_GAI_ADDRCONFIG
if (error == EAI_BADFLAGS && family == APR_UNSPEC) {
Re: svn commit: r602176 - /apr/apr/trunk/network_io/unix/sockaddr.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
William A. Rowe, Jr. wrote:
> Joe Orton wrote:
>>
>> The previous behaviour makes far more sense, and it would not be
>> unreasonable for applications to rely on it. In fact it looks like
>> the APR_IPV6_ADDR_OK flag is exactly a case which relies on that
>> behaviour, and is now presumably broken.
>
> Interesting. So, if we modify this to honor only the case of
> APR_IPV4_ADDR_OK plus the APR_INET6 family, it would satisfy you?
Patch committed, please examine and see if this covers your concern?
> Of course at this point, it's not possible to do so with a trivial
> patch, since apr_sockaddr_ip_get never sees that flag.
Ignore me as usual, quoting the wrong side of the equation.
Re: svn commit: r602176 - /apr/apr/trunk/network_io/unix/sockaddr.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Joe Orton wrote:
> On Tue, Dec 11, 2007 at 02:35:36PM -0600, William Rowe wrote:
>> If you want this resolved, we agree this needs to be reverted in apr 2.0?
>>
>> http://svn.apache.org/viewvc/apr/apr/trunk/network_io/unix/sa_common.c?view=log&pathrev=63986#rev60946
>>
>> talk about ancient history, but most of the time we butt heads there's
>> a root in a bad design decision. If this decision was right, then
>> apr should have some reciprocal behavior to get from point A to B and
>> back again. Apparently internal consistency doesn't interest you?
>> I consider it essential, even if the user has to throw a flag to get
>> out of the mess we created.
>
> I'd guess the original API was designed simply to produce some
> "human-readable" representation of the address, i.e. for logging
> purposes; it's not an unreasonable interface to provide per se. Perhaps
> changing it in 2.0 to add a flags argument to make the ::ffff:-stripping
> optional would make sense.
>
> (and also maybe exposing inet_pton and inet_ntop interfaces using
> apr_sockaddr_t)
Well I'm back to this, we've had requests to address this in 1.3.0, and now
I'm trying to do something dirt simple stupid...
apr_sockaddr_t *client
-> char* text
-> domain pipe to a root process
-> validate against legit patterns
-> create low-port origin sockaddr_t from text
-> domain pipe back to the requestor
Grrr.
Bill
Re: svn commit: r602176 - /apr/apr/trunk/network_io/unix/sockaddr.c
Posted by Joe Orton <jo...@redhat.com>.
On Tue, Dec 11, 2007 at 02:35:36PM -0600, William Rowe wrote:
> If you want this resolved, we agree this needs to be reverted in apr 2.0?
>
> http://svn.apache.org/viewvc/apr/apr/trunk/network_io/unix/sa_common.c?view=log&pathrev=63986#rev60946
>
> talk about ancient history, but most of the time we butt heads there's
> a root in a bad design decision. If this decision was right, then
> apr should have some reciprocal behavior to get from point A to B and
> back again. Apparently internal consistency doesn't interest you?
> I consider it essential, even if the user has to throw a flag to get
> out of the mess we created.
I'd guess the original API was designed simply to produce some
"human-readable" representation of the address, i.e. for logging
purposes; it's not an unreasonable interface to provide per se. Perhaps
changing it in 2.0 to add a flags argument to make the ::ffff:-stripping
optional would make sense.
(and also maybe exposing inet_pton and inet_ntop interfaces using
apr_sockaddr_t)
joe
Re: svn commit: r602176 - /apr/apr/trunk/network_io/unix/sockaddr.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Joe Orton wrote:
> On Tue, Dec 11, 2007 at 12:59:11PM -0600, William Rowe wrote:
>> Joe Orton wrote:
>>> No. But given that new interfaces are needed can we just give up on
>>> hacking the resolver and fix the actual issue, i.e. allow duplication of
>>> the sockaddr object?
>> We disagree that this isn't an issue in and of itself, but I'll tweak to
>> the 'new flag' as you suggest (and it could affect a broader range of cases
>> than just family==APR_INET6).
>
> Blargh. What problems will be solved by introducing these changes?
> None. Is it actually a good idea for APR to encourage propagation of
> v4-mapped IPv6 addresses? No. How many checkins will it take to
> introduce this feature correctly? As yet unknown. How much has this
> new code actually been tested? I'll guess "not at all"...
Contrawise, the initial patch was tested, as I believed I needed it for
mod_ftp, and it resolved the flaw. The revised patch was "minimally"
tested, because immediately after authoring it, I jumped on solving mod_ftp
from the sa-copy method.
> network_io/unix/sockaddr.c: In function 'find_addresses':
> network_io/unix/sockaddr.c:424: warning: unused variable 'error'
Trivial to fix. </shrug> That's not the real origin of your objection.
It worked as we simply called call_resolver twice with the same args.
Stupid but not lethal.
> How much more time should we waste introducing changes which add
> complexity, bring potential regressions, and solve non-problems? I vote
> none and revert sockaddr.c back to before r602176.
If you want this resolved, we agree this needs to be reverted in apr 2.0?
http://svn.apache.org/viewvc/apr/apr/trunk/network_io/unix/sa_common.c?view=log&pathrev=63986#rev60946
talk about ancient history, but most of the time we butt heads there's
a root in a bad design decision. If this decision was right, then
apr should have some reciprocal behavior to get from point A to B and
back again. Apparently internal consistency doesn't interest you?
I consider it essential, even if the user has to throw a flag to get
out of the mess we created.
Presuming this is a vote to not provide this feature and not a technical
veto, let's let others chime in a while. Happy to fix that flaw. As Jeff
authored the ::ffff: unmapping, I'm especially interested in his input.
>>> Something like this?
>> We don't disagree this is also a useful feature (which I hacked into ftp
>> just last night). I don't care for _clone, though. _dup makes more sense
>> to me, but since it will take an existing *sa, I'm ok with _copy too.
>> See the end of the message for all apr examples of those three forms.
>
> Fair enough. I've a vague preference for _copy over _dup since the
> former is not an abbreviation, so I'll go with that unless anybody else
> pipes up.
+1 - fyi that call to vars_set is overkill, only addr->ipaddr_ptr needs
to be adjusted once the entire sa has been pmemdup'ed (out of all of
the adjustments within vars_set).
Bill
Re: svn commit: r602176 - /apr/apr/trunk/network_io/unix/sockaddr.c
Posted by Joe Orton <jo...@redhat.com>.
On Tue, Dec 11, 2007 at 12:59:11PM -0600, William Rowe wrote:
> Joe Orton wrote:
>> No. But given that new interfaces are needed can we just give up on
>> hacking the resolver and fix the actual issue, i.e. allow duplication of
>> the sockaddr object?
>
> We disagree that this isn't an issue in and of itself, but I'll tweak to
> the 'new flag' as you suggest (and it could affect a broader range of cases
> than just family==APR_INET6).
Blargh. What problems will be solved by introducing these changes?
None. Is it actually a good idea for APR to encourage propagation of
v4-mapped IPv6 addresses? No. How many checkins will it take to
introduce this feature correctly? As yet unknown. How much has this
new code actually been tested? I'll guess "not at all"...
network_io/unix/sockaddr.c: In function 'find_addresses':
network_io/unix/sockaddr.c:424: warning: unused variable 'error'
How much more time should we waste introducing changes which add
complexity, bring potential regressions, and solve non-problems? I vote
none and revert sockaddr.c back to before r602176.
>> Something like this?
>
> We don't disagree this is also a useful feature (which I hacked into ftp
> just last night). I don't care for _clone, though. _dup makes more sense
> to me, but since it will take an existing *sa, I'm ok with _copy too.
> See the end of the message for all apr examples of those three forms.
Fair enough. I've a vague preference for _copy over _dup since the
former is not an abbreviation, so I'll go with that unless anybody else
pipes up.
joe
Re: svn commit: r602176 - /apr/apr/trunk/network_io/unix/sockaddr.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Joe Orton wrote:
> On Tue, Dec 11, 2007 at 05:21:03AM -0600, William Rowe wrote:
>> Joe Orton wrote:
>>> I'd rather just see a new flag added alongside the existing
>>> APR_IPV4_ADDR_* flags to make it explicit that this is a new and very
>>> different interface guarantee, APR_IPV6_ADDR_V4MAPPED maybe.
>> Perhaps, but the flag existed and had no definition, so this really
>> strikes me as a rational solution. My 2c.
>
> It's pretty horrid IMO: the existing APR_IPV?_ADDR_OK flags specify
> different *ordering* for the searches. This overloads a different
> combination of input parameters to have an unrelated side-effect -
> forcing use of v4-mapped IPv6 addresses.
That's a fair argument, and I can accept this. APR_IPV6_V4MAPPED_OK
works for me.
>> Either the existing flag or a new flag, it doesn't seem like this can
>> be resolved before 1.3.0. Does anyone see a way to make the reciprocal
>> sa-from-ip-and-inet6-family actually work before then?
>
> No. But given that new interfaces are needed can we just give up on
> hacking the resolver and fix the actual issue, i.e. allow duplication of
> the sockaddr object?
We disagree that this isn't an issue in and of itself, but I'll tweak to
the 'new flag' as you suggest (and it could affect a broader range of cases
than just family==APR_INET6).
> Something like this?
We don't disagree this is also a useful feature (which I hacked into ftp
just last night). I don't care for _clone, though. _dup makes more sense
to me, but since it will take an existing *sa, I'm ok with _copy too.
See the end of the message for all apr examples of those three forms.
> Index: network_io/unix/sockaddr.c
> ===================================================================
> --- network_io/unix/sockaddr.c (revision 603185)
> +++ network_io/unix/sockaddr.c (working copy)
> @@ -588,6 +588,27 @@
> return find_addresses(sa, hostname, family, port, flags, p);
> }
>
> +APR_DECLARE(apr_sockaddr_t *) apr_sockaddr_clone(const apr_sockaddr_t *old,
> + apr_port_t port,
> + apr_pool_t *p)
> +{
> + apr_sockaddr_t *sa = apr_pmemdup(p, old, sizeof *sa);
> +
> + sa->pool = p;
> + sa->next = NULL;
> +
> + if (sa->hostname) {
> + sa->hostname = apr_pstrdup(p, sa->hostname);
> + }
> + if (sa->servname) {
> + sa->servname = apr_pstrdup(p, sa->servname);
> + }
> +
> + apr_sockaddr_vars_set(sa, old->family, port);
> +
> + return sa;
> +}
> +
> APR_DECLARE(apr_status_t) apr_getnameinfo(char **hostname,
> apr_sockaddr_t *sockaddr,
> apr_int32_t flags)
> Index: include/apr_network_io.h
> ===================================================================
> --- include/apr_network_io.h (revision 603185)
> +++ include/apr_network_io.h (working copy)
> @@ -696,6 +696,18 @@
> APR_DECLARE(int) apr_sockaddr_equal(const apr_sockaddr_t *addr1,
> const apr_sockaddr_t *addr2);
>
> +/** Create a duplicate of a socket address structure.
> + * @param old Old socket address structure
> + * @param port Port to use in returned structure
> + * @return Duplicate socket address structure which varies only
> + * by the port specified.
> + * @note Available only in APR version 1.3.0 onwards.
> + */
> +APR_DECLARE(apr_sockaddr_t *) apr_sockaddr_clone(const apr_sockaddr_t *old,
> + apr_port_t port,
> + apr_pool_t *p);
> +
> +
> /**
> * Return the type of the socket.
> * @param sock The socket to query.
>
>
$ grep dup * | grep DECLARE
apr_file_io.h:APR_DECLARE(apr_status_t) apr_file_dup(apr_file_t **new_file,
apr_file_io.h:APR_DECLARE(apr_status_t) apr_file_dup2(apr_file_t *new_file,
apr_mmap.h:APR_DECLARE(apr_status_t) apr_mmap_dup(apr_mmap_t **new_mmap,
apr_strings.h:APR_DECLARE(char *) apr_pstrdup(apr_pool_t *p, const char *s);
apr_strings.h:APR_DECLARE(char *) apr_pstrmemdup(apr_pool_t *p, const char *s, apr_size_t n);
apr_strings.h:APR_DECLARE(char *) apr_pstrndup(apr_pool_t *p, const char *s, apr_size_t n);
apr_strings.h:APR_DECLARE(void *) apr_pmemdup(apr_pool_t *p, const void *m, apr_size_t n);
$ grep copy * | grep DECLARE
apr_file_io.h:APR_DECLARE(apr_status_t) apr_file_copy(const char *from_path,
apr_hash.h:APR_DECLARE(apr_hash_t *) apr_hash_copy(apr_pool_t *pool,
apr_tables.h:APR_DECLARE(apr_array_header_t *) apr_array_copy(apr_pool_t *p,
apr_tables.h:APR_DECLARE(apr_array_header_t *) apr_array_copy_hdr(apr_pool_t *p,
apr_tables.h:APR_DECLARE(apr_table_t *) apr_table_copy(apr_pool_t *p,
$ grep clone * | grep DECLARE
apr_tables.h:APR_DECLARE(apr_table_t *) apr_table_clone(apr_pool_t *p,
Re: svn commit: r602176 - /apr/apr/trunk/network_io/unix/sockaddr.c
Posted by Joe Orton <jo...@redhat.com>.
On Tue, Dec 11, 2007 at 05:21:03AM -0600, William Rowe wrote:
> Joe Orton wrote:
>> I'd rather just see a new flag added alongside the existing
>> APR_IPV4_ADDR_* flags to make it explicit that this is a new and very
>> different interface guarantee, APR_IPV6_ADDR_V4MAPPED maybe.
>
> Perhaps, but the flag existed and had no definition, so this really
> strikes me as a rational solution. My 2c.
It's pretty horrid IMO: the existing APR_IPV?_ADDR_OK flags specify
different *ordering* for the searches. This overloads a different
combination of input parameters to have an unrelated side-effect -
forcing use of v4-mapped IPv6 addresses.
> Either the existing flag or a new flag, it doesn't seem like this can
> be resolved before 1.3.0. Does anyone see a way to make the reciprocal
> sa-from-ip-and-inet6-family actually work before then?
No. But given that new interfaces are needed can we just give up on
hacking the resolver and fix the actual issue, i.e. allow duplication of
the sockaddr object? Something like this?
Index: network_io/unix/sockaddr.c
===================================================================
--- network_io/unix/sockaddr.c (revision 603185)
+++ network_io/unix/sockaddr.c (working copy)
@@ -588,6 +588,27 @@
return find_addresses(sa, hostname, family, port, flags, p);
}
+APR_DECLARE(apr_sockaddr_t *) apr_sockaddr_clone(const apr_sockaddr_t *old,
+ apr_port_t port,
+ apr_pool_t *p)
+{
+ apr_sockaddr_t *sa = apr_pmemdup(p, old, sizeof *sa);
+
+ sa->pool = p;
+ sa->next = NULL;
+
+ if (sa->hostname) {
+ sa->hostname = apr_pstrdup(p, sa->hostname);
+ }
+ if (sa->servname) {
+ sa->servname = apr_pstrdup(p, sa->servname);
+ }
+
+ apr_sockaddr_vars_set(sa, old->family, port);
+
+ return sa;
+}
+
APR_DECLARE(apr_status_t) apr_getnameinfo(char **hostname,
apr_sockaddr_t *sockaddr,
apr_int32_t flags)
Index: include/apr_network_io.h
===================================================================
--- include/apr_network_io.h (revision 603185)
+++ include/apr_network_io.h (working copy)
@@ -696,6 +696,18 @@
APR_DECLARE(int) apr_sockaddr_equal(const apr_sockaddr_t *addr1,
const apr_sockaddr_t *addr2);
+/** Create a duplicate of a socket address structure.
+ * @param old Old socket address structure
+ * @param port Port to use in returned structure
+ * @return Duplicate socket address structure which varies only
+ * by the port specified.
+ * @note Available only in APR version 1.3.0 onwards.
+ */
+APR_DECLARE(apr_sockaddr_t *) apr_sockaddr_clone(const apr_sockaddr_t *old,
+ apr_port_t port,
+ apr_pool_t *p);
+
+
/**
* Return the type of the socket.
* @param sock The socket to query.
Re: svn commit: r602176 - /apr/apr/trunk/network_io/unix/sockaddr.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Joe Orton wrote:
> On Mon, Dec 10, 2007 at 04:32:08PM -0600, William Rowe wrote:
>> Joe Orton wrote:
>>> The previous behaviour makes far more sense, and it would not be
>>> unreasonable for applications to rely on it. In fact it looks like the
>>> APR_IPV6_ADDR_OK flag is exactly a case which relies on that behaviour,
>>> and is now presumably broken.
>> Interesting. So, if we modify this to honor only the case of
>> APR_IPV4_ADDR_OK plus the APR_INET6 family, it would satisfy you?
>
> Not really, it violates the existing interface constraints:
>
> "APR_IPV4_ADDR_OK ... only valid if family is APR_UNSPEC"
Meaning - it is not defined (invalid) in 1.2.x. I'll agree then, this
can't change in 1.2.x, and backporting becomes out of the question.
The remaining question is, when we say something is "only valid when..."
and therefore nobody has coded to this ever, granting an 'invalid'
combination with a new meaning seems legitimate. No?
> I'd rather just see a new flag added alongside the existing
> APR_IPV4_ADDR_* flags to make it explicit that this is a new and very
> different interface guarantee, APR_IPV6_ADDR_V4MAPPED maybe.
Perhaps, but the flag existed and had no definition, so this really
strikes me as a rational solution. My 2c.
Either the existing flag or a new flag, it doesn't seem like this can
be resolved before 1.3.0. Does anyone see a way to make the reciprocal
sa-from-ip-and-inet6-family actually work before then?
I'm out of clues.
Bill
Re: svn commit: r602176 - /apr/apr/trunk/network_io/unix/sockaddr.c
Posted by Joe Orton <jo...@redhat.com>.
On Mon, Dec 10, 2007 at 04:32:08PM -0600, William Rowe wrote:
> Joe Orton wrote:
>>
>> The previous behaviour makes far more sense, and it would not be
>> unreasonable for applications to rely on it. In fact it looks like the
>> APR_IPV6_ADDR_OK flag is exactly a case which relies on that behaviour,
>> and is now presumably broken.
>
> Interesting. So, if we modify this to honor only the case of
> APR_IPV4_ADDR_OK plus the APR_INET6 family, it would satisfy you?
Not really, it violates the existing interface constraints:
"APR_IPV4_ADDR_OK ... only valid if family is APR_UNSPEC"
I'd rather just see a new flag added alongside the existing
APR_IPV4_ADDR_* flags to make it explicit that this is a new and very
different interface guarantee, APR_IPV6_ADDR_V4MAPPED maybe.
joe
Re: svn commit: r602176 - /apr/apr/trunk/network_io/unix/sockaddr.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Joe Orton wrote:
>
> The previous behaviour makes far more sense, and it would not be
> unreasonable for applications to rely on it. In fact it looks like the
> APR_IPV6_ADDR_OK flag is exactly a case which relies on that behaviour,
> and is now presumably broken.
Interesting. So, if we modify this to honor only the case of
APR_IPV4_ADDR_OK plus the APR_INET6 family, it would satisfy you?
Of course at this point, it's not possible to do so with a trivial
patch, since apr_sockaddr_ip_get never sees that flag.
Bill
Re: svn commit: r602176 - /apr/apr/trunk/network_io/unix/sockaddr.c
Posted by Joe Orton <jo...@redhat.com>.
On Mon, Dec 10, 2007 at 03:20:18PM -0600, William Rowe wrote:
> Joe Orton wrote:
>> Either way, trying to work around that with an APR resolver hack seems
>> completely wrong, -1, it will propagate v4-mapped IPv6 addresses when none
>> are necessary, that may very well break/confuse other callers.
>
> Howso? The user explicitly passes an IPv4 address and requests an IPv6
> flavor sa.
No, you've changed the behaviour for *every* family==AF_INET6 lookup
whether that's for "www.google.com" or an IPv4 dotted quad. Previously
such lookups would fail where the hostname had no v6 address associated;
now, where there is a v4 address associated, they will always succeed
and give out the v4-mapped IPv6 address.
The previous behaviour makes far more sense, and it would not be
unreasonable for applications to rely on it. In fact it looks like the
APR_IPV6_ADDR_OK flag is exactly a case which relies on that behaviour,
and is now presumably broken.
joe
Re: svn commit: r602176 - /apr/apr/trunk/network_io/unix/sockaddr.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Joe Orton wrote:
>
> Thank you very much Colm for decoding Bill for me.
Ack, agreed.
> This is for FTP EPRT/EPSV support I'd guess. I can imagine how it gets
> broken by the ::ffff:-stripping in apr_sockaddr_ip_getbuf since you have
> to send the serialized (family, address) on the wire for one of those
> newfangled FTP commands.
And consider the case of plenty of UDP based operations. Most of them
require something similar.
> So yes Bill, you'd need to add a variant of apr_sockaddr_ip_getbuf to
> make this work and I also agree the ::ffff:-stripping should never
> really have been done there in the first place (but now must retained
> for compatibility, I suppose).
Thanks for that acknowledgment, so we all agree APR is broken in this
respect?
> Either way, trying to work around that with an APR resolver hack seems
> completely wrong, -1, it will propagate v4-mapped IPv6 addresses when
> none are necessary, that may very well break/confuse other callers.
Howso? The user explicitly passes an IPv4 address and requests an IPv6
flavor sa. The documentation of AI_V4MAPPED is very clear, this IPv4
acceptance will only occur if the IPv6 resolver can't first handle the
address (which is good behavior IMHO).
It's truly not a "hack" in that the getaddrinfo authors predicted it's
need, that we have treated apr's IPv4 mapped IPv6 addresses in IPv4 format,
so this "hack" simply brings full circle the host of "hacks" that you
acknowledged above were bogus.
If APR drops it's lookup behavior, I agree this new "hack" is not valid.
But that's a topic for APR 2.0, no? In the meantime, how to resolve this
twisted creation for the authors of both TCP and most especially UDP
protocol applications?
Bill
Re: svn commit: r602176 - /apr/apr/trunk/network_io/unix/sockaddr.c
Posted by Joe Orton <jo...@redhat.com>.
On Mon, Dec 10, 2007 at 07:55:47PM +0000, Colm MacCarthaigh wrote:
> On Mon, Dec 10, 2007 at 12:57:18PM -0600, William A. Rowe, Jr. wrote:
> > Now apr has provided a tuple of ip address, family and port that
> > cannot be reconstituted by APR. So, for example, where we have
> > to create a connection to the very same host/family on a different
> > port, it becomes impossible.
>
> O.k., so the scenario is ;
>
> 1. Host application listens on :: , accepts incoming socket
> from ::ffff:127.0.0.1
>
> 2. Host application resolves the IP into a text format.
> (calls getnameinfo or whatever)
>
> 3. Host application takes that text format and turns
> it back into a sockaddr. (calls getaddrinfo or whatever)
>
> 4. Connects to that IP.
>
> Why would it ever do steps 2 and 3?
Thank you very much Colm for decoding Bill for me.
This is for FTP EPRT/EPSV support I'd guess. I can imagine how it gets
broken by the ::ffff:-stripping in apr_sockaddr_ip_getbuf since you have
to send the serialized (family, address) on the wire for one of those
newfangled FTP commands.
So yes Bill, you'd need to add a variant of apr_sockaddr_ip_getbuf to
make this work and I also agree the ::ffff:-stripping should never
really have been done there in the first place (but now must retained
for compatibility, I suppose).
Either way, trying to work around that with an APR resolver hack seems
completely wrong, -1, it will propagate v4-mapped IPv6 addresses when
none are necessary, that may very well break/confuse other callers.
joe
Re: svn commit: r602176 - /apr/apr/trunk/network_io/unix/sockaddr.c
Posted by Colm MacCarthaigh <co...@stdlib.net>.
On Mon, Dec 10, 2007 at 02:39:19PM -0600, William A. Rowe, Jr. wrote:
> Jump into the ftp_cmd_eprt function...
yeah, this is as awkward case that does go through a text-lookup :( The
patch makes sense for that case, but it still feels a bit strange
putting it in APR.
> Now; are you suggesting that it would be more efficient and effective
> if we cloned the local_addr structure, modified the port and directly
> used the clone to apr_socket_bind rather than going through these extra
> apr_sockaddr_info_get gymnastics?
>
> If so, what is the proper method to clone an sa in the apr schema?
Hmm, we should probably look modifying apr_sockaddr_vars_set or adding a
call for resetting the port to zero reliably.
--
Colm MacCárthaigh Public Key: colm+pgp@stdlib.net
Re: svn commit: r602176 - /apr/apr/trunk/network_io/unix/sockaddr.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Colm MacCarthaigh wrote:
> On Mon, Dec 10, 2007 at 12:57:18PM -0600, William A. Rowe, Jr. wrote:
>> Now apr has provided a tuple of ip address, family and port that
>> cannot be reconstituted by APR. So, for example, where we have
>> to create a connection to the very same host/family on a different
>> port, it becomes impossible.
>
> O.k., so the scenario is ;
>
> 1. Host application listens on :: , accepts incoming socket
> from ::ffff:127.0.0.1
>
> 2. Host application resolves the IP into a text format.
> (calls getnameinfo or whatever)
>
> 3. Host application takes that text format and turns
> it back into a sockaddr. (calls getaddrinfo or whatever)
>
> 4. Connects to that IP.
>
> Why would it ever do steps 2 and 3?
>
> Maybe we should support using INET6 sockets for outbound IPv4
> connections more - and the patch would enable that for connections
> involving lookups - but the use-case scanario is mighty strange, it has
> to go through a text lookup.
>
>> They know the family and IP from an existing connection, and that
>> existing connection is internally inconsistent.
>
> How? we don't modify the sockaddr.
See this file;
http://svn.apache.org/repos/asf/httpd/mod_ftp/trunk/modules/ftp/ftp_commands.c
Jump into the ftp_cmd_eprt function...
/* XXX: Anything special to handle IPv6 ip_addr string where c->remote_ip
* is IPv4 mapped?
*/
if (((c->local_addr->family == APR_INET) && (family == APR_INET))
#if APR_HAVE_IPV6
|| ((c->local_addr->family == APR_INET6) && (family == APR_INET6))
#endif
) {
apr_sockaddr_info_get(&sa, c->local_ip, family,
local_port, 0, c->pool);
if (!sa) {
ap_log_rerror(APLOG_MARK, APLOG_WARNING, rv, r,
"Couldn't resolve explicit local socket address"
" (apr or socket stack bug?) Retrying");
apr_sockaddr_info_get(&sa, NULL, family,
local_port, 0, c->pool);
}
}
else {
/* If the family differs, it's difficult to map the EPRT origin IP */
apr_sockaddr_info_get(&sa, NULL, family,
local_port, 0, c->pool);
}
if (!sa) {
ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r,
"Couldn't resolve local socket address"
" (apr or socket stack bug?) Giving up");
return FTP_REPLY_CANNOT_OPEN_DATACONN;
}
apr_socket_opt_set(s, APR_SO_REUSEADDR, 1);
rv = apr_socket_bind(s, sa);
To decipher; FTP by design should originate the connection to the client's
specified adapter from the *same adapter/address* to which the client had
connected.
In httpd, we know this from the c->local_addr.
Here, we take the c->local_ip and c->family, but we now need to give this
a different outgoing port assignment. We reconstruct a sockaddr_t that
consists of the IP, Family, and our new idea for a origin port.
This code in apr-1.2 falls through to the (!sa) case because 127.0.0.1
and family APR_INET6 returns NULL for the sa. then (as Joe hinted) we
can always retry with UNSPEC. But that means we do route through a
different interface, potentially.
This code with apr-1.x succeeds to bind to 127.0.0.1 as an IPV6 mapped
IPV6 address, so the interface is preserved.
Now; are you suggesting that it would be more efficient and effective
if we cloned the local_addr structure, modified the port and directly
used the clone to apr_socket_bind rather than going through these extra
apr_sockaddr_info_get gymnastics?
If so, what is the proper method to clone an sa in the apr schema?
Bill
Re: svn commit: r602176 - /apr/apr/trunk/network_io/unix/sockaddr.c
Posted by Colm MacCarthaigh <co...@stdlib.net>.
On Mon, Dec 10, 2007 at 12:57:18PM -0600, William A. Rowe, Jr. wrote:
> Now apr has provided a tuple of ip address, family and port that
> cannot be reconstituted by APR. So, for example, where we have
> to create a connection to the very same host/family on a different
> port, it becomes impossible.
O.k., so the scenario is ;
1. Host application listens on :: , accepts incoming socket
from ::ffff:127.0.0.1
2. Host application resolves the IP into a text format.
(calls getnameinfo or whatever)
3. Host application takes that text format and turns
it back into a sockaddr. (calls getaddrinfo or whatever)
4. Connects to that IP.
Why would it ever do steps 2 and 3?
Maybe we should support using INET6 sockets for outbound IPv4
connections more - and the patch would enable that for connections
involving lookups - but the use-case scanario is mighty strange, it has
to go through a text lookup.
> They know the family and IP from an existing connection, and that
> existing connection is internally inconsistent.
How? we don't modify the sockaddr.
--
Colm MacCárthaigh Public Key: colm+pgp@stdlib.net
Re: svn commit: r602176 - /apr/apr/trunk/network_io/unix/sockaddr.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Joe Orton wrote:
>
> I am getting pretty sick of trying to decode the meaning behind oblique
> remarks in commit messages. Is that supposed to help explain the
> motivation for this change? Who is "we"? What addresses have been
> coerced by whom? What is a "socket lookup call"?
Great; dialog, thank you :) Precisely why this isn't backported.
Let me clarify, please?
For example, httpd takes a connection on IPv6 over ::ffff:127.0.0.1
(an IPv4 mapped address, the socket and sockaddr structures created
by APR) and records the result of apr_sockaddr_ip_get;
/* This is an IPv4-mapped IPv6 address; drop the leading
* part of the address string so we're left with the familiar
* IPv4 format.
*/
memmove(buf, buf + strlen("::ffff:"),
strlen(buf + strlen("::ffff:"))+1);
which causes "127.0.0.1" or any other mapped address to be recorded.
HOWEVER, we do not modify the ->family elements to state APR_INET.
That's fine, but ->family and this IP are internally inconsistant.
Now apr has provided a tuple of ip address, family and port that
cannot be reconstituted by APR. So, for example, where we have
to create a connection to the very same host/family on a different
port, it becomes impossible.
>> Unfortunately, we failed to resolve, for example, 127.0.0.1
>> for INET6 addressing, where we would resolve ::ffff:127.0.0.1
>
> If apr_sockaddr_info_get is called with family==AF_INET6 and the given
> hostname does not map to any IPv6 addresses, I would expect failure.
> Why is the caller not using family==AF_UNSPEC if they don't know the
> family which the addresses of the given hostname might map to?
They know the family and IP from an existing connection, and that
existing connection is internally inconsistent.
Do we need a flavor of apr_sockaddr_ip_get that returns a usuable
address that corresponds to the apr_sockaddr_t's ->family?
Or would you have an entirely different suggestion?
Re: svn commit: r602176 - /apr/apr/trunk/network_io/unix/sockaddr.c
Posted by Joe Orton <jo...@redhat.com>.
On Fri, Dec 07, 2007 at 06:47:29PM -0000, William Rowe wrote:
> Author: wrowe
> Date: Fri Dec 7 10:47:29 2007
> New Revision: 602176
>
> URL: http://svn.apache.org/viewvc?rev=602176&view=rev
> Log:
> Where hostname is provided in ipv4 numeric form, as we've
> foolishly cooerced all of our IPV4_MAPPED_IPV6 addresses,
> we'll need to accept this as a socket lookup call!
I am getting pretty sick of trying to decode the meaning behind oblique
remarks in commit messages. Is that supposed to help explain the
motivation for this change? Who is "we"? What addresses have been
coerced by whom? What is a "socket lookup call"?
> Unfortunately, we failed to resolve, for example, 127.0.0.1
> for INET6 addressing, where we would resolve ::ffff:127.0.0.1
If apr_sockaddr_info_get is called with family==AF_INET6 and the given
hostname does not map to any IPv6 addresses, I would expect failure.
Why is the caller not using family==AF_UNSPEC if they don't know the
family which the addresses of the given hostname might map to?
joe
Re: svn commit: r602176 - /apr/apr/trunk/network_io/unix/sockaddr.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Folks, looking for feedback.
The connection rec in httpd has our apparently-ipv4 addresses stored
as the local and remote host, but we tag these as APR_INET6 addresses.
This makes it very difficult to look things up later on.
I don't think there's a problem if we look up an unspecified or IPV4
form for that address, but in the case of calling up the very same
family we recorded, INET6, shouldn't apr_sockaddr_info_get still
work with that combination???
Otherwise, our truncation of the leading ::ffff: was a pretty foolish
thing to do :(
Let me know if anyone sees issues with the patch below, otherwise I'll
backport Monday.
Bill
wrowe@apache.org wrote:
> Author: wrowe
> Date: Fri Dec 7 10:47:29 2007
> New Revision: 602176
>
> URL: http://svn.apache.org/viewvc?rev=602176&view=rev
> Log:
> Where hostname is provided in ipv4 numeric form, as we've
> foolishly cooerced all of our IPV4_MAPPED_IPV6 addresses,
> we'll need to accept this as a socket lookup call!
>
> Unfortunately, we failed to resolve, for example, 127.0.0.1
> for INET6 addressing, where we would resolve ::ffff:127.0.0.1
> But the AI_V4MAPPED flag will let us resolve this address after
> attempting to resolve the IPV6 notation.
>
> Feedback and careful review is desired before this is applied
> to branches 1.2 and 0.9. Thanks.
>
>
>
> Modified:
> apr/apr/trunk/network_io/unix/sockaddr.c
>
> Modified: apr/apr/trunk/network_io/unix/sockaddr.c
> URL: http://svn.apache.org/viewvc/apr/apr/trunk/network_io/unix/sockaddr.c?rev=602176&r1=602175&r2=602176&view=diff
> ==============================================================================
> --- apr/apr/trunk/network_io/unix/sockaddr.c (original)
> +++ apr/apr/trunk/network_io/unix/sockaddr.c Fri Dec 7 10:47:29 2007
> @@ -344,6 +344,11 @@
> servname = apr_itoa(p, port);
> #endif /* OSF1 */
> }
> +#if APR_HAVE_IPV6 && defined(AI_V4MAPPED)
> + else if (family == APR_INET6) {
> + hints.ai_flags |= AI_V4MAPPED;
> + }
> +#endif
> error = getaddrinfo(hostname, servname, &hints, &ai_list);
> #ifdef HAVE_GAI_ADDRCONFIG
> if (error == EAI_BADFLAGS && family == APR_UNSPEC) {
>
>
>
>
Re: svn commit: r602176 - /apr/apr/trunk/network_io/unix/sockaddr.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Folks, looking for feedback.
The connection rec in httpd has our apparently-ipv4 addresses stored
as the local and remote host, but we tag these as APR_INET6 addresses.
This makes it very difficult to look things up later on.
I don't think there's a problem if we look up an unspecified or IPV4
form for that address, but in the case of calling up the very same
family we recorded, INET6, shouldn't apr_sockaddr_info_get still
work with that combination???
Otherwise, our truncation of the leading ::ffff: was a pretty foolish
thing to do :(
Let me know if anyone sees issues with the patch below, otherwise I'll
backport Monday.
Bill
wrowe@apache.org wrote:
> Author: wrowe
> Date: Fri Dec 7 10:47:29 2007
> New Revision: 602176
>
> URL: http://svn.apache.org/viewvc?rev=602176&view=rev
> Log:
> Where hostname is provided in ipv4 numeric form, as we've
> foolishly cooerced all of our IPV4_MAPPED_IPV6 addresses,
> we'll need to accept this as a socket lookup call!
>
> Unfortunately, we failed to resolve, for example, 127.0.0.1
> for INET6 addressing, where we would resolve ::ffff:127.0.0.1
> But the AI_V4MAPPED flag will let us resolve this address after
> attempting to resolve the IPV6 notation.
>
> Feedback and careful review is desired before this is applied
> to branches 1.2 and 0.9. Thanks.
>
>
>
> Modified:
> apr/apr/trunk/network_io/unix/sockaddr.c
>
> Modified: apr/apr/trunk/network_io/unix/sockaddr.c
> URL: http://svn.apache.org/viewvc/apr/apr/trunk/network_io/unix/sockaddr.c?rev=602176&r1=602175&r2=602176&view=diff
> ==============================================================================
> --- apr/apr/trunk/network_io/unix/sockaddr.c (original)
> +++ apr/apr/trunk/network_io/unix/sockaddr.c Fri Dec 7 10:47:29 2007
> @@ -344,6 +344,11 @@
> servname = apr_itoa(p, port);
> #endif /* OSF1 */
> }
> +#if APR_HAVE_IPV6 && defined(AI_V4MAPPED)
> + else if (family == APR_INET6) {
> + hints.ai_flags |= AI_V4MAPPED;
> + }
> +#endif
> error = getaddrinfo(hostname, servname, &hints, &ai_list);
> #ifdef HAVE_GAI_ADDRCONFIG
> if (error == EAI_BADFLAGS && family == APR_UNSPEC) {
>
>
>
>