On 27 October 2017 at 17:06, Pranit Bauva <pranit.bauva@xxxxxxxxx> wrote:
> +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@xxxxxxxxx>
---
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
