Date: Wed, 1 Mar 2000 15:32:15 +1100 From: "John Saunders" <john@nlc.net.au> To: "Bill Fenner" <fenner@research.att.com> Cc: <freebsd-gnats-submit@freebsd.org>, "FreeBSD stable" <freebsd-stable@FreeBSD.ORG> Subject: Re: bin/10344 Message-ID: <01a201bf8337$245603c0$4ab511cb@scitec.com.au> References: <200002162354.PAA00567@windsor.research.att.com>
next in thread | previous in thread | raw e-mail | index | archive | help
This is a multi-part message in MIME format.
------=_NextPart_000_019F_01BF8393.52335430
Content-Type: text/plain;
charset="iso-8859-1"
Content-Transfer-Encoding: 7bit
> I disagree -- bind 8.2.2p5's res_nsend() returns resplen, which is the
> non-adjusted length of the reply that was read from the server.
I've done a complete analysis on the problem, what we have is a simple
case of running off the end of the buffer.
The function gethostanswer is passed a querybuf which has a size of
1024 bytes (determined by the #define MAXPACKET), a DNS answer length
(which in the test case is 7460), and 2 other paramters which don't
concern the problem at hand.
The function then constructs a pointer 'cp' to the starts of the 1024
byte querybuf. It also constructs a pointer 'eom' to indicate the end
of the buffer. Then inside a while statement it extracts each answer.
The while loop terminates if either all answers have been extracted,
or when 'cp' exceeds 'eom', or if an error occured.
The problem is in the construction of the 'eom' pointer, currently
the code uses the DNS answer length to calculate this. However in
the test case the answer is greater than the buffer size. This
allows the while loop to step the 'cp' pointer past the end of the
buffer. The code is...
eom = answer->buf + anslen;
The corrected code is as follows. It correctly calculates the 'eom'
position for the case of the DNS answer length being longer than
the buffer the answer is provided in.
eom = answer->buf + (anslen > MAXPACKET ? MAXPACKET : anslen);
Other interesting info. If I redefine MAXPACKET to be 2048 I am able
to get a few more results. If I change it to 4096 then a call to
BOUNDS_CHECK fails and the function returns a NULL. The included
patch alters the way BOUNDS_CHECK works so that the truncated
packet can be handled nicely. I was able to increase some defines
to get more answers back after this change.
I am not sure of the affect on other parts of the system if suddenly
more answers come back. So I would suggest that MAXADDRS and
MAXALIASES remain at 35 and MAXPACKET at 1024.
The patch also removes your "return (NULL)" that you committed, however
I thought that leaving the dprintf in would be a good warning.
Cheers.
--- gethostbydns.c.orig Sat Feb 26 11:55:47 2000
+++ gethostbydns.c Wed Mar 1 15:03:57 2000
@@ -79,8 +79,8 @@
#define SPRINTF(x) ((size_t)sprintf x)
-#define MAXALIASES 35
-#define MAXADDRS 35
+#define MAXALIASES 300
+#define MAXADDRS 300
static const char AskedForGot[] =
"gethostby*.gethostanswer: asked for \"%s\", got \"%s\"";
@@ -99,7 +99,7 @@
#if PACKETSZ > 1024
#define MAXPACKET PACKETSZ
#else
-#define MAXPACKET 1024
+#define MAXPACKET 8192
#endif
typedef union {
@@ -142,12 +142,7 @@
} while (0)
#define BOUNDS_CHECK(ptr, count) \
- do { \
- if ((ptr) + (count) > eom) { \
- h_errno = NO_RECOVERY; \
- return (NULL); \
- } \
- } while (0)
+ ((ptr) + (count) > eom)
static struct hostent *
gethostanswer(answer, anslen, qname, qtype)
@@ -170,7 +165,7 @@
tname = qname;
host.h_name = NULL;
- eom = answer->buf + anslen;
+ eom = answer->buf + (anslen > MAXPACKET ? MAXPACKET : anslen);
switch (qtype) {
case T_A:
case T_AAAA:
@@ -235,7 +230,11 @@
continue;
}
cp += n; /* name */
- BOUNDS_CHECK(cp, 3 * INT16SZ + INT32SZ);
+ if (BOUNDS_CHECK(cp, 3 * INT16SZ + INT32SZ))
+ {
+ had_error++;
+ continue;
+ }
type = _getshort(cp);
cp += INT16SZ; /* type */
class = _getshort(cp);
@@ -245,7 +244,11 @@
cp += INT32SZ; /* TTL */
n = _getshort(cp);
cp += INT16SZ; /* len */
- BOUNDS_CHECK(cp, n);
+ if (BOUNDS_CHECK(cp, n))
+ {
+ had_error++;
+ continue;
+ }
erdata = cp + n;
if (class != C_IN) {
/* XXX - debug? syslog? */
@@ -666,7 +669,6 @@
}
if (n > sizeof buf.buf) {
dprintf("static buffer is too small (%d)\n", n);
- return (NULL);
}
if (!(hp = gethostanswer(&buf, n, qbuf, T_PTR)))
return (NULL); /* h_errno was set by gethostanswer() */
-- +------------------------------------------------------------+
. | John Saunders - mailto:john@nlc.net.au (EMail) |
,--_|\ | - http://www.nlc.net.au/ (WWW) |
/ Oz \ | - 02-9489-4932 or 041-822-3814 (Phone) |
\_,--\_/ | NORTHLINK COMMUNICATIONS P/L - Supplying a professional, |
v | and above all friendly, internet connection service. |
+------------------------------------------------------------+
------=_NextPart_000_019F_01BF8393.52335430
Content-Type: application/octet-stream;
name="gethostbydns.diff"
Content-Transfer-Encoding: quoted-printable
Content-Disposition: attachment;
filename="gethostbydns.diff"
--- gethostbydns.c.orig Sat Feb 26 11:55:47 2000=0A=
+++ gethostbydns.c Wed Mar 1 15:03:57 2000=0A=
@@ -79,8 +79,8 @@=0A=
=0A=
#define SPRINTF(x) ((size_t)sprintf x)=0A=
=0A=
-#define MAXALIASES 35=0A=
-#define MAXADDRS 35=0A=
+#define MAXALIASES 300=0A=
+#define MAXADDRS 300=0A=
=0A=
static const char AskedForGot[] =3D=0A=
"gethostby*.gethostanswer: asked for \"%s\", got \"%s\"";=0A=
@@ -99,7 +99,7 @@=0A=
#if PACKETSZ > 1024=0A=
#define MAXPACKET PACKETSZ=0A=
#else=0A=
-#define MAXPACKET 1024=0A=
+#define MAXPACKET 8192=0A=
#endif=0A=
=0A=
typedef union {=0A=
@@ -142,12 +142,7 @@=0A=
} while (0)=0A=
=0A=
#define BOUNDS_CHECK(ptr, count) \=0A=
- do { \=0A=
- if ((ptr) + (count) > eom) { \=0A=
- h_errno =3D NO_RECOVERY; \=0A=
- return (NULL); \=0A=
- } \=0A=
- } while (0)=0A=
+ ((ptr) + (count) > eom)=0A=
=0A=
static struct hostent *=0A=
gethostanswer(answer, anslen, qname, qtype)=0A=
@@ -170,7 +165,7 @@=0A=
=0A=
tname =3D qname;=0A=
host.h_name =3D NULL;=0A=
- eom =3D answer->buf + anslen;=0A=
+ eom =3D answer->buf + (anslen > MAXPACKET ? MAXPACKET : anslen);=0A=
switch (qtype) {=0A=
case T_A:=0A=
case T_AAAA:=0A=
@@ -235,7 +230,11 @@=0A=
continue;=0A=
}=0A=
cp +=3D n; /* name */=0A=
- BOUNDS_CHECK(cp, 3 * INT16SZ + INT32SZ);=0A=
+ if (BOUNDS_CHECK(cp, 3 * INT16SZ + INT32SZ))=0A=
+ {=0A=
+ had_error++;=0A=
+ continue;=0A=
+ }=0A=
type =3D _getshort(cp);=0A=
cp +=3D INT16SZ; /* type */=0A=
class =3D _getshort(cp);=0A=
@@ -245,7 +244,11 @@=0A=
cp +=3D INT32SZ; /* TTL */=0A=
n =3D _getshort(cp);=0A=
cp +=3D INT16SZ; /* len */=0A=
- BOUNDS_CHECK(cp, n);=0A=
+ if (BOUNDS_CHECK(cp, n))=0A=
+ {=0A=
+ had_error++;=0A=
+ continue;=0A=
+ }=0A=
erdata =3D cp + n;=0A=
if (class !=3D C_IN) {=0A=
/* XXX - debug? syslog? */=0A=
@@ -666,7 +669,6 @@=0A=
}=0A=
if (n > sizeof buf.buf) {=0A=
dprintf("static buffer is too small (%d)\n", n);=0A=
- return (NULL);=0A=
}=0A=
if (!(hp =3D gethostanswer(&buf, n, qbuf, T_PTR)))=0A=
return (NULL); /* h_errno was set by gethostanswer() */=0A=
------=_NextPart_000_019F_01BF8393.52335430--
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-stable" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?01a201bf8337$245603c0$4ab511cb>