You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@apr.apache.org by jo...@apache.org on 2010/08/09 14:51:29 UTC
svn commit: r983618 - in /apr/apr/trunk: network_io/unix/sockets.c
test/testsock.c
Author: jorton
Date: Mon Aug 9 12:51:29 2010
New Revision: 983618
URL: http://svn.apache.org/viewvc?rev=983618&view=rev
Log:
* network_io/unix/sockets.c (apr_socket_connect): Copy the remote
address by value rather than by reference. This ensures that the
sockaddr object returned by apr_socket_addr_get is allocated from
the same pool as the socket object itself, as apr_socket_accept
does; avoiding any potential lifetime mismatches.
* test/testsock.c (test_get_addr): Enhance test case to cover this.
Modified:
apr/apr/trunk/network_io/unix/sockets.c
apr/apr/trunk/test/testsock.c
Modified: apr/apr/trunk/network_io/unix/sockets.c
URL: http://svn.apache.org/viewvc/apr/apr/trunk/network_io/unix/sockets.c?rev=983618&r1=983617&r2=983618&view=diff
==============================================================================
--- apr/apr/trunk/network_io/unix/sockets.c (original)
+++ apr/apr/trunk/network_io/unix/sockets.c Mon Aug 9 12:51:29 2010
@@ -387,10 +387,13 @@ apr_status_t apr_socket_connect(apr_sock
/* A real remote address was passed in. If the unspecified
* address was used, the actual remote addr will have to be
* determined using getpeername() if required. */
- /* ### this should probably be a structure copy + fixup as per
- * _accept()'s handling of local_addr */
- sock->remote_addr = sa;
sock->remote_addr_unknown = 0;
+
+ /* Copy the address structure details in. */
+ sock->remote_addr->sa = sa->sa;
+ sock->remote_addr->salen = sa->salen;
+ /* Adjust ipaddr_ptr et al. */
+ apr_sockaddr_vars_set(sock->remote_addr, sa->family, sa->port);
}
if (sock->local_addr->port == 0) {
Modified: apr/apr/trunk/test/testsock.c
URL: http://svn.apache.org/viewvc/apr/apr/trunk/test/testsock.c?rev=983618&r1=983617&r2=983618&view=diff
==============================================================================
--- apr/apr/trunk/test/testsock.c (original)
+++ apr/apr/trunk/test/testsock.c Mon Aug 9 12:51:29 2010
@@ -334,8 +334,11 @@ static void test_get_addr(abts_case *tc,
apr_status_t rv;
apr_socket_t *ld, *sd, *cd;
apr_sockaddr_t *sa, *ca;
+ apr_pool_t *subp;
char *a, *b;
+ APR_ASSERT_SUCCESS(tc, "create subpool", apr_pool_create(&subp, p));
+
ld = setup_socket(tc);
APR_ASSERT_SUCCESS(tc,
@@ -343,7 +346,7 @@ static void test_get_addr(abts_case *tc,
apr_socket_addr_get(&sa, APR_LOCAL, ld));
rv = apr_socket_create(&cd, sa->family, SOCK_STREAM,
- APR_PROTO_TCP, p);
+ APR_PROTO_TCP, subp);
APR_ASSERT_SUCCESS(tc, "create client socket", rv);
APR_ASSERT_SUCCESS(tc, "enable non-block mode",
@@ -369,7 +372,7 @@ static void test_get_addr(abts_case *tc,
}
APR_ASSERT_SUCCESS(tc, "accept connection",
- apr_socket_accept(&sd, ld, p));
+ apr_socket_accept(&sd, ld, subp));
{
/* wait for writability */
@@ -389,18 +392,38 @@ static void test_get_addr(abts_case *tc,
APR_ASSERT_SUCCESS(tc, "get local address of server socket",
apr_socket_addr_get(&sa, APR_LOCAL, sd));
-
APR_ASSERT_SUCCESS(tc, "get remote address of client socket",
apr_socket_addr_get(&ca, APR_REMOTE, cd));
-
- a = apr_psprintf(p, "%pI", sa);
- b = apr_psprintf(p, "%pI", ca);
+ /* Test that the pool of the returned sockaddr objects exactly
+ * match the socket. */
+ ABTS_PTR_EQUAL(tc, subp, sa->pool);
+ ABTS_PTR_EQUAL(tc, subp, ca->pool);
+
+ /* Check equivalence. */
+ a = apr_psprintf(p, "%pI fam=%d", sa, sa->family);
+ b = apr_psprintf(p, "%pI fam=%d", ca, ca->family);
ABTS_STR_EQUAL(tc, a, b);
+
+ /* Check pool of returned sockaddr, as above. */
+ APR_ASSERT_SUCCESS(tc, "get local address of client socket",
+ apr_socket_addr_get(&sa, APR_LOCAL, cd));
+ APR_ASSERT_SUCCESS(tc, "get remote address of server socket",
+ apr_socket_addr_get(&ca, APR_REMOTE, sd));
+
+ /* Check equivalence. */
+ a = apr_psprintf(p, "%pI fam=%d", sa, sa->family);
+ b = apr_psprintf(p, "%pI fam=%d", ca, ca->family);
+ ABTS_STR_EQUAL(tc, a, b);
+
+ ABTS_PTR_EQUAL(tc, subp, sa->pool);
+ ABTS_PTR_EQUAL(tc, subp, ca->pool);
apr_socket_close(cd);
apr_socket_close(sd);
apr_socket_close(ld);
+
+ apr_pool_destroy(subp);
}
static void test_wait(abts_case *tc, void *data)
RE: svn commit: r983618 - in /apr/apr/trunk: network_io/unix/sockets.c test/testsock.c
Posted by "Plüm, Rüdiger, VF-Group" <ru...@vodafone.com>.
> -----Original Message-----
> From: Joe Orton
> Sent: Montag, 9. August 2010 15:14
> To: dev@httpd.apache.org
> Subject: Re: svn commit: r983618 - in /apr/apr/trunk:
> network_io/unix/sockets.c test/testsock.c
>
> This fixes a slow memory leak in mod_proxy FYI. The sockaddr
> passed to
> apr_socket_connect() is allocated out of worker->cp->pool.
> When a new
> backend connection is created, core_create_conn extracts the address
> from that socket to the conn_rec and it gets duped in that pool again.
Many thanks for the heads up Joe. I guess this is the root cause for
PR49713.
Regards
Rüdiger
Re: svn commit: r983618 - in /apr/apr/trunk:
network_io/unix/sockets.c test/testsock.c
Posted by Joe Orton <jo...@redhat.com>.
This fixes a slow memory leak in mod_proxy FYI. The sockaddr passed to
apr_socket_connect() is allocated out of worker->cp->pool. When a new
backend connection is created, core_create_conn extracts the address
from that socket to the conn_rec and it gets duped in that pool again.
On Mon, Aug 09, 2010 at 12:51:29PM -0000, jorton@apache.org wrote:
> Author: jorton
> Date: Mon Aug 9 12:51:29 2010
> New Revision: 983618
>
> URL: http://svn.apache.org/viewvc?rev=983618&view=rev
> Log:
> * network_io/unix/sockets.c (apr_socket_connect): Copy the remote
> address by value rather than by reference. This ensures that the
> sockaddr object returned by apr_socket_addr_get is allocated from
> the same pool as the socket object itself, as apr_socket_accept
> does; avoiding any potential lifetime mismatches.
>
> * test/testsock.c (test_get_addr): Enhance test case to cover this.
>
> Modified:
> apr/apr/trunk/network_io/unix/sockets.c
> apr/apr/trunk/test/testsock.c
>
> Modified: apr/apr/trunk/network_io/unix/sockets.c
> URL: http://svn.apache.org/viewvc/apr/apr/trunk/network_io/unix/sockets.c?rev=983618&r1=983617&r2=983618&view=diff
> ==============================================================================
> --- apr/apr/trunk/network_io/unix/sockets.c (original)
> +++ apr/apr/trunk/network_io/unix/sockets.c Mon Aug 9 12:51:29 2010
> @@ -387,10 +387,13 @@ apr_status_t apr_socket_connect(apr_sock
> /* A real remote address was passed in. If the unspecified
> * address was used, the actual remote addr will have to be
> * determined using getpeername() if required. */
> - /* ### this should probably be a structure copy + fixup as per
> - * _accept()'s handling of local_addr */
> - sock->remote_addr = sa;
> sock->remote_addr_unknown = 0;
> +
> + /* Copy the address structure details in. */
> + sock->remote_addr->sa = sa->sa;
> + sock->remote_addr->salen = sa->salen;
> + /* Adjust ipaddr_ptr et al. */
> + apr_sockaddr_vars_set(sock->remote_addr, sa->family, sa->port);
> }
>
> if (sock->local_addr->port == 0) {
>
> Modified: apr/apr/trunk/test/testsock.c
> URL: http://svn.apache.org/viewvc/apr/apr/trunk/test/testsock.c?rev=983618&r1=983617&r2=983618&view=diff
> ==============================================================================
> --- apr/apr/trunk/test/testsock.c (original)
> +++ apr/apr/trunk/test/testsock.c Mon Aug 9 12:51:29 2010
> @@ -334,8 +334,11 @@ static void test_get_addr(abts_case *tc,
> apr_status_t rv;
> apr_socket_t *ld, *sd, *cd;
> apr_sockaddr_t *sa, *ca;
> + apr_pool_t *subp;
> char *a, *b;
>
> + APR_ASSERT_SUCCESS(tc, "create subpool", apr_pool_create(&subp, p));
> +
> ld = setup_socket(tc);
>
> APR_ASSERT_SUCCESS(tc,
> @@ -343,7 +346,7 @@ static void test_get_addr(abts_case *tc,
> apr_socket_addr_get(&sa, APR_LOCAL, ld));
>
> rv = apr_socket_create(&cd, sa->family, SOCK_STREAM,
> - APR_PROTO_TCP, p);
> + APR_PROTO_TCP, subp);
> APR_ASSERT_SUCCESS(tc, "create client socket", rv);
>
> APR_ASSERT_SUCCESS(tc, "enable non-block mode",
> @@ -369,7 +372,7 @@ static void test_get_addr(abts_case *tc,
> }
>
> APR_ASSERT_SUCCESS(tc, "accept connection",
> - apr_socket_accept(&sd, ld, p));
> + apr_socket_accept(&sd, ld, subp));
>
> {
> /* wait for writability */
> @@ -389,18 +392,38 @@ static void test_get_addr(abts_case *tc,
>
> APR_ASSERT_SUCCESS(tc, "get local address of server socket",
> apr_socket_addr_get(&sa, APR_LOCAL, sd));
> -
> APR_ASSERT_SUCCESS(tc, "get remote address of client socket",
> apr_socket_addr_get(&ca, APR_REMOTE, cd));
> -
> - a = apr_psprintf(p, "%pI", sa);
> - b = apr_psprintf(p, "%pI", ca);
>
> + /* Test that the pool of the returned sockaddr objects exactly
> + * match the socket. */
> + ABTS_PTR_EQUAL(tc, subp, sa->pool);
> + ABTS_PTR_EQUAL(tc, subp, ca->pool);
> +
> + /* Check equivalence. */
> + a = apr_psprintf(p, "%pI fam=%d", sa, sa->family);
> + b = apr_psprintf(p, "%pI fam=%d", ca, ca->family);
> ABTS_STR_EQUAL(tc, a, b);
> +
> + /* Check pool of returned sockaddr, as above. */
> + APR_ASSERT_SUCCESS(tc, "get local address of client socket",
> + apr_socket_addr_get(&sa, APR_LOCAL, cd));
> + APR_ASSERT_SUCCESS(tc, "get remote address of server socket",
> + apr_socket_addr_get(&ca, APR_REMOTE, sd));
> +
> + /* Check equivalence. */
> + a = apr_psprintf(p, "%pI fam=%d", sa, sa->family);
> + b = apr_psprintf(p, "%pI fam=%d", ca, ca->family);
> + ABTS_STR_EQUAL(tc, a, b);
> +
> + ABTS_PTR_EQUAL(tc, subp, sa->pool);
> + ABTS_PTR_EQUAL(tc, subp, ca->pool);
>
> apr_socket_close(cd);
> apr_socket_close(sd);
> apr_socket_close(ld);
> +
> + apr_pool_destroy(subp);
> }
>
> static void test_wait(abts_case *tc, void *data)
>