Re: Patches for bug found in APR sockets

[Jeff Trawick <tr...@attglobal.net>]

Norman Tuttle wrote:

>> While trying to memory-tune code which we are working on based on 
>> Apache Flood, an apr project, I noticed some potentially memory-leak 
>> prone code. While alloc_socket() called by apr_socket_create() does an 
>> apr_palloc() to allocate memory for a socket's remote_addr member, the 
>> apr_socket_connect() function sets the remote_addr directly to the 
>> apr_sockaddr_t * which it passes. The problem is that that defeats the 
>> purpose of allocating a buffer for it, since you're replacing with a 
>> new pointer which presumably also had to allocate its space (and the 
>> typical apr_socket_addr_get() would do that). So we need to use a 
>> memcpy() instead!

not really a leak; I'd call the preallocation of those sockaddrs at socket 
creation time "unfortunate overhead" ;)  for it to be a leak, continued 
operations on a particular socket would have to result in (unnecessary) storage 
growth

I don't see any benefit in doing the memcpy instead of the pointer copy.  It 
just slows down the application (barring any CPU cache problems related to 
locality of the different sockaddrs, which I shouldn't even pretend to be able 
to point out) and doesn't save any memory.

The interesting question IMO is can we get rid of the unfortunate overhead -- 
the preallocation of sockaddrs at socket creation time.  I believe that is 
required to support the ancient ideom

    apr_socket_create(&sock);
    apr_socket_addr_get(&remote_addr, REMOTE, sock);
    apr_sockaddr_ip_set(remote_addr, "1.2.3.4");
    apr_sockaddr_port_set(remote_addr, 80);
    apr_connect(sock, NULL);

and furthermore that the ideom is inherently busted (IPv4 numeric address 
strings only?  yuck!) and furthermore apr_socket_connect(), on Unix at least, 
hasn't allowed the sockaddr to be NULL for some time, in contradiction to its 
documentation :)  Yet more cruft that I didn't come back and clean up at the 
appropriate time.

/* XXX assumes IPv4... I don't think this function is needed anyway
  * since we have apr_sockaddr_info_get(), but we need to clean up Apache's
  * listen.c a bit more first.
  */
APR_DECLARE(apr_status_t) apr_sockaddr_ip_set(apr_sockaddr_t *sockaddr,
                                          const char *addr)

I'll start a new thread on some networking apis to remove and give folks a 
chance to holler.  Once that is resolved, I suspect you'll find that the 
unfortunate overhead is now worthless, and some alternative patch can clean up 
that issue.

|PS Question: how many similar issues exist in the current state of the APR?

Is this the lead-in to the not-getting-your-money's-worth-? speech or the 
we-are-humble-programmers-in-need-of-your-help speech?  I can't keep it straight.

Re: Patches for bug found in APR sockets

[Jeff Trawick <tr...@attglobal.net>]

Jeff Trawick wrote:

> The interesting question IMO is can we get rid of the unfortunate 
> overhead -- the preallocation of sockaddrs at socket creation time.  I 
> believe that is required to support the ancient ideom
> 
>    apr_socket_create(&sock);
>    apr_socket_addr_get(&remote_addr, REMOTE, sock);
>    apr_sockaddr_ip_set(remote_addr, "1.2.3.4");
>    apr_sockaddr_port_set(remote_addr, 80);
>    apr_connect(sock, NULL);
> 
> I'll start a new thread on some networking apis to remove and give folks 
> a chance to holler.  Once that is resolved, I suspect you'll find that 
> the unfortunate overhead is now worthless, and some alternative patch 
> can clean up that issue.

sockaddr_ip_set(), sockaddr_port_set() are now gone...  now is prime time to 
see if there is any remaining need to allocate those sockaddrs, and separately 
to see if any well known APR users somehow rely on those sockaddrs being 
preallocated (i.e., they grab local socket address from socket before it would 
necessarily be set)

Re: Patches for bug found in APR sockets (another attempt at explaining the issue)

[Jeff Trawick <tr...@attglobal.net>]

Norman Tuttle wrote:

>  Jeff:
> 
> I think you misunderstood my point here. The preallocation is actually 
> desirable as it gives the sockets closure relative to pool dynamics. In 
> simplified English, that means that we can allocate a socket structure 
> (apr_socket_create), and when we deallocate it,

i.e., when we clear the pool that the socket lives in

> all that we need goes 
> away. The problem with the code I modified is that it causes a 
> dependence to occur on a structure which may not be in the same 
> pool-space. If the pointer is instead copied to the preallocated 
> structure, I can get rid of the original value in a more local pool, and 
> let the socket take care of the data it needs when it is finished with 
> it. This enables us to have cleaner interface to the sockets. Otherwise 
> (if you take out the preallocation of the needed elements), you would 
> really have to specify somewhere in the docs that the structure being 
> pointed to would require persistence for the life of the socket. This is 
> really easier to manage through the socket itself.

besides copying the structure into the socket, you need to worry about updating

     apr_pool_t *pool;       (update with the socket's pool)
     char *hostname;         (apr_pstrdup() into the socket's pool)
     char *servname;         (apr_pstrdup() into the socket's pool)
     void *ipaddr_ptr;       (update with new address within socket's sockaddr)
     apr_sockaddr_t *next;   (just clear this one)

But this is so that the apr_socket_t and apr_sockaddr_t can be allocated from 
pools with different lifetimes?  I agree it is conceptually cleaner if the 
implied pool relationship is lifted.  In practice I can see how having the 
sockaddr live longer than the socket is reasonable, and current code supports 
that (right?).  But what is the use of having a socket that lives longer than a 
sockaddr?

Re: Patches for bug found in APR sockets (another attempt at explaining the issue)

[Norman Tuttle <nt...@opendemand.com>]

  Jeff:

I think you misunderstood my point here. The preallocation is actually 
desirable as it gives the sockets closure relative to pool dynamics. In 
simplified English, that means that we can allocate a socket structure 
(apr_socket_create), and when we deallocate it, all that we need goes 
away. The problem with the code I modified is that it causes a 
dependence to occur on a structure which may not be in the same 
pool-space. If the pointer is instead copied to the preallocated 
structure, I can get rid of the original value in a more local pool, and 
let the socket take care of the data it needs when it is finished with 
it. This enables us to have cleaner interface to the sockets. Otherwise 
(if you take out the preallocation of the needed elements), you would 
really have to specify somewhere in the docs that the structure being 
pointed to would require persistence for the life of the socket. This is 
really easier to manage through the socket itself.

-Norman Tuttle ntuttle@opendemand.com

Jeff Trawick wrote:

> Norman Tuttle wrote:
>
>>> While trying to memory-tune code which we are working on based on 
>>> Apache Flood, an apr project, I noticed some potentially memory-leak 
>>> prone code. While alloc_socket() called by apr_socket_create() does 
>>> an apr_palloc() to allocate memory for a socket's remote_addr 
>>> member, the apr_socket_connect() function sets the remote_addr 
>>> directly to the apr_sockaddr_t * which it passes. The problem is 
>>> that that defeats the purpose of allocating a buffer for it, since 
>>> you're replacing with a new pointer which presumably also had to 
>>> allocate its space (and the typical apr_socket_addr_get() would do 
>>> that). So we need to use a memcpy() instead!
>>
>
> not really a leak; I'd call the preallocation of those sockaddrs at 
> socket creation time "unfortunate overhead" ;)  for it to be a leak, 
> continued operations on a particular socket would have to result in 
> (unnecessary) storage growth
>
> I don't see any benefit in doing the memcpy instead of the pointer 
> copy.  It just slows down the application (barring any CPU cache 
> problems related to locality of the different sockaddrs, which I 
> shouldn't even pretend to be able to point out) and doesn't save any 
> memory.
>
> The interesting question IMO is can we get rid of the unfortunate 
> overhead -- the preallocation of sockaddrs at socket creation time.  I 
> believe that is required to support the ancient ideom
>
>    apr_socket_create(&sock);
>    apr_socket_addr_get(&remote_addr, REMOTE, sock);
>    apr_sockaddr_ip_set(remote_addr, "1.2.3.4");
>    apr_sockaddr_port_set(remote_addr, 80);
>    apr_connect(sock, NULL);
>
> and furthermore that the ideom is inherently busted (IPv4 numeric 
> address strings only?  yuck!) and furthermore apr_socket_connect(), on 
> Unix at least, hasn't allowed the sockaddr to be NULL for some time, 
> in contradiction to its documentation :)  Yet more cruft that I didn't 
> come back and clean up at the appropriate time.
>
> /* XXX assumes IPv4... I don't think this function is needed anyway
>  * since we have apr_sockaddr_info_get(), but we need to clean up 
> Apache's
>  * listen.c a bit more first.
>  */
> APR_DECLARE(apr_status_t) apr_sockaddr_ip_set(apr_sockaddr_t *sockaddr,
>                                          const char *addr)
>
> I'll start a new thread on some networking apis to remove and give 
> folks a chance to holler.  Once that is resolved, I suspect you'll 
> find that the unfortunate overhead is now worthless, and some 
> alternative patch can clean up that issue.
>
> |PS Question: how many similar issues exist in the current state of 
> the APR?
>
> Is this the lead-in to the not-getting-your-money's-worth-? speech or 
> the we-are-humble-programmers-in-need-of-your-help speech?  I can't 
> keep it straight.
>
>
>
>