From 07fb02c0d50dcdab2f4162c2819e503c3c5671c3 Mon Sep 17 00:00:00 2001 From: Marcel Moolenaar Date: Wed, 18 Dec 2013 17:03:43 +0000 Subject: [PATCH] Fix an inappropriate free of a non-dynamic value. While here, make the code more naive and robust: 1. When setting ev_value, also always set ev_flags appropriately 2. Always check ev_value and ev_flags before calling free. Both the value and the EV_DYNAMIC property can come directly from the consumers of the environment functionality, so it's good to be careful. And since this code is typically not looked at for long periods of time, it's good to have it be a little "dumb-looking". Trigger case for the bug: env_setenv("foo", 0, "1", NULL, NULL); env_setenv("foo", 0, "2", NULL, NULL); Obtained from: Juniper Networks, Inc. --- lib/libstand/environment.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/lib/libstand/environment.c b/lib/libstand/environment.c index fde4cf9c387..291e3300443 100644 --- a/lib/libstand/environment.c +++ b/lib/libstand/environment.c @@ -75,7 +75,14 @@ env_setenv(const char *name, int flags, const void *value, * for one already. */ if ((ev->ev_sethook != NULL) && !(flags & EV_NOHOOK)) - return(ev->ev_sethook(ev, flags, value)); + return (ev->ev_sethook(ev, flags, value)); + + /* If there is data in the variable, discard it. */ + if (ev->ev_value != NULL && (ev->ev_flags & EV_DYNAMIC) != 0) + free(ev->ev_value); + ev->ev_value = NULL; + ev->ev_flags &= ~EV_DYNAMIC; + } else { /* @@ -84,6 +91,7 @@ env_setenv(const char *name, int flags, const void *value, ev = malloc(sizeof(struct env_var)); ev->ev_name = strdup(name); ev->ev_value = NULL; + ev->ev_flags = 0; /* hooks can only be set when the variable is instantiated */ ev->ev_sethook = sethook; ev->ev_unsethook = unsethook; @@ -117,21 +125,16 @@ env_setenv(const char *name, int flags, const void *value, } } } - - /* If there is data in the variable, discard it */ - if (ev->ev_value != NULL) - free(ev->ev_value); /* If we have a new value, use it */ if (flags & EV_VOLATILE) { ev->ev_value = strdup(value); + ev->ev_flags |= EV_DYNAMIC; } else { ev->ev_value = (char *)value; + ev->ev_flags |= flags & EV_DYNAMIC; } - /* Keep the flag components that are relevant */ - ev->ev_flags = flags & (EV_DYNAMIC); - return(0); } @@ -201,7 +204,7 @@ env_discard(struct env_var *ev) if (environ == ev) environ = ev->ev_next; free(ev->ev_name); - if (ev->ev_flags & EV_DYNAMIC) + if (ev->ev_value != NULL && (ev->ev_flags & EV_DYNAMIC) != 0) free(ev->ev_value); free(ev); }