Fri, Jan 09, 2015 at 03:32:37PM -0700, Todd C. Miller wrote:
> On Fri, 09 Jan 2015 15:45:51 -0600, Brent Cook wrote:
>
> > - If imsg_add fails, it frees buf. But, so does imsg_close. Punt for
> > now and just die if imsg_add fails (maybe this should be handled more
> > nicely?)
>
> I think it makes sense to treat imsg_add() failure the same as
> imsg_create returning NULL. Perhaps if imsg_add() fails you can
> set buf to NULL since it has been freed?
Thanks, how about this?
- Nothing seems to free the result of host_dns(), so add
host_dns_free() and call after each query.
- If imsg_add fails, it frees buf. Avoid dereferencing the freed buf
afterward in imsg_close().
Granted, imsg_add _shouldn't_ fail today because we preallocated the
whole buf in imsg_create anyway, thus it doesn't have to realloc. But I
guess that's not a guarantee for tomorrow.
---
src/usr.sbin/ntpd/config.c | 11 +++++++++++
src/usr.sbin/ntpd/ntp_dns.c | 21 ++++++++++++++-------
src/usr.sbin/ntpd/ntpd.c | 21 ++++++++++++++-------
src/usr.sbin/ntpd/ntpd.h | 1 +
4 files changed, 40 insertions(+), 14 deletions(-)
diff --git a/src/usr.sbin/ntpd/config.c b/src/usr.sbin/ntpd/config.c
index 31d3a5c..51e4ccc 100644
--- a/src/usr.sbin/ntpd/config.c
+++ b/src/usr.sbin/ntpd/config.c
@@ -109,6 +109,17 @@ host_v6(const char *s)
return (h);
}
+void
+host_dns_free(struct ntp_addr *hn)
+{
+ struct ntp_addr *h = hn, *tmp;
+ while (h) {
+ tmp = h;
+ h = h->next;
+ free(tmp);
+ }
+}
+
int
host_dns(const char *s, struct ntp_addr **hn)
{
diff --git a/src/usr.sbin/ntpd/ntp_dns.c b/src/usr.sbin/ntpd/ntp_dns.c
index f670e90..b8f3b98 100644
--- a/src/usr.sbin/ntpd/ntp_dns.c
+++ b/src/usr.sbin/ntpd/ntp_dns.c
@@ -159,13 +159,20 @@ dns_dispatch_imsg(void)
buf = imsg_create(ibuf_dns, IMSG_HOST_DNS,
imsg.hdr.peerid, 0,
cnt * sizeof(struct sockaddr_storage));
- if (buf == NULL)
- break;
- if (cnt > 0)
- for (h = hn; h != NULL; h = h->next)
- imsg_add(buf, &h->ss, sizeof(h->ss));
-
- imsg_close(ibuf_dns, buf);
+ if (cnt > 0) {
+ if (buf) {
+ for (h = hn; h != NULL; h = h->next)
+ if (imsg_add(buf, &h->ss,
+ sizeof(h->ss)) == -1) {
+ buf = NULL;
+ break;
+ }
+ if (buf)
+ imsg_close(ibuf_dns, buf);
+ }
+ host_dns_free(hn);
+ hn = NULL;
+ }
break;
default:
break;
diff --git a/src/usr.sbin/ntpd/ntpd.c b/src/usr.sbin/ntpd/ntpd.c
index edcf5b2..9943a88 100644
--- a/src/usr.sbin/ntpd/ntpd.c
+++ b/src/usr.sbin/ntpd/ntpd.c
@@ -358,13 +358,20 @@ dispatch_imsg(struct ntpd_conf *lconf)
buf = imsg_create(ibuf, IMSG_HOST_DNS,
imsg.hdr.peerid, 0,
cnt * sizeof(struct sockaddr_storage));
- if (buf == NULL)
- break;
- if (cnt > 0)
- for (h = hn; h != NULL; h = h->next)
- imsg_add(buf, &h->ss, sizeof(h->ss));
-
- imsg_close(ibuf, buf);
+ if (cnt > 0) {
+ if (buf) {
+ for (h = hn; h != NULL; h = h->next)
+ if (imsg_add(buf, &h->ss,
+ sizeof(h->ss)) == -1) {
+ buf = NULL;
+ break;
+ }
+ if (buf)
+ imsg_close(ibuf, buf);
+ }
+ host_dns_free(hn);
+ hn = NULL;
+ }
break;
default:
break;
diff --git a/src/usr.sbin/ntpd/ntpd.h b/src/usr.sbin/ntpd/ntpd.h
index 6e12bed..1040d15 100644
--- a/src/usr.sbin/ntpd/ntpd.h
+++ b/src/usr.sbin/ntpd/ntpd.h
@@ -281,6 +281,7 @@ int parse_config(const char *, struct ntpd_conf *);
/* config.c */
void host(const char *, struct ntp_addr **);
int host_dns(const char *, struct ntp_addr **);
+void host_dns_free(struct ntp_addr *);
struct ntp_peer *new_peer(void);
struct ntp_conf_sensor *new_sensor(char *);
--
1.9.1
