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?
> 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
