Re: [PATCH v16 Part II 2/8] bisect--helper: `bisect_write` shell function in C

From: Martin Ågren <martin.agren@gmail.com>
Date: Oct 27, 2017, 17:28 UTC
Message-ID: <20171027172845.15437-1-martin.agren@gmail.com>
In-Reply-To: <0102015f5e5ee22c-ea9c3a38-5d42-4dce-a54b-45c59768a70b-000000@eu-west-1.amazonses.com>

On 27 October 2017 at 17:06, Pranit Bauva <pranit.bauva@gmail.com> wrote:

Show 7 quoted lines

> +static void free_terms(struct bisect_terms *terms)
> +{
> +       if (!terms->term_good)
> +               free((void *) terms->term_good);
> +       if (!terms->term_bad)
> +               free((void *) terms->term_bad);
> +}

These look like no-ops. Remove `!` for correctness, or `if (...)` for simplicity, since `free()` can handle NULL.

You leave the pointers dangling, but you're ok for now since this is the last thing that happens in `cmd_bisect__helper()`. Your later patches add more users, but they're also ok, since they immediately assign new values.

In case you (and others) find it useful, the below is a patch I've been sitting on for a while as part of a series to plug various memory-leaks. `FREE_AND_NULL_CONST()` would be useful in precisely these situations.

-- >8 --
Subject: git-compat-util: add FREE_AND_NULL_CONST() wrapper

Commit 481df65 ("git-compat-util: add a FREE_AND_NULL() wrapper around free(ptr); ptr = NULL", 2017-06-15) added `FREE_AND_NULL()`. One advantage of this macro is that it reduces risk of copy-paste errors and reviewer-fatigue, especially when cleaning up more than just a single pointer.

However, `FREE_AND_NULL()` can not be used with a const-pointer:

  struct foo { const char *bar; }
  ...
  FREE_AND_NULL(baz.bar);

This causes the compiler to barf as `free()` is called: "error: passing argument 1 of ‘free’ discards ‘const’ qualifier from pointer target type". A naive attempt to remedy this might look like this:

  FREE_AND_NULL((void *)baz.bar);

Now the assignment is problematic: "error: lvalue required as left operand of assignment".

Add a `FREE_AND_NULL_CONST()` wrapper macro which acts as `FREE_AND_NULL()`, except it casts to `(void *)` when freeing. As a demonstration, use this in two existing code paths that were identified by some grepping. Future patches will use it to clean up struct-fields:

  FREE_AND_NULL_CONST(baz.bar);

This macro is a slightly bigger hammer than `FREE_AND_NULL` and has a slightly larger potential for misuse. Document that `FREE_AND_NULL` should be preferred.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 argv-array.c      | 3 +--
 git-compat-util.h | 8 ++++++++
 submodule.c       | 3 +--
 3 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/argv-array.c b/argv-array.c
index 5d370fa33..433a5997a 100644
--- a/argv-array.c
+++ b/argv-array.c
@@ -59,8 +59,7 @@ void argv_array_pop(struct argv_array *array)
 {
 	if (!array->argc)
 		return;
-	free((char *)array->argv[array->argc - 1]);
-	array->argv[array->argc - 1] = NULL;
+	FREE_AND_NULL_CONST(array->argv[array->argc - 1]);
 	array->argc--;
 }
 
diff --git a/git-compat-util.h b/git-compat-util.h
index cedad4d58..ca3dcbc58 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -815,6 +815,14 @@ extern FILE *fopen_or_warn(const char *path, const char *mode);
  */
 #define FREE_AND_NULL(p) do { free(p); (p) = NULL; } while (0)
 
+/*
+ * FREE_AND_NULL_CONST(ptr) is like FREE_AND_NULL(ptr) except it casts
+ * to (void *) when calling free. This is useful when handling, e.g., a
+ * `const char *`, but it can also be misused. Prefer FREE_AND_NULL, and
+ * use this only when you need to and it is safe to do so.
+ */
+#define FREE_AND_NULL_CONST(p) do { free((void *)(p)); (p) = NULL; } while (0)
+
 #define ALLOC_ARRAY(x, alloc) (x) = xmalloc(st_mult(sizeof(*(x)), (alloc)))
 #define REALLOC_ARRAY(x, alloc) (x) = xrealloc((x), st_mult(sizeof(*(x)), (alloc)))
 
diff --git a/submodule.c b/submodule.c
index 63e7094e1..7759f9050 100644
--- a/submodule.c
+++ b/submodule.c
@@ -364,8 +364,7 @@ int parse_submodule_update_strategy(const char *value,
 {
 	enum submodule_update_type type;
 
-	free((void*)dst->command);
-	dst->command = NULL;
+	FREE_AND_NULL_CONST(dst->command);
 
 	type = parse_submodule_update_type(value);
 	if (type == SM_UPDATE_UNSPECIFIED)

-- 
2.15.0.rc2.5.g97dd00bb7