Re: dhclient(8): fix segfault if calloc()/strdup() return NULL
That's disgusting. If it's important to know where it failed, that's
why you call the regular funciton, check the error and say something
intelligent. Then it's easy to look at the code and know it's doing
the right thing, rather than having to wade through some macro and
gccisms. bleah.
I'd far rather see it in the code, doing it right.
On Sun, Dec 16, 2012 at 5:43 PM, Martin Pelikan
wrote:
> On Sun, Dec 16, 2012 at 01:01:49PM -0700, Theo de Raadt wrote:
>> > know exactly where the memory allocation fails since the error messages
>>
>> I hate xstrdup() and xcalloc() for exactly that reason. You don't
>> know why it failed.
>
> Actually, you can make them macros using all that underscore gcc magic,
> like so:
>
> #define XMALLOC(where, howmany) do {\
> if (((where) = malloc(howmany)) == NULL) { \
> err(1, "%s: malloc %zu (%s:%d)\n", __FUNCTION__,\
> howmany, __FILE__, __LINE__); \
> } } while (0)
>
> Your error messages are exact, .data should be kept small (only one copy
> of the string + one string per each function + line numbers in .text),
> only thing worrying me is that it doesn't really look pretty in the code.
>
> If you wanted the value of 'howmany' in your error message, you should use
> %zu as malloc(3) eats size_t, but regular constants will be of type int
> and produce warnings. Playing with __builtin_constant() will get _really_
> ugly, though :-(
>
> Or, if you want to play user-friendly, you can pass a string to that
> macro, describing what is the software actually trying to do, in English.
> --
> Martin Pelikan
Re: dhclient(8): fix segfault if calloc()/strdup() return NULL
On Sun, Dec 16, 2012 at 01:01:49PM -0700, Theo de Raadt wrote:
> > know exactly where the memory allocation fails since the error messages
>
> I hate xstrdup() and xcalloc() for exactly that reason. You don't
> know why it failed.
Actually, you can make them macros using all that underscore gcc magic,
like so:
#define XMALLOC(where, howmany) do {\
if (((where) = malloc(howmany)) == NULL) { \
err(1, "%s: malloc %zu (%s:%d)\n", __FUNCTION__,\
howmany, __FILE__, __LINE__); \
} } while (0)
Your error messages are exact, .data should be kept small (only one copy
of the string + one string per each function + line numbers in .text),
only thing worrying me is that it doesn't really look pretty in the code.
If you wanted the value of 'howmany' in your error message, you should use
%zu as malloc(3) eats size_t, but regular constants will be of type int
and produce warnings. Playing with __builtin_constant() will get _really_
ugly, though :-(
Or, if you want to play user-friendly, you can pass a string to that
macro, describing what is the software actually trying to do, in English.
--
Martin Pelikan
Re: dhclient(8): fix segfault if calloc()/strdup() return NULL
> 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. I hate xstrdup() and xcalloc() for exactly that reason. You don't know why it failed. Either write correct checks and correct failure paths, or give me a coredump so that someone can write correct checks and correct failure paths. Even if the correct paths are sometimes terminal.
Re: dhclient(8): fix segfault if calloc()/strdup() return NULL
On Thu, Dec 13, 2012 at 12:07:42PM +0100, Joerg Zinke wrote:
>
> Am 11.12.2012 um 04:12 schrieb Lawrence Teo :
>
> > 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 - 1.191
> > +++ dhclient.c 10 Dec 2012 04:09:12 -
> > @@ -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->defau
Re: dhclient(8): fix segfault if calloc()/strdup() return NULL
Am 11.12.2012 um 04:12 schrieb Lawrence Teo :
> 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.c9 Dec 2012 20:28:03 - 1.191
> +++ dhclient.c10 Dec 2012 04:09:12 -
> @@ -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)
>
dhclient(8): fix segfault if calloc()/strdup() return NULL
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?
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 - 1.191
+++ dhclient.c 10 Dec 2012 04:09:12 -
@@ -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 =
