apr-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: [PATCH] apr_sockaddr_ip_get vs psprintf %pI
Date Fri, 10 Feb 2006 20:36:45 GMT
AIUI this feature %pI is present on APR 1.2.x but broken on the same.

+1 to backport since we are stuck with APR 1.2 for httpd for some time to come.

Bill

Joe Orton wrote:
> The psprintf code makes the assumption that the apr_vformatter code does 
> not allocate from the pool being used.  The %pI implementation breaks 
> this assumption because apr_sockaddr_ip_get does palloc; simple uses of 
> %pI can end up printing a partial string depending on the state of the 
> pool on entry.
> 
> This can be fixed by introducing a non-allocating variant of 
> apr_sockaddr_ip_get(); I called this apr_sockaddr_ip_getbuf here.  Any 
> objections?
> 
> Index: include/apr_network_io.h
> ===================================================================
> --- include/apr_network_io.h	(revision 278993)
> +++ include/apr_network_io.h	(working copy)
> @@ -669,6 +669,15 @@
>                                                apr_sockaddr_t *sockaddr);
>  
>  /**
> + * Write the IP address (in numeric address string format) of the APR
> + * socket address @a sockaddr into the buffer @a buf (of size @a buflen).
> + * @param addr The IP address.
> + * @param sockaddr The socket address to reference.
> + */
> +APR_DECLARE(apr_status_t) apr_sockaddr_ip_getbuf(char *buf, apr_size_t buflen,
> +                                                 apr_sockaddr_t *sockaddr);
> +
> +/**
>   * See if the IP addresses in two APR socket addresses are
>   * equivalent.  Appropriate logic is present for comparing
>   * IPv4-mapped IPv6 addresses with IPv4 addresses.
> Index: strings/apr_snprintf.c
> ===================================================================
> --- strings/apr_snprintf.c	(revision 278993)
> +++ strings/apr_snprintf.c	(working copy)
> @@ -474,7 +474,14 @@
>  
>      p = conv_10(sa->port, TRUE, &is_negative, p, &sub_len);
>      *--p = ':';
> -    apr_sockaddr_ip_get(&ipaddr_str, sa);
> +    ipaddr_str = buf_end - NUM_BUF_SIZE;
> +    if (apr_sockaddr_ip_getbuf(ipaddr_str, sa->addr_str_len, sa)) {
> +        /* Should only fail if the buffer is too small, which it
> +         * should not be; but fail safe anyway: */
> +        *--p = '?';
> +        *len = buf_end - p;
> +        return p;
> +    }
>      sub_len = strlen(ipaddr_str);
>  #if APR_HAVE_IPV6
>      if (sa->family == APR_INET6 &&
> Index: network_io/unix/sockaddr.c
> ===================================================================
> --- network_io/unix/sockaddr.c	(revision 278993)
> +++ network_io/unix/sockaddr.c	(working copy)
> @@ -98,27 +98,37 @@
>      }
>  }
>  
> -APR_DECLARE(apr_status_t) apr_sockaddr_ip_get(char **addr,
> -                                         apr_sockaddr_t *sockaddr)
> +APR_DECLARE(apr_status_t) apr_sockaddr_ip_getbuf(char *buf, apr_size_t buflen,
> +                                                 apr_sockaddr_t *sockaddr)
>  {
> -    *addr = apr_palloc(sockaddr->pool, sockaddr->addr_str_len);
> -    apr_inet_ntop(sockaddr->family,
> -                  sockaddr->ipaddr_ptr,
> -                  *addr,
> -                  sockaddr->addr_str_len);
> +    if (!apr_inet_ntop(sockaddr->family, sockaddr->ipaddr_ptr, buf, buflen)) {
> +        return APR_ENOSPC;
> +    }
> +
>  #if APR_HAVE_IPV6
> -    if (sockaddr->family == AF_INET6 &&
> -        IN6_IS_ADDR_V4MAPPED((struct in6_addr *)sockaddr->ipaddr_ptr)) {
> +    if (sockaddr->family == AF_INET6 
> +        && IN6_IS_ADDR_V4MAPPED((struct in6_addr *)sockaddr->ipaddr_ptr)
> +        && buflen > strlen("::ffff:")) {
>          /* This is an IPv4-mapped IPv6 address; drop the leading
>           * part of the address string so we're left with the familiar
>           * IPv4 format.
>           */
> -        *addr += strlen("::ffff:");
> +        memmove(buf, buf + strlen("::ffff:"),
> +                strlen(buf + strlen("::ffff:")));
>      }
>  #endif
> +    /* ensure NUL termination if the buffer is too short */
> +    buf[buflen-1] = '\0';
>      return APR_SUCCESS;
>  }
>  
> +APR_DECLARE(apr_status_t) apr_sockaddr_ip_get(char **addr,
> +                                              apr_sockaddr_t *sockaddr)
> +{
> +    *addr = apr_palloc(sockaddr->pool, sockaddr->addr_str_len);
> +    return apr_sockaddr_ip_getbuf(*addr, sockaddr->addr_str_len, sockaddr);
> +}
> +
>  void apr_sockaddr_vars_set(apr_sockaddr_t *addr, int family, apr_port_t port)
>  {
>      addr->family = family;
> Index: test/testsock.c
> ===================================================================
> --- test/testsock.c	(revision 278993)
> +++ test/testsock.c	(working copy)
> @@ -203,6 +203,19 @@
>      APR_ASSERT_SUCCESS(tc, "Problem closing socket", rv);
>  }
>  
> +static void test_print_addr(abts_case *tc, void *data)
> +{
> +    apr_sockaddr_t *sa;
> +    char *s;
> +
> +    APR_ASSERT_SUCCESS(tc, "Problem generating sockaddr",
> +                       apr_sockaddr_info_get(&sa, "0.0.0.0", APR_INET, 80, 0, p));
> +
> +    s = apr_psprintf(p, "foo %pI bar", sa);
> +
> +    ABTS_STR_EQUAL(tc, "foo 0.0.0.0:80 bar", s);
> +}
> +
>  abts_suite *testsock(abts_suite *suite)
>  {
>      suite = ADD_SUITE(suite)
> @@ -212,6 +225,7 @@
>      abts_run_test(suite, test_send, NULL);
>      abts_run_test(suite, test_recv, NULL);
>      abts_run_test(suite, test_timeout, NULL);
> +    abts_run_test(suite, test_print_addr, NULL);
>  
>      return suite;
>  }
> 
> 


Mime
View raw message