apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jeff Trawick <traw...@attglobal.net>
Subject Re: Patches for bug found in APR sockets
Date Tue, 02 Dec 2003 15:41:25 GMT
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 

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_addr_get(&remote_addr, REMOTE, sock);
    apr_sockaddr_ip_set(remote_addr, "");
    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.

View raw message