Re: squid-3.HEAD IPAddress leak

Alex Rousskov wrote:
> On Fri, 2008-02-08 at 15:52 +1300, Amos Jeffries wrote:
>
>> The second;
>> 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.
>
> Do you have a reference for that? I do not want to bug you with more
> questions but I am surprised to learn that some kind of a
> sockaddr_storage wrapper cannot work well for Squid... We may have to
> fix Polygraph that is using that approach, IIRC.

I don't have any online references. Everything I have found online
indicated that the _storage should be padded properly.
But, I found in my hex-level debugging of the IPv6 code in squid that
some things consistently went badly because the cast sa_family field was
something much higher than any IANA protocol family. Its too long ago
now to recall exact test cases. May have been a system-specific given
this connect() bug.

>
>> The third;
>> addrinfo* defines a whole new type. Wrapping that old sockaddr* mess and
>> providing in a nice set fo bells and whistles for use. Most importantly
>> that flag we need to pas to the system calls.
>
> I have to say that the "nice set of bells and whistles" in a basic
> address structure used throughout a performance-sensitive program raises
> red flags, but perhaps the actual performance implications are not as
> bad.
>
>> The important bits for the squid comm code are:
>>
>> struct addrinfo
>> {
>> int ai_family;
>> int ai_socktype;
>> int ai_protocol;
>> struct sockaddr *ai_addr; // pointer to sockaddr_storage/*_in/*_in6
>> int ai_addrlen;
>> char *ai_canonname; // we never new/free this ourselves in squid.
>> struct addrinfo *ai_next; // Pointer to next in list.
>> };
>
> Can you replace IPAddress data members with the above, except not use
> any pointers and forget about ai_next and ai_canonname? I think doing so
> will eliminate temporary allocations and other things that look rather
> scary to both code quality and performance folks.

In the cases where addrinfo* is going into read-only usage yes.

>
> When you do need a list of addresses or canonname, it is OK to use
> addrinfo and convert from/to IPAddress as needed, of course. I am
> guessing such uses will not affect performance or overall code quality.
>

Amos

-- 
Please use Squid 2.6STABLE17+ or 3.0STABLE1+
There are serious security advisories out on all earlier releases.