[squid-dev] [MERGE] Fix splay

[Kinkie]

Fix committed as r13825.
I am not convinced this is all however. For some reason duplicate
values in ACLs are not handled correctly in calling code, and this
triggers an assert in Splay->insert(). I suspect that this was not
noticed before as this check is not performed when clients call
SplayNode->insert directly.
I've added an explicit check in ACLIP, but I am convinced that there are more.

IMVHO a possible solution is to change in Splay::insert()
- assert (find (value, compare) == NULL);
+ if (find(value, compare) != NULL) return;

aka ignoring duplicates.

What do you guys think?
Please keep in mind I am conducting work to replace Splay with
std::map or std::unordered_map where it makes sense (which is almost
everywhere, IMO).



On Tue, Jan 6, 2015 at 9:21 PM, Kinkie <gkinkie at gmail.com> wrote:
> Hi,
>   I'm working on this right now. I missed constructing several Splays,
> this is artifact of the C-ish construction pattern for several
> involved classes.
> Sorry about the breakage.
>
> On Tue, Jan 6, 2015 at 9:12 PM, Tsantilas Christos
> <chtsanti at users.sourceforge.net> wrote:
>> Hi all,
>> I am getting assertions while squid.conf parsed using the latest squid
>> sources. Looks that the reason is the splay trees. Is it possible that this
>> patch causes these bugs?
>>
>> I am seeing this problem when an "acl localhost src 127.0.0.1/32" line is
>> parsed (duplicate value?) or when "proxy_auth" or snmp_community acls
>> parsed.
>>
>> Backtace for proxy_auth acl line ("acl UserChtsanti proxy_auth chtsanti") is
>> the following:
>>
>> #0  find<char*> (this=0x0, compare=0xb89f80 <Debug::Levels>,
>>     value=@0x7fff4bcf01e8: 0x1396b50 "chtsanti") at
>> ../../include/splay.h:287
>> #1  Splay<char*>::insert (this=0x0,
>>     value=@0x7fff4bcf01e8: 0x1396b50 "chtsanti",
>>     compare=0x6a6620 <splaystrcmp(char* const&, char* const&)>)
>>     at ../../include/splay.h:302
>> #2  0x00000000006a7130 in ACLUserData::parse (this=0x1395f50)
>>     at UserData.cc:120
>> #3  0x00000000006e5d8e in ACL::ParseAclLine (parser=...,
>>     head=0xcb2318 <Config+1336>) at Acl.cc:263
>> #4  0x000000000052ad18 in parse_acl (ae=<optimized out>) at cache_cf.cc:1292
>> #5  parse_line (buff=<optimized out>) at cf_parser.cci:921
>> #6  0x000000000052c2b2 in parseOneConfigFile (
>>     file_name=file_name at entry=0x1386d30 "squid-http-bypass.conf",
>>     depth=depth at entry=0) at cache_cf.cc:543
>> #7  0x000000000052cde9 in parseConfigFile (
>>     file_name=0x1386d30 "squid-http-bypass.conf") at cache_cf.cc:584
>> #8  0x00000000005ffe51 in SquidMain (argc=<optimized out>,
>> argv=0x7fff4bcf08a8)
>>     at main.cc:1397
>> #9  0x000000000050815b in SquidMainSafe (argv=<optimized out>,
>>     argc=<optimized out>) at main.cc:1251
>> #10 main (argc=<optimized out>, argv=<optimized out>) at main.cc:1244
>> (gdb) up
>> #1  Splay<char*>::insert (this=0x0,
>>     value=@0x7fff4bcf01e8: 0x1396b50 "chtsanti",
>>     compare=0x6a6620 <splaystrcmp(char* const&, char* const&)>)
>>     at ../../include/splay.h:302
>> 302         assert (!find (value, compare));
>>
>>
>>
>>
>> On 01/06/2015 05:52 AM, Amos Jeffries wrote:
>>>
>>> -----BEGIN PGP SIGNED MESSAGE-----
>>> Hash: SHA1
>>>
>>> On 3/01/2015 1:54 a.m., Kinkie wrote:
>>>>>
>>>>> I believe this is missing several "delete Foo;" in the ACL
>>>>> destructors. You used new() to allocate the Splay<> objects,
>>>>> there should be matching delete() at the same owner/abstraction
>>>>> level. For example: " ACLIP::~ACLIP() { if (data) {
>>>>> data->destroy(IPSplay::DefaultFree); data->destroy(); delete
>>>>> data; } } " ... although, it would be even better if the data
>>>>> members could be made non-pointers now they are Splay<> objects.
>>>>> Is that easy enough to do in this patch ?
>>>>
>>>>
>>>> Well, splay doesn't have a destructor :\ I'll see if I can do
>>>> something about it.
>>>>
>>>>> Other than the new() / delete() issue. +1.
>>>>
>>>>
>>>> Ok, thanks. I'll also see about the empty lines.
>>>>
>>>
>>> For the record this was merged as trunk rev.13810.
>>>
>>> Amos
>>> -----BEGIN PGP SIGNATURE-----
>>> Version: GnuPG v2.0.22 (MingW32)
>>>
>>> iQEcBAEBAgAGBQJUq1wFAAoJELJo5wb/XPRjjcIIAI1DKgOnGgX+l5HQXsc6YK2J
>>> ALTV1FryIfE0v179uSy2RshWuq3VdgDbVBHQmHjLhcEtK2wJVNHBWzML7Bqfpdn8
>>> O5ayKRqsjg5nhSsgr0NbaGhivK/JCM5vO3Q4vejFQp2Oy3geZOBvZfntFoUBp1xu
>>> o2P5ZzgFU1sQ9LoM5WoCjfWwfb7BOrKbwkEX/BzK0v9iOx9b3U6dpOtCKT11EDTg
>>> MW9x0UVjAC/TVRY9bXiBAEUHG4TPPIa5Syrx9bIqobA8u2UTLd36TQqNdfJDz3jP
>>> sVGN6B/p6W4gbEsTJnqfP7YEXmhb+gMvCYNdNvlhS7152APdi8+TNNJkSaqWGQQ=
>>> =buZA
>>> -----END PGP SIGNATURE-----
>>> _______________________________________________
>>> squid-dev mailing list
>>> squid-dev at lists.squid-cache.org
>>> http://lists.squid-cache.org/listinfo/squid-dev
>>>
>>
>> _______________________________________________
>> squid-dev mailing list
>> squid-dev at lists.squid-cache.org
>> http://lists.squid-cache.org/listinfo/squid-dev
>
>
>
> --
>     Francesco



-- 
    Francesco