httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "William A. Rowe, Jr." <wr...@rowe-clan.net>
Subject Re: util_ldap [Bug 29217] - Remove references to calloc() and free()
Date Fri, 11 Jun 2004 16:33:35 GMT
The proper logic to add to a cache is

wrlock

  test if exists again

  add element

unlock

because there is a race condition in the logic below

rdlock

  test if the element exists

>> race is here, prior to wrlocking, another thread may wrlock->insert

  promote to wrlock
  
    insert

unlock

At 06:07 PM 6/10/2004, Brad Nicholes wrote:
>   It appears to me that if it doesn't handle low memory situations or
>it is giving false positives, those are separate issues from pools vs.
>calloc/free.  I still think we need to implement some better monitoring
>or logging code in the cache_mgr and enhance the cache-status pages so
>that we can track things like false positives.  Maybe tracking the
>entries by user name and authentication state rather than just the
>number entries and how often the cache was hit.
>
>   Maybe the real problem is with the locking.  In fact just taking a
>quick scan through the code again, I am seeing something that bothers me
>in util_ldap_cache_comparedn()
>
>        if (curl) {
>            /* compare successful - add to the compare cache */
>            LDAP_CACHE_RDLOCK();
>            newnode.reqdn = (char *)reqdn;
>            newnode.dn = (char *)dn;
>            util_ald_cache_insert(curl->dn_compare_cache, &newnode);
>            LDAP_CACHE_UNLOCK();
>        }
>
>It appears to be acquiring a read lock but then inserts a new node into
>the cache.  Shouldn't it be acquiring a write lock before doing an
>insert?
>
>
>Brad
>
>
>Brad Nicholes
>Senior Software Engineer
>Novell, Inc., the leading provider of Net business solutions
>http://www.novell.com 
>
>>>> Graham Leggett <minfrin@sharp.fm> Thursday, June 10, 2004 4:24:54
>PM >>>
>Brad Nicholes wrote:
>
>> Do we even know that there is a problem with this code?  I haven't
>seen
>> any memory leaks so far.  I would hate to go to all of the work to
>> redesign and rewrite the ldap_cache manager for little to no gain.
>
>It does not seem to handle the "we ran out of memory while trying to
>add 
>to the cache" case very gracefully. It doesn't crash any more, but I'm
>
>experiencing false negatives still, which is the problem I was trying
>to 
>fix when I started trying to fix the code.
>
>Regards,
>Graham
>--



Mime
View raw message