[PATCH 1/3] parse_opt_string_list: stop allocating new strings

From: Jeff King <peff@peff.net>
Date: Jun 13, 2016, 05:39 UTC
Message-ID: <20160613053912.GA23880@sigill.intra.peff.net>
In-Reply-To: <20160613053203.GB3950@sigill.intra.peff.net>

The parse_opt_string_list callback is basically a thin wrapper to string_list_append() any string options we get. However, it calls:

  string_list_append(v, xstrdup(arg));

which duplicates the option value. This is wrong for two reasons:

  1. If the string list has strdup_strings set, then we are
     making an extra copy, which is simply leaked.

  2. If the string list does not have strdup_strings set,
     then we pass memory ownership to the string list, but
     it does not realize this. If we later call
     string_list_clear(), which can happen if "--no-foo" is
     passed, then we will leak all of the existing entries.

Instead, we should just pass the argument straight to string_list_append, and it can decide whether to copy or not based on its strdup_strings flag.

It's possible that some (buggy) caller could be relying on this extra copy (e.g., because it parses some options from an allocated argv array and then frees the array), but it's not likely. For one, we generally only use parse_options on the argv given to us in main(). And two, such a caller is broken anyway, because other option types like OPT_STRING() do not make such a copy. This patch brings us in line with them.

Noticed-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 parse-options-cb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/parse-options-cb.c b/parse-options-cb.c
index 239898d..ba5acf3 100644
--- a/parse-options-cb.c
+++ b/parse-options-cb.c
@@ -144,7 +144,7 @@ int parse_opt_string_list(const struct option *opt, const char *arg, int unset)
 	if (!arg)
 		return -1;
 
-	string_list_append(v, xstrdup(arg));
+	string_list_append(v, arg);
 	return 0;
 }

-- 
2.9.0.rc2.149.gd580ccd