[PATCH] value-pairs: Fix a double free on parse errors
In the command-line parser code, when we encountered an error, we explicitly free'd the value-pairs structure, but did not return. A few lines later, we called vp_cmdline_parse_rekey_finish(), which also tried to free the same structure. Instead, we should only call _rekey_finish() if we didn't NULL out the value pairs structure earlier. Signed-off-by: Gergely Nagy <algernon@balabit.hu> --- lib/value-pairs.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/lib/value-pairs.c b/lib/value-pairs.c index 12cf831..912164f 100644 --- a/lib/value-pairs.c +++ b/lib/value-pairs.c @@ -678,7 +678,8 @@ value_pairs_new_from_cmdline (GlobalConfig *cfg, vp = NULL; } g_option_context_free (ctx); - vp_cmdline_parse_rekey_finish (user_data_args); + if (vp) + vp_cmdline_parse_rekey_finish (user_data_args); return vp; } -- 1.7.9.1
On Wed, 2012-03-14 at 10:31 +0100, Gergely Nagy wrote:
In the command-line parser code, when we encountered an error, we explicitly free'd the value-pairs structure, but did not return. A few lines later, we called vp_cmdline_parse_rekey_finish(), which also tried to free the same structure.
Instead, we should only call _rekey_finish() if we didn't NULL out the value pairs structure earlier.
Signed-off-by: Gergely Nagy <algernon@balabit.hu> --- lib/value-pairs.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/lib/value-pairs.c b/lib/value-pairs.c index 12cf831..912164f 100644 --- a/lib/value-pairs.c +++ b/lib/value-pairs.c @@ -678,7 +678,8 @@ value_pairs_new_from_cmdline (GlobalConfig *cfg, vp = NULL; } g_option_context_free (ctx); - vp_cmdline_parse_rekey_finish (user_data_args); + if (vp) + vp_cmdline_parse_rekey_finish (user_data_args);
Ops, I haven't noticed this patch, however I think my solution (just posted in the other thread) is somewhat better. You may have some data allocated by the parser, which wouldn't be freed in this case. Please have a look at my patch and comment if that's ok. Thanks. -- Bazsi
Balazs Scheidler <bazsi@balabit.hu> writes:
Ops, I haven't noticed this patch, however I think my solution (just posted in the other thread) is somewhat better.
You may have some data allocated by the parser, which wouldn't be freed in this case.
Please have a look at my patch and comment if that's ok.
Mmm, it's certainly nicer, but it calls the vp transform set finisher even on the error path, which might not be the best idea (that can also allocate stuff). On the other hand, that stuff might be freed elsewhere, so it's probably not a big deal. I'll take it to a test drive during the long weekend, but nevertheless, your approach does look a lot better. -- |8]
participants (2)
-
Balazs Scheidler
-
Gergely Nagy