Add reference counting to Defaults bindings.

Previously, we checked that the previous entry's binding pointer
was not the same while freeing.  However, to be able to merge
Defaults records we cannot rely on Defaults entries with the same
binding being immediately adjacent.  This removes the prev_binding
checks in favor of a reference count which allows us to plug the
memory leak in cvtsudoers when merging Defaults.

plugins/sudoers/alias.c

@@ -353,20 +353,20 @@ alias_find_used(struct sudoers_parse_tree *parse_tree, struct rbtree *used_alias
     TAILQ_FOREACH(d, &parse_tree->defaults, entries) {
 	switch (d->type) {
 	    case DEFAULTS_HOST:
-		errors += alias_find_used_members(parse_tree, d->binding,
+		errors += alias_find_used_members(parse_tree,
-		    HOSTALIAS, used_aliases);
+		    &d->binding->members, HOSTALIAS, used_aliases);
 		break;
 	    case DEFAULTS_USER:
-		errors += alias_find_used_members(parse_tree, d->binding,
+		errors += alias_find_used_members(parse_tree,
-		    USERALIAS, used_aliases);
+		    &d->binding->members, USERALIAS, used_aliases);
 		break;
 	    case DEFAULTS_RUNAS:
-		errors += alias_find_used_members(parse_tree, d->binding,
+		errors += alias_find_used_members(parse_tree,
-		    RUNASALIAS, used_aliases);
+		    &d->binding->members, RUNASALIAS, used_aliases);
 		break;
 	    case DEFAULTS_CMND:
-		errors += alias_find_used_members(parse_tree, d->binding,
+		errors += alias_find_used_members(parse_tree,
-		    CMNDALIAS, used_aliases);
+		    &d->binding->members, CMNDALIAS, used_aliases);
 		break;
 	    default:
 		break;

plugins/sudoers/cvtsudoers.c

@@ -1233,7 +1233,6 @@ filter_defaults(struct sudoers_parse_tree *parse_tree,
     struct member_list runas_aliases = TAILQ_HEAD_INITIALIZER(runas_aliases);
     struct member_list host_aliases = TAILQ_HEAD_INITIALIZER(host_aliases);
     struct member_list cmnd_aliases = TAILQ_HEAD_INITIALIZER(cmnd_aliases);
-    struct member_list *prev_binding = NULL;
     struct defaults *def, *def_next;
     struct member *m, *m_next;
     int alias_type;
@@ -1253,8 +1252,10 @@ filter_defaults(struct sudoers_parse_tree *parse_tree,
 	    break;
 	case DEFAULTS_USER:
 	    if (!ISSET(conf->defaults, CVT_DEFAULTS_USER) ||
-		!userlist_matches_filter(parse_tree, def->binding, conf))
+		    !userlist_matches_filter(parse_tree, &def->binding->members,
+		    conf)) {
 		keep = false;
+	    }
 	    alias_type = USERALIAS;
 	    break;
 	case DEFAULTS_RUNAS:
@@ -1264,14 +1265,18 @@ filter_defaults(struct sudoers_parse_tree *parse_tree,
 	    break;
 	case DEFAULTS_HOST:
 	    if (!ISSET(conf->defaults, CVT_DEFAULTS_HOST) ||
-		!hostlist_matches_filter(parse_tree, def->binding, conf))
+		    !hostlist_matches_filter(parse_tree, &def->binding->members,
+		    conf)) {
 		keep = false;
+	    }
 	    alias_type = HOSTALIAS;
 	    break;
 	case DEFAULTS_CMND:
 	    if (!ISSET(conf->defaults, CVT_DEFAULTS_CMND) ||
-		!cmndlist_matches_filter(parse_tree, def->binding, conf))
+		    !cmndlist_matches_filter(parse_tree, &def->binding->members,
+		    conf)) {
 		keep = false;
+	    }
 	    alias_type = CMNDALIAS;
 	    break;
 	default:
@@ -1280,12 +1285,16 @@ filter_defaults(struct sudoers_parse_tree *parse_tree,
 	}
 
 	if (!keep) {
-	    /* Look for aliases used by the binding. */
+	    /*
+	     * Look for aliases used by the binding.
+	     * Consecutive Defaults can share the same binding.
+	     */
 	    /* XXX - move to function */
-	    if (alias_type != UNSPEC && def->binding != prev_binding) {
+	    if (alias_type != UNSPEC &&
-		TAILQ_FOREACH_SAFE(m, def->binding, entries, m_next) {
+		    (def_next == NULL || def->binding != def_next->binding)) {
+		TAILQ_FOREACH_SAFE(m, &def->binding->members, entries, m_next) {
 		    if (m->type == ALIAS) {
-			TAILQ_REMOVE(def->binding, m, entries);
+			TAILQ_REMOVE(&def->binding->members, m, entries);
 			switch (alias_type) {
 			case USERALIAS:
 			    TAILQ_INSERT_TAIL(&user_aliases, m, entries);
@@ -1308,18 +1317,7 @@ filter_defaults(struct sudoers_parse_tree *parse_tree,
 		}
 	    }
 	    TAILQ_REMOVE(&parse_tree->defaults, def, entries);
-	    free_default(def, &prev_binding);
+	    free_default(def);
-	    if (prev_binding != NULL) {
-		/* Remove and free Defaults that share the same binding. */
-		while (def_next != NULL && def_next->binding == prev_binding) {
-		    def = def_next;
-		    def_next = TAILQ_NEXT(def, entries);
-		    TAILQ_REMOVE(&parse_tree->defaults, def, entries);
-		    free_default(def, &prev_binding);
-		}
-	    }
-	} else {
-	    prev_binding = def->binding;
 	}
     }
 

plugins/sudoers/cvtsudoers_csv.c

@@ -288,7 +288,7 @@ print_member_list_csv(FILE *fp, struct sudoers_parse_tree *parse_tree,
  */
 static void
 print_defaults_binding_csv(FILE *fp, struct sudoers_parse_tree *parse_tree,
-     struct member_list *binding, int type, bool expand_aliases)
+     struct defaults_binding *binding, int type, bool expand_aliases)
 {
     int alias_type;
     debug_decl(print_defaults_binding_csv, SUDOERS_DEBUG_UTIL);
@@ -296,8 +296,8 @@ print_defaults_binding_csv(FILE *fp, struct sudoers_parse_tree *parse_tree,
     if (type != DEFAULTS) {
 	/* Print each member object in binding. */
 	alias_type = defaults_to_alias_type(type);
-	print_member_list_csv(fp, parse_tree, binding, false, alias_type,
+	print_member_list_csv(fp, parse_tree, &binding->members, false,
-	    expand_aliases);
+	    alias_type, expand_aliases);
     }
 
     debug_return;

plugins/sudoers/cvtsudoers_json.c

@@ -371,18 +371,18 @@ print_alias_json(struct sudoers_parse_tree *parse_tree, struct alias *a, void *v
  */
 static void
 print_binding_json(struct json_container *jsonc,
-    struct sudoers_parse_tree *parse_tree, struct member_list *binding,
+    struct sudoers_parse_tree *parse_tree, struct defaults_binding *binding,
     int type, bool expand_aliases)
 {
     struct member *m;
     debug_decl(print_binding_json, SUDOERS_DEBUG_UTIL);
 
-    if (TAILQ_EMPTY(binding))
+    if (TAILQ_EMPTY(&binding->members))
 	debug_return;
 
     /* Print each member object in binding. */
     sudo_json_open_array(jsonc, "Binding");
-    TAILQ_FOREACH(m, binding, entries) {
+    TAILQ_FOREACH(m, &binding->members, entries) {
 	print_member_json(jsonc, parse_tree, m, defaults_to_word_type(type),
 	     expand_aliases);
     }

plugins/sudoers/cvtsudoers_merge.c

@@ -220,34 +220,34 @@ static void
 alias_rename_defaults(const char *old_name, const char *new_name,
     int alias_type, struct defaults_list *defaults)
 {
-    struct defaults *def;
+    struct defaults *def, *def_next;
-    struct member_list *prev_binding = NULL;
     struct member *m;
     debug_decl(alias_rename_defaults, SUDOERS_DEBUG_ALIAS);
 
-    TAILQ_FOREACH(def, defaults, entries) {
+    TAILQ_FOREACH_SAFE(def, defaults, entries, def_next) {
-	if (def->binding == prev_binding)
+	/* Consecutive Defaults can share the same binding. */
+	if (def_next != NULL && def->binding == def_next->binding)
 	    continue;
+
 	switch (def->type) {
 	case DEFAULTS_USER:
 	    if (alias_type != USERALIAS)
-		goto wrong_type;
+		continue;
 	    break;
 	case DEFAULTS_RUNAS:
 	    if (alias_type != RUNASALIAS)
-		goto wrong_type;
+		continue;
 	    break;
 	case DEFAULTS_HOST:
 	    if (alias_type != HOSTALIAS)
-		goto wrong_type;
+		continue;
 	    break;
 	default:
-	wrong_type:
-	    prev_binding = NULL;
 	    continue;
 	}
-	if (def->binding != NULL) {
+
-	    TAILQ_FOREACH(m, def->binding, entries) {
+	/* Rename matching aliases in the binding's member_list. */
+	TAILQ_FOREACH(m, &def->binding->members, entries) {
 	    if (m->type != ALIAS)
 		continue;
 	    if (strcmp(m->name, old_name) == 0) {
@@ -261,8 +261,6 @@ alias_rename_defaults(const char *old_name, const char *new_name,
 	    }
 	}
     }
-	prev_binding = def->binding;
-    }
 
     debug_return;
 }
@@ -493,7 +491,7 @@ defaults_var_matches(struct defaults *d1, struct defaults *d2)
     if (strcmp(d1->var, d2->var) != 0)
 	debug_return_bool(false);
     if (d1->type != DEFAULTS) {
-	if (!member_list_equivalent(d1->binding, d2->binding))
+	if (!member_list_equivalent(&d1->binding->members, &d2->binding->members))
 	    debug_return_bool(false);
     }
 
@@ -618,16 +616,18 @@ merge_defaults(struct sudoers_parse_tree_list *parse_trees,
 		}
 		m->type = WORD;
 		m->negated = false;
-		TAILQ_INIT(def->binding);
+		TAILQ_INIT(&def->binding->members);
-		TAILQ_INSERT_TAIL(def->binding, m, entries);
+		def->binding->refcnt = 1;
+		TAILQ_INSERT_TAIL(&def->binding->members, m, entries);
 		def->type = DEFAULTS_HOST;
 	    }
 
 	    /*
 	     * Only add Defaults entry if not overridden by subsequent sudoers.
-	     * XXX - leaks def if not merged but can't free due to binding.
 	     */
-	    if (!defaults_has_conflict(def, parse_tree)) {
+	    if (defaults_has_conflict(def, parse_tree)) {
+		free_default(def);
+	    } else {
 		if (def->type != DEFAULTS_HOST) {
 		    sudo_warnx(U_("%s:%d:%d: unable to make Defaults \"%s\" host-specific"), def->file, def->line, def->column, def->var);
 		}

plugins/sudoers/defaults.c

@@ -714,19 +714,19 @@ default_binding_matches(struct sudoers_parse_tree *parse_tree,
     case DEFAULTS:
 	debug_return_bool(true);
     case DEFAULTS_USER:
-	if (userlist_matches(parse_tree, sudo_user.pw, d->binding) == ALLOW)
+	if (userlist_matches(parse_tree, sudo_user.pw, &d->binding->members) == ALLOW)
 	    debug_return_bool(true);
 	break;
     case DEFAULTS_RUNAS:
-	if (runaslist_matches(parse_tree, d->binding, NULL, NULL, NULL) == ALLOW)
+	if (runaslist_matches(parse_tree, &d->binding->members, NULL, NULL, NULL) == ALLOW)
 	    debug_return_bool(true);
 	break;
     case DEFAULTS_HOST:
-	if (hostlist_matches(parse_tree, sudo_user.pw, d->binding) == ALLOW)
+	if (hostlist_matches(parse_tree, sudo_user.pw, &d->binding->members) == ALLOW)
 	    debug_return_bool(true);
 	break;
     case DEFAULTS_CMND:
-	if (cmndlist_matches(parse_tree, d->binding, NULL, NULL) == ALLOW)
+	if (cmndlist_matches(parse_tree, &d->binding->members, NULL, NULL) == ALLOW)
 	    debug_return_bool(true);
 	break;
     }

plugins/sudoers/fmtsudoers_cvt.c

@@ -194,8 +194,8 @@ sudoers_format_default_line(struct sudo_lbuf *lbuf,
 	    alias_type = UNSPEC;
 	    break;
     }
-    TAILQ_FOREACH(m, d->binding, entries) {
+    TAILQ_FOREACH(m, &d->binding->members, entries) {
-	if (m != TAILQ_FIRST(d->binding))
+	if (m != TAILQ_FIRST(&d->binding->members))
 	    sudo_lbuf_append(lbuf, ", ");
 	sudoers_format_member(lbuf, parse_tree, m, ", ", alias_type);
     }

plugins/sudoers/gram.c

@@ -3421,7 +3421,7 @@ new_default(char *var, char *val, short op)
     d->val = val;
     /* d->type = 0; */
     d->op = op;
-    /* d->binding = NULL */
+    /* d->binding = NULL; */
     d->line = this_lineno;
     d->column = sudolinebuf.toke_start + 1;
     d->file = sudo_rcstr_addref(sudoers);
@@ -3494,6 +3494,22 @@ new_digest(int digest_type, char *digest_str)
     debug_return_ptr(digest);
 }
 
+static void
+free_defaults_binding(struct defaults_binding *binding)
+{
+    debug_decl(free_defaults_binding, SUDOERS_DEBUG_PARSER);
+
+    /* Bindings may be shared among multiple Defaults entries. */
+    if (binding != NULL) {
+	if (--binding->refcnt == 0) {
+	    free_members(&binding->members);
+	    free(binding);
+	}
+    }
+
+    debug_return;
+}
+
 /*
  * Add a list of defaults structures to the defaults list.
  * The binding, if non-NULL, specifies a list of hosts, users, or
@@ -3503,7 +3519,7 @@ static bool
 add_defaults(int type, struct member *bmem, struct defaults *defs)
 {
     struct defaults *d, *next;
-    struct member_list *binding;
+    struct defaults_binding *binding;
     bool ret = true;
     debug_decl(add_defaults, SUDOERS_DEBUG_PARSER);
 
@@ -3519,10 +3535,11 @@ add_defaults(int type, struct member *bmem, struct defaults *defs)
 	}
 	if (bmem != NULL) {
 	    parser_leak_remove(LEAK_MEMBER, bmem);
-	    HLTQ_TO_TAILQ(binding, bmem, entries);
+	    HLTQ_TO_TAILQ(&binding->members, bmem, entries);
 	} else {
-	    TAILQ_INIT(binding);
+	    TAILQ_INIT(&binding->members);
 	}
+	binding->refcnt = 0;
 
 	/*
 	 * Set type and binding (who it applies to) for new entries.
@@ -3532,6 +3549,7 @@ add_defaults(int type, struct member *bmem, struct defaults *defs)
 	HLTQ_FOREACH_SAFE(d, defs, entries, next) {
 	    d->type = type;
 	    d->binding = binding;
+	    binding->refcnt++;
 	    TAILQ_INSERT_TAIL(&parsed_policy.defaults, d, entries);
 	}
     }
@@ -3612,30 +3630,23 @@ free_members(struct member_list *members)
 void
 free_defaults(struct defaults_list *defs)
 {
-    struct member_list *prev_binding = NULL;
     struct defaults *def;
     debug_decl(free_defaults, SUDOERS_DEBUG_PARSER);
 
     while ((def = TAILQ_FIRST(defs)) != NULL) {
 	TAILQ_REMOVE(defs, def, entries);
-	free_default(def, &prev_binding);
+	free_default(def);
     }
 
     debug_return;
 }
 
 void
-free_default(struct defaults *def, struct member_list **binding)
+free_default(struct defaults *def)
 {
     debug_decl(free_default, SUDOERS_DEBUG_PARSER);
 
-    if (def->binding != *binding) {
+    free_defaults_binding(def->binding);
-	*binding = def->binding;
-	if (def->binding != NULL) {
-	    free_members(def->binding);
-	    free(def->binding);
-	}
-    }
     sudo_rcstr_delref(def->file);
     free(def->var);
     free(def->val);
@@ -3775,7 +3786,6 @@ free_cmndspecs(struct cmndspec_list *csl)
 void
 free_privilege(struct privilege *priv)
 {
-    struct member_list *prev_binding = NULL;
     struct defaults *def;
     debug_decl(free_privilege, SUDOERS_DEBUG_PARSER);
 
@@ -3784,7 +3794,7 @@ free_privilege(struct privilege *priv)
     free_cmndspecs(&priv->cmndlist);
     while ((def = TAILQ_FIRST(&priv->defaults)) != NULL) {
 	TAILQ_REMOVE(&priv->defaults, def, entries);
-	free_default(def, &prev_binding);
+	free_default(def);
     }
     free(priv);
 

plugins/sudoers/gram.y

@@ -1265,7 +1265,7 @@ new_default(char *var, char *val, short op)
     d->val = val;
     /* d->type = 0; */
     d->op = op;
-    /* d->binding = NULL */
+    /* d->binding = NULL; */
     d->line = this_lineno;
     d->column = sudolinebuf.toke_start + 1;
     d->file = sudo_rcstr_addref(sudoers);
@@ -1338,6 +1338,22 @@ new_digest(int digest_type, char *digest_str)
     debug_return_ptr(digest);
 }
 
+static void
+free_defaults_binding(struct defaults_binding *binding)
+{
+    debug_decl(free_defaults_binding, SUDOERS_DEBUG_PARSER);
+
+    /* Bindings may be shared among multiple Defaults entries. */
+    if (binding != NULL) {
+	if (--binding->refcnt == 0) {
+	    free_members(&binding->members);
+	    free(binding);
+	}
+    }
+
+    debug_return;
+}
+
 /*
  * Add a list of defaults structures to the defaults list.
  * The binding, if non-NULL, specifies a list of hosts, users, or
@@ -1347,7 +1363,7 @@ static bool
 add_defaults(int type, struct member *bmem, struct defaults *defs)
 {
     struct defaults *d, *next;
-    struct member_list *binding;
+    struct defaults_binding *binding;
     bool ret = true;
     debug_decl(add_defaults, SUDOERS_DEBUG_PARSER);
 
@@ -1363,10 +1379,11 @@ add_defaults(int type, struct member *bmem, struct defaults *defs)
 	}
 	if (bmem != NULL) {
 	    parser_leak_remove(LEAK_MEMBER, bmem);
-	    HLTQ_TO_TAILQ(binding, bmem, entries);
+	    HLTQ_TO_TAILQ(&binding->members, bmem, entries);
 	} else {
-	    TAILQ_INIT(binding);
+	    TAILQ_INIT(&binding->members);
 	}
+	binding->refcnt = 0;
 
 	/*
 	 * Set type and binding (who it applies to) for new entries.
@@ -1376,6 +1393,7 @@ add_defaults(int type, struct member *bmem, struct defaults *defs)
 	HLTQ_FOREACH_SAFE(d, defs, entries, next) {
 	    d->type = type;
 	    d->binding = binding;
+	    binding->refcnt++;
 	    TAILQ_INSERT_TAIL(&parsed_policy.defaults, d, entries);
 	}
     }
@@ -1456,30 +1474,23 @@ free_members(struct member_list *members)
 void
 free_defaults(struct defaults_list *defs)
 {
-    struct member_list *prev_binding = NULL;
     struct defaults *def;
     debug_decl(free_defaults, SUDOERS_DEBUG_PARSER);
 
     while ((def = TAILQ_FIRST(defs)) != NULL) {
 	TAILQ_REMOVE(defs, def, entries);
-	free_default(def, &prev_binding);
+	free_default(def);
     }
 
     debug_return;
 }
 
 void
-free_default(struct defaults *def, struct member_list **binding)
+free_default(struct defaults *def)
 {
     debug_decl(free_default, SUDOERS_DEBUG_PARSER);
 
-    if (def->binding != *binding) {
+    free_defaults_binding(def->binding);
-	*binding = def->binding;
-	if (def->binding != NULL) {
-	    free_members(def->binding);
-	    free(def->binding);
-	}
-    }
     sudo_rcstr_delref(def->file);
     free(def->var);
     free(def->val);
@@ -1619,7 +1630,6 @@ free_cmndspecs(struct cmndspec_list *csl)
 void
 free_privilege(struct privilege *priv)
 {
-    struct member_list *prev_binding = NULL;
     struct defaults *def;
     debug_decl(free_privilege, SUDOERS_DEBUG_PARSER);
 
@@ -1628,7 +1638,7 @@ free_privilege(struct privilege *priv)
     free_cmndspecs(&priv->cmndlist);
     while ((def = TAILQ_FIRST(&priv->defaults)) != NULL) {
 	TAILQ_REMOVE(&priv->defaults, def, entries);
-	free_default(def, &prev_binding);
+	free_default(def);
     }
     free(priv);
 

plugins/sudoers/parse.c

@@ -695,11 +695,11 @@ display_defaults(struct sudoers_parse_tree *parse_tree, struct passwd *pw,
     TAILQ_FOREACH(d, &parse_tree->defaults, entries) {
 	switch (d->type) {
 	    case DEFAULTS_HOST:
-		if (hostlist_matches(parse_tree, pw, d->binding) != ALLOW)
+		if (hostlist_matches(parse_tree, pw, &d->binding->members) != ALLOW)
 		    continue;
 		break;
 	    case DEFAULTS_USER:
-		if (userlist_matches(parse_tree, pw, d->binding) != ALLOW)
+		if (userlist_matches(parse_tree, pw, &d->binding->members) != ALLOW)
 		    continue;
 		break;
 	    case DEFAULTS_RUNAS:
@@ -724,7 +724,7 @@ display_bound_defaults_by_type(struct sudoers_parse_tree *parse_tree,
     int deftype, struct sudo_lbuf *lbuf)
 {
     struct defaults *d;
-    struct member_list *binding = NULL;
+    struct defaults_binding *binding = NULL;
     struct member *m;
     char *dsep;
     int atype, nfound = 0;
@@ -760,8 +760,8 @@ display_bound_defaults_by_type(struct sudoers_parse_tree *parse_tree,
 	    if (nfound != 1)
 		sudo_lbuf_append(lbuf, "\n");
 	    sudo_lbuf_append(lbuf, "    Defaults%s", dsep);
-	    TAILQ_FOREACH(m, binding, entries) {
+	    TAILQ_FOREACH(m, &binding->members, entries) {
-		if (m != TAILQ_FIRST(binding))
+		if (m != TAILQ_FIRST(&binding->members))
 		    sudo_lbuf_append(lbuf, ", ");
 		sudoers_format_member(lbuf, parse_tree, m, ", ", atype);
 	    }

plugins/sudoers/parse.h

@@ -251,6 +251,11 @@ struct runascontainer {
     struct member *runasgroups;
 };
 
+struct defaults_binding {
+    struct member_list members;
+    unsigned int refcnt;
+};
+
 struct sudoers_comment {
     STAILQ_ENTRY(sudoers_comment) entries;
     char *str;
@@ -277,7 +282,7 @@ struct defaults {
     TAILQ_ENTRY(defaults) entries;
     char *var;				/* variable name */
     char *val;				/* variable value */
-    struct member_list *binding;	/* user/host/runas binding */
+    struct defaults_binding *binding;	/* user/host/runas binding */
     char *file;				/* file Defaults entry was in */
     short type;				/* DEFAULTS{,_USER,_RUNAS,_HOST} */
     char op;				/* true, false, '+', '-' */
@@ -364,7 +369,7 @@ void free_cmndspecs(struct cmndspec_list *csl);
 void free_privilege(struct privilege *priv);
 void free_userspec(struct userspec *us);
 void free_userspecs(struct userspec_list *usl);
-void free_default(struct defaults *def, struct member_list **binding);
+void free_default(struct defaults *def);
 void free_defaults(struct defaults_list *defs);
 void init_parse_tree(struct sudoers_parse_tree *parse_tree, char *lhost, char *shost);
 void free_parse_tree(struct sudoers_parse_tree *parse_tree);

plugins/sudoers/parse_ldif.c

@@ -252,7 +252,8 @@ ldif_store_options(struct sudoers_parse_tree *parse_tree,
 	    sudo_fatalx(U_("%s: %s"), __func__,
 		U_("unable to allocate memory"));
 	}
-	TAILQ_INIT(d->binding);
+	TAILQ_INIT(&d->binding->members);
+	d->binding->refcnt = 1;
 	d->type = DEFAULTS;
 	d->op = sudo_ldap_parse_option(ls->str, &var, &val);
 	if ((d->var = strdup(var)) == NULL) {