Re: Squid-2.5.STABLE5 still not ready

[Henrik Nordstrom]

On Thu, 12 Feb 2004, Duane Wessels wrote:

> This change looks suspicious to me:
> 
> @@ -199,11 +199,15 @@
>  static void
>  ipcacheAddEntry(ipcache_entry * i)
>  {
> -hash_link *e = hash_lookup(ip_table, i->hash.key);
> +ipcache_entry *e = (ipcache_entry *) hash_lookup(ip_table, i->hash.key);
>  if (NULL != e) {
> - /* avoid colission */
> - ipcache_entry *q = (ipcache_entry *) e;
> - ipcacheRelease(q);
> + /* avoid collision */
> + if (i->flags.negcached && !e->flags.negcached && e->expires > squid_curtime) {
> + /* Don't waste good information */
> + ipcacheFreeEntry(i);
> + return;
> + }
> + ipcacheRelease(e);
>  }
>  hash_join(ip_table, &i->hash);
>  dlinkAdd(i, &i->lru, &lru_list);
> 
> Previously we were freeing e (aka q), but now we are freeing i, then inserting
> it into ip_table?

Previously we was replacing e by i. Now we are not in certain conditions 
and then intend discarding i instead. Note the return.

But yes, this area is the problem. The new entry (i) is referenced after
return from ipcacheAddEntry() and can not be discarded like this.

Many thanks! Sometimes it needs more than one pair of eyes to see obvious 
things.

Regards
Henrik

Re: Squid-2.5.STABLE5 still not ready

[Henrik Nordstrom]

On Thu, 12 Feb 2004, Duane Wessels wrote:

> src/ipcache.c had a minor change (+7 -3 lines) on Nov 28
> and a very big change ( +89 -81 lines) on Dec 6.

Right.. Don't know what I was on when looking into what changes had been 
done.

Will look very close at these changes again. The changes are clearly of 
such nature that the problems indicated could occur if not done correctly.

Regards
Henrik

Re: Squid-2.5.STABLE5 still not ready

[Henrik Nordstrom]

On Thu, 12 Feb 2004, Duane Wessels wrote:

> This change looks suspicious to me:
> 
> @@ -199,11 +199,15 @@
>  static void
>  ipcacheAddEntry(ipcache_entry * i)
>  {
> -hash_link *e = hash_lookup(ip_table, i->hash.key);
> +ipcache_entry *e = (ipcache_entry *) hash_lookup(ip_table, i->hash.key);
>  if (NULL != e) {
> - /* avoid colission */
> - ipcache_entry *q = (ipcache_entry *) e;
> - ipcacheRelease(q);
> + /* avoid collision */
> + if (i->flags.negcached && !e->flags.negcached && e->expires > squid_curtime) {
> + /* Don't waste good information */
> + ipcacheFreeEntry(i);
> + return;
> + }
> + ipcacheRelease(e);
>  }
>  hash_join(ip_table, &i->hash);
>  dlinkAdd(i, &i->lru, &lru_list);

Have now backed out this bad change. Hopefully this is the source of the
problem and there is no other stupid errors.

Thanks for the help.

I just got a report about another likely regression error however, this
time related to NTLM challenge reuses, so STABLE5 is still not ready but 
we are getting very very close to a release candidate.

Regards
Henrik

Re: Squid-2.5.STABLE5 still not ready

[Duane Wessels]

This change looks suspicious to me:

@@ -199,11 +199,15 @@
 static void
 ipcacheAddEntry(ipcache_entry * i)
 {
-hash_link *e = hash_lookup(ip_table, i->hash.key);
+ipcache_entry *e = (ipcache_entry *) hash_lookup(ip_table, i->hash.key);
 if (NULL != e) {
-   /* avoid colission */
-   ipcache_entry *q = (ipcache_entry *) e;
-   ipcacheRelease(q);
+   /* avoid collision */
+   if (i->flags.negcached && !e->flags.negcached && e->expires > squid_curtime) {
+   /* Don't waste good information */
+   ipcacheFreeEntry(i);
+   return;
+   }
+   ipcacheRelease(e);
 }
 hash_join(ip_table, &i->hash);
 dlinkAdd(i, &i->lru, &lru_list);

Previously we were freeing e (aka q), but now we are freeing i, then inserting
it into ip_table?

DW

Re: Squid-2.5.STABLE5 still not ready

[Duane Wessels]

> To make things even more strange this problem apparently is introduced
> some time in November, but there has not been any ipcache or dlink related
> changes from what I can see..

Really?

src/ipcache.c had a minor change (+7 -3 lines) on Nov 28
and a very big change ( +89 -81 lines) on Dec 6.

Duane W.

Squid-2.5.STABLE5 still not ready

[Henrik Nordstrom]

The Squid-2.5.STABLE5 release is unfortunately still not ready. Being
blocked by Bug #891 

This bug report covers two symptoms, both indicating that somehow an 
ipcache entry gets freed while it is still active.

A) Segfault due to NULL pointer referencing an entry which is cleared 
(freed)

B) 100% CPU loop due to a cyclic ipcache LRU list. My guess is that this
is probably caused by the ipcache entry previously freed is now reused and
inserted into the list before triggering problem 'A' above.

I have been staring at the ipcache code but can for my life not figure out 
what may go wrong causing such symptoms. Can I please get a few other eyes 
looking into this problem?

To make things even more strange this problem apparently is introduced 
some time in November, but there has not been any ipcache or dlink related 
changes from what I can see..

Regards
Henrik