From 6a4c0e8fe5ba1b8060132b12d549e82dbd3411e2 Mon Sep 17 00:00:00 2001 From: Andre Noll Date: Sun, 13 Feb 2022 20:46:20 +0100 Subject: [PATCH] lls_parse_arg(): Avoid NULL pointer dereference. If a string option with multiple=false is given twice at the command line, the second and all subsequent calls to lls_parse_arg() discard the previous value stored in lor->value[0]. However, if the option takes an optional argument and was first specified without argument, lor->value remains NULL because the first call to lls_parse_arg() returned early due to the shortcut at the beginning of the function while the second call skips the allocation because lor->given is increased also when no argument is given, so it equals one during the second call. Thus, the attempt to free lor->value[0] further down in the function results in a NULL pointer dereference. Fix this by checking lor->value rather then lor->given. To make this work we have to move up the code which frees the old string value. While at it, reduce memory usage by not over-sizing the array. We now only allocate space for idx + 1 values rather than lor->given + 1. This is different in the case mentioned above. --- lopsub.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/lopsub.c b/lopsub.c index cbf6661..b5017c3 100644 --- a/lopsub.c +++ b/lopsub.c @@ -632,17 +632,23 @@ static int lls_parse_arg(struct lls_arg *la, const struct lls_option *opts, bool multiple; int idx, ret; - if (!la->arg) - goto success; - if (opt->arg_info == LLS_NO_ARGUMENT) { + if (la->arg && opt->arg_info == LLS_NO_ARGUMENT) { xasprintf(errctx, "arg: %s, option: %s", la->arg, opt->name); return -E_LLS_ARG_GIVEN; } multiple = opt->flags & LLS_MULTIPLE; + if (opt->arg_type == LLS_STRING && !opt->values && !multiple) { + if (lor->value) { /* discard previous value */ + free(lor->value[0].string_val); + free(lor->value); + lor->value = NULL; + } + } + if (!la->arg) + goto success; idx = multiple? lor->given : 0; - if (lor->given == 0 || multiple) { - ret = xrealloc(&lor->value, - (lor->given + 1) * sizeof(*lor->value)); + if (!lor->value || multiple) { + ret = xrealloc(&lor->value, (idx + 1) * sizeof(*lor->value)); if (ret < 0) { xasprintf(errctx, "option value array for --%s", opt->name); @@ -651,8 +657,6 @@ static int lls_parse_arg(struct lls_arg *la, const struct lls_option *opts, } switch (opt->arg_type) { case LLS_STRING: - if (!opt->values && lor->given > 0 && !multiple) - free(lor->value[idx].string_val); if (opt->values) { ret = check_enum_arg(la->arg, opt, errctx); if (ret < 0) -- 2.39.2