apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Norman Tuttle <ntut...@opendemand.com>
Subject Re: Patches for bug found in APR sockets (another attempt at explaining the issue)
Date Tue, 09 Dec 2003 03:36:23 GMT

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, "");
>    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