On Thu, Dec 13, 2012 at 12:07:42PM +0100, Joerg Zinke wrote:
>
> Am 11.12.2012 um 04:12 schrieb Lawrence Teo <l...@openbsd.org>:
>
> > There are a number of calloc() and strdup() calls in the
> > apply_defaults() and clone_lease() functions whose return values are not
> > checked. If they happen to return NULL, dhclient will segfault.
> >
> > This diff checks their return values and does some cleanup if they
> > return NULL. The diff also ensures that dhclient will not attempt to
> > make any changes (e.g. delete addresses or flush routes) if it
> > encounters these errors, but instead will try again at the next retry
> > interval.
> >
> > Thoughts/OK?
>
> Instead of checking each and every result:
> What about just introducing xstrdup() and xcalloc() similar to
> xmalloc.c in SSH? Or is recovering from OOM case considered
> important here?
After discussing my original diff with krw@, I have committed a revised
version that errors out if those strdup() and calloc() calls fail, since
that is what the rest of the code does in almost all OOM cases.
I'm not opposed to introducing xstrdup() and xcalloc() to do the same
thing, though I think one advantage of the current method is that we
know exactly where the memory allocation fails since the error messages
are very specific, e.g. "no memory for unpriv_ibuf" vs. "xcalloc: out of
memory (allocating 512 bytes)". Those specific errors could be helpful
during troubleshooting.
But since I'm very new to dhclient's code, I would defer to krw@ and
other dhclient hackers to see which method is preferred. :)
Thanks,
Lawrence
> > Index: dhclient.c
> > ===================================================================
> > RCS file: /cvs/src/sbin/dhclient/dhclient.c,v
> > retrieving revision 1.191
> > diff -u -p -r1.191 dhclient.c
> > --- dhclient.c 9 Dec 2012 20:28:03 -0000 1.191
> > +++ dhclient.c 10 Dec 2012 04:09:12 -0000
> > @@ -677,10 +677,17 @@ bind_lease(void)
> > struct client_lease *lease;
> > char *domainname, *nameservers;
> >
> > + lease = apply_defaults(client->new);
> > + if (lease == NULL) {
> > + client->state = S_INIT;
> > + set_timeout_interval(config->retry_interval, state_init);
> > + go_daemon();
> > + return;
> > + }
> > +
> > delete_addresses(ifi->name, ifi->rdomain);
> > flush_routes_and_arp_cache(ifi->rdomain);
> >
> > - lease = apply_defaults(client->new);
> > options = lease->options;
> >
> > memset(&mask, 0, sizeof(mask));
> > @@ -1939,6 +1946,10 @@ apply_defaults(struct client_lease *leas
> > int i, j;
> >
> > newlease = clone_lease(lease);
> > + if (newlease == NULL) {
> > + warning("Unable to clone lease");
> > + return (NULL);
> > + }
> >
> > for (i = 0; i < 256; i++) {
> > for (j = 0; j < config->ignored_option_count; j++) {
> > @@ -1959,6 +1970,8 @@ apply_defaults(struct client_lease *leas
> > newlease->options[i].len = config->defaults[i].len;
> > newlease->options[i].data = calloc(1,
> > config->defaults[i].len);
> > + if (newlease->options[i].data == NULL)
> > + goto cleanup;
> > memcpy(newlease->options[i].data,
> > config->defaults[i].data, config->defaults[i].len);
> > break;
> > @@ -1970,6 +1983,8 @@ apply_defaults(struct client_lease *leas
> > lease->options[i].len;
> > newlease->options[i].data = calloc(1,
> > newlease->options[i].len);
> > + if (newlease->options[i].data == NULL)
> > + goto cleanup;
> > memcpy(newlease->options[i].data,
> > config->defaults[i].data, config->defaults[i].len);
> > memcpy(newlease->options[i].data +
> > @@ -1984,6 +1999,8 @@ apply_defaults(struct client_lease *leas
> > lease->options[i].len;
> > newlease->options[i].data = calloc(1,
> > newlease->options[i].len);
> > + if (newlease->options[i].data == NULL)
> > + goto cleanup;
> > memcpy(newlease->options[i].data,
> > lease->options[i].data, lease->options[i].len);
> > memcpy(newlease->options[i].data +
> > @@ -1998,6 +2015,8 @@ apply_defaults(struct client_lease *leas
> > config->defaults[i].len;
> > newlease->options[i].data = calloc(1,
> > config->defaults[i].len);
> > + if (newlease->options[i].data == NULL)
> > + goto cleanup;
> > memcpy(newlease->options[i].data,
> > config->defaults[i].data,
> > config->defaults[i].len);
> > @@ -2010,6 +2029,14 @@ apply_defaults(struct client_lease *leas
> > }
> >
> > return (newlease);
> > +
> > +cleanup:
> > + if (newlease)
> > + free_client_lease(newlease);
> > +
> > + warning("Unable to apply defaults");
> > +
> > + return (NULL);
> > }
> >
> > struct client_lease *
> > @@ -2019,6 +2046,8 @@ clone_lease(struct client_lease *oldleas
> > int i;
> >
> > newlease = calloc(1, sizeof(struct client_lease));
> > + if (newlease == NULL)
> > + goto cleanup;
> >
> > newlease->expiry = oldlease->expiry;
> > newlease->renewal = oldlease->renewal;
> > @@ -2026,19 +2055,33 @@ clone_lease(struct client_lease *oldleas
> > newlease->is_static = oldlease->is_static;
> > newlease->is_bootp = oldlease->is_bootp;
> >
> > - if (oldlease->server_name)
> > + if (oldlease->server_name) {
> > newlease->server_name = strdup(oldlease->server_name);
> > - if (oldlease->filename)
> > + if (newlease->server_name == NULL)
> > + goto cleanup;
> > + }
> > + if (oldlease->filename) {
> > newlease->filename = strdup(oldlease->filename);
> > + if (newlease->filename == NULL)
> > + goto cleanup;
> > + }
> >
> > for (i = 0; i < 256; i++) {
> > newlease->options[i].len = oldlease->options[i].len;
> > newlease->options[i].data = calloc(1, newlease->options[i].len);
> > + if (newlease->options[i].data == NULL)
> > + goto cleanup;
> > memcpy(newlease->options[i].data, oldlease->options[i].data,
> > newlease->options[i].len);
> > }
> >
> > return (newlease);
> > +
> > +cleanup:
> > + if (newlease)
> > + free_client_lease(newlease);
> > +
> > + return (NULL);
> > }
> >
> > void
