Re: New squid packages 2.4.6-2woody9 restarts very often.

[Daniel Hess <daniel@rio-grande.ping.de>]

Sorry for once more replaying to my own mail, ... :)

On Tue, Aug 23, 2005 at 05:00:12AM +0200, Daniel Hess wrote:
> Before the update (without squid-2.4.STABLE7-dns_query-4.patch)
> RR->rdlength, which gets added to off, was not passed from
> rfc1035RRUnpack to rfc1035NameUnpack. Now it gets passed and "len + 1"
> is added to it (line 326 in patched version).
> 
> When later RR->rdlength is added to off, just before the assert, off
> gets to big.

As this gave me no peace i got the latest stable upstream release
(2.5.STABLE10) and compared rfc1035RRUnpack.

They now use rdlength (and not RR->rdlength) to store the ntohs(s) value
(which gets later copied to RR->rdlength) and increment off. This
prevents the assert from getting triggered wrongfully.

An patch (which fixes the problem for me) to the behaviour of the new
release which also includes an fix to an memory leak in case a invalid
packet causes NameUnpack to go beyond the RDATA area (also in the new
release) follows.

  - Daniel

diff -Nur squid-2.4.6.bak/lib/rfc1035.c squid-2.4.6/lib/rfc1035.c
--- squid-2.4.6.bak/lib/rfc1035.c	Wed Aug 24 00:34:55 2005
+++ squid-2.4.6/lib/rfc1035.c	Wed Aug 24 00:46:49 2005
@@ -347,6 +347,7 @@
 {
     unsigned short s;
     unsigned int i;
+    unsigned short rdlength;
     off_t rdata_off;
     if (rfc1035NameUnpack(buf, sz, off, NULL, RR->name, RFC1035_MAXHOSTNAMESZ, 0)) {
 	RFC1035_UNPACK_DEBUG;
@@ -373,7 +374,8 @@
     RR->ttl = ntohl(i);
     memcpy(&s, buf + (*off), sizeof(s));
     (*off) += sizeof(s);
-    if ((*off) + ntohs(s) > sz) {
+    rdlength = ntohs(s);
+    if ((*off) + rdlength > sz) {
 	/*
 	 * We got a truncated packet.  'dnscache' truncates UDP
 	 * replies at 512 octets, as per RFC 1035.
@@ -382,31 +384,33 @@
 	memset(RR, '\0', sizeof(*RR));
 	return 1;
     }
-    RR->rdlength = ntohs(s);
+    RR->rdlength = rdlength;
     switch (RR->type) {
     case RFC1035_TYPE_PTR:
 	RR->rdata = xmalloc(RFC1035_MAXHOSTNAMESZ);
 	rdata_off = *off;
+	RR->rdlength = 0;	/* Filled in by rfc1035NameUnpack */
 	if (rfc1035NameUnpack(buf, sz, &rdata_off, &RR->rdlength, RR->rdata, RFC1035_MAXHOSTNAMESZ, 0))
 	    return 1;
-	if (rdata_off > ((*off) + RR->rdlength)) {
+	if (rdata_off > ((*off) + rdlength)) {
 	    /*
 	     * This probably doesn't happen for valid packets, but
 	     * I want to make sure that NameUnpack doesn't go beyond
 	     * the RDATA area.
 	     */
 	    RFC1035_UNPACK_DEBUG;
+	    xfree(RR->rdata);
 	    memset(RR, '\0', sizeof(*RR));
 	    return 1;
 	}
 	break;
     case RFC1035_TYPE_A:
     default:
-	RR->rdata = xmalloc(RR->rdlength);
-	memcpy(RR->rdata, buf + (*off), RR->rdlength);
+	RR->rdata = xmalloc(rdlength);
+	memcpy(RR->rdata, buf + (*off), rdlength);
 	break;
     }
-    (*off) += RR->rdlength;
+    (*off) += rdlength;
     assert((*off) <= sz);
     return 0;
 }