On Sat, Sep 08, 2018 at 11:48:35AM +0200, Clemens Goessnitzer wrote:
>
>
> On 08/09/18 11:40, Theo Buehler wrote:
> > On Sat, Sep 08, 2018 at 11:07:30AM +0200, Anton Lindqvist wrote:
> > > On Fri, Sep 07, 2018 at 09:22:33PM +0200, Theo Buehler wrote:
> > > > On Fri, Sep 07, 2018 at 09:15:30PM +0200, Clemens Goessnitzer wrote:
> > > > > This patch adds 2 missing NULL pointer checks to rebound.c after
> > > > > malloc().
> > > >
> > > > The same function also contains an unchecked calloc.
> > >
> > > Since it's a daemon I guess it makes sense to continue execution instead
> > > of dying. However, we should make sure to not leak memory along the
> > > error paths. Also, log something when preloading the cache fails.
> > >
> > > Regress passed; ok?
> >
> > You may want to consider returning -1 instead of 1 on failure for
> > consistency with the rest of the daemon. Apart from that this looks
> > good.
> >
> > ok
> >
>
> Or maybe something with goto error?
Committed, I settled on using goto. Thanks!
>
> Clemens
> Index: rebound.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rebound/rebound.c,v
> retrieving revision 1.98
> diff -u -p -r1.98 rebound.c
> --- rebound.c 1 May 2018 15:14:43 -0000 1.98
> +++ rebound.c 8 Sep 2018 09:45:23 -0000
> @@ -630,12 +630,12 @@ encodename(const char *name, unsigned ch
> return totlen;
> }
>
> -static void
> +static int
> preloadcache(const char *name, uint16_t type, void *rdata, uint16_t rdatalen)
> {
> - struct dnspacket *req, *resp;
> + struct dnspacket *req = NULL, *resp = NULL;
> size_t reqlen, resplen;
> - struct dnscache *ent;
> + struct dnscache *ent = NULL;
> unsigned char *p;
> uint16_t len, class;
> uint32_t ttl;
> @@ -643,6 +643,8 @@ preloadcache(const char *name, uint16_t
> /* header + len + name + type + class */
> reqlen = sizeof(*req) + 1 + strlen(name) + 2 + 2;
> req = malloc(reqlen);
> + if (req == NULL)
> + goto error;
>
> req->id = 0;
> req->flags = htons(0x100);
> @@ -661,6 +663,9 @@ preloadcache(const char *name, uint16_t
> /* req + name (compressed) + type + class + ttl + len + data */
> resplen = reqlen + 2 + 2 + 2 + 4 + 2 + rdatalen;
> resp = malloc(resplen);
> + if (resp == NULL)
> + goto error;
> +
> memcpy(resp, req, reqlen);
> resp->flags = htons(0x100 | 0x8000); /* response */
> resp->ancount = htons(1);
> @@ -682,6 +687,9 @@ preloadcache(const char *name, uint16_t
> memcpy(p, rdata, rdatalen);
>
> ent = calloc(1, sizeof(*ent));
> + if (ent == NULL)
> + goto error;
> +
> ent->req = req;
> ent->reqlen = reqlen;
> ent->resp = resp;
> @@ -690,6 +698,13 @@ preloadcache(const char *name, uint16_t
>
> RB_INSERT(cachetree, &cachetree, ent);
>
> + return 0;
> +
> +error:
> + free(req);
> + free(resp);
> + free(ent);
> + return -1;
> }
>
> static void
> @@ -699,7 +714,8 @@ preloadA(const char *name, const char *i
>
> inet_aton(ip, &in);
>
> - preloadcache(name, htons(1), &in, 4);
> + if (preloadcache(name, htons(1), &in, 4) == -1)
> + logerr("failed to add cahce entry for %s", name);
> }
>
> static void
> @@ -715,7 +731,8 @@ preloadPTR(const char *ip, const char *n
>
> encodename(name, namebuf);
>
> - preloadcache(ipbuf, htons(12), namebuf, 1 + strlen(namebuf));
> + if (preloadcache(ipbuf, htons(12), namebuf, 1 + strlen(namebuf)) == -1)
> + logerr("failed to load cache entry for %s", ip);
> }
>
> static int
