Re: squid-3.HEAD IPAddress leak

> On Fri, Feb 08, 2008, Amos Jeffries wrote:
>
>> sockaddr_storage (as Husni uses, and Adrian mentioned) was created to
>> provide a better way of using sockaddr* so the sockaddr_in and
>> sockaddr_in6 bits could be read-written easily. But the big/litte endian
>> problems between OS screwed up the sockaddr_in* sa_family field
>> locations
>> inside it so developers still can't portably use it for the v6/v6 flag
>> they wanted.
>
> How'd that screw things up? Hm, I'll have to read.
>
>> ... (1) pointer to somewhere holding the sockaddr_storage/*_in/*_in6
>> struct sockaddr *ai_addr;
>
> Which you have to allocate.

and do when needed.

>
>> char *ai_canonname;
>
> Which we're not using, and you have to allocate.

and don't.

>
>> ... and because some system lookups in IPv6 can return _multiple_
>> addresses
>> ... it has a linked-list of branchs for each of the alternatives
>> struct addrinfo *ai_next; /* Pointer to next in list. */
>> };
>>
>
> Which is only useful in the context of providing lists of addresses for
> a given host or IP address. What getaddrinfo() returns.

and consider read-only.

>
>> > Why do we need AddrInfo now
>>
>> To kill dozens of castings, magic operations on object sizes, and cut
>> down
>> dozens of lines of 'compatibility code'.
>
> And add in a minimum of one, and a maximum of two allocations per socket
> operation.
>
>> Together they make a pretty tree. But every used piece is eseentially
>> another new, memset, free.
>
> Ah, and here you will have problems.

Agreed.

>
> The members of that struct should probably be malloc, free, and not
> new/delete. You're using new/delete which -should- map to a default
> new operator and head off to the malloc libraries, but -squids- idea
> of the malloc interface could differ from the -library- idea of
> the malloc interface.

I was thinking squid overiding the new/delete to its xmalloc/xfree at the
same time it overrode the general malloc/free.

>
> So you should probably drop the new/delete'ing of the addrinfo stuff
> and replace it with malloc/free.

If thats better, no problem.

>
> You're also memset()'ing the addrinfo struct whether you allocated
> it or not, which may be double-memset'ing the thing, and if someone
> passed in an addrinfo it may have structure members which have now
> been leaked.

In the current usage the call should be the allocation. Not external.
Allowing external allocation via another API call would be the only
optimisation I can think of that does not break anything anywhere.

memset is needed there because I could not tell that the new/delete
_guaranteed_ pre-NULL'd memory and a single set bit in any unused fields
might cause a crash later.
With your deeper knowledge of the memory allocation in squid-3, feel free
to alter the default memset()'s.

Amos
Received on Thu Feb 07 2008 - 20:29:00 MST