From 257a0bf128e109442dce91c4aaa1d97fed17ad1a Mon Sep 17 00:00:00 2001 From: Robin Haberkorn Date: Wed, 19 Mar 2025 12:32:10 +0300 Subject: fixed leaking data on rubout * Objects, that are restored with TECO_DEFINE_UNDO_OBJECT_OWN(), could actually leak memory on rubout since the old object was not deleted when overwriting it. * Now that it is, it is crucial to at least nullify objects/pointers after calling the corresponding push-function. These conditions are now explicitly documented. * The test suite now runs successfully under Valgrind even with full leak checking. --- src/goto-commands.c | 2 ++ src/qreg-commands.h | 1 + src/qreg.c | 2 ++ src/undo.h | 37 +++++++++++++++++++++---------------- tests/testsuite.at | 3 +++ 5 files changed, 29 insertions(+), 16 deletions(-) diff --git a/src/goto-commands.c b/src/goto-commands.c index d463151..bf5743e 100644 --- a/src/goto-commands.c +++ b/src/goto-commands.c @@ -90,6 +90,8 @@ teco_state_label_input(teco_machine_main_t *ctx, gunichar chr, GError **error) teco_undo_string_own(ctx->goto_label); else teco_string_clear(&ctx->goto_label); + memset(&ctx->goto_label, 0, sizeof(ctx->goto_label)); + return &teco_state_start; } diff --git a/src/qreg-commands.h b/src/qreg-commands.h index 6dbd1c4..f6ad82a 100644 --- a/src/qreg-commands.h +++ b/src/qreg-commands.h @@ -30,6 +30,7 @@ teco_state_expectqreg_reset(teco_machine_main_t *ctx) teco_undo_qregspec_own(ctx->expectqreg); else teco_machine_qregspec_free(ctx->expectqreg); + ctx->expectqreg = NULL; } gboolean teco_state_expectqreg_initial(teco_machine_main_t *ctx, GError **error); diff --git a/src/qreg.c b/src/qreg.c index 4beea74..1b97689 100644 --- a/src/qreg.c +++ b/src/qreg.c @@ -1708,6 +1708,8 @@ teco_machine_qregspec_auto_complete(teco_machine_qregspec_t *ctx, teco_string_t void teco_machine_qregspec_free(teco_machine_qregspec_t *ctx) { + if (!ctx) + return; teco_machine_stringbuilding_clear(&ctx->machine_stringbuilding); teco_string_clear(&ctx->name); g_free(ctx); diff --git a/src/undo.h b/src/undo.h index d7b7c8e..6fbb1c1 100644 --- a/src/undo.h +++ b/src/undo.h @@ -70,15 +70,14 @@ gpointer teco_undo_push_size(teco_undo_action_t action_cb, gsize size) */ /* * FIXME: Due to the requirements on the variable, we could be tempted to inline - * references to it directly into the action()-function, saving the `ptr` + * references to it directly into the action()-function, getting rid of the `ptr` * in the undo token. This is however often practically not possible. * We could however add a variant for true global variables. + * Or perhaps just use TECO_DEFINE_UNDO_CALL() in these few cases. * * FIXME: Sometimes, the push-function is used only in a single compilation unit, * so it should be declared `static` or `static inline`. * Is it worth complicating our APIs in order to support that? - * - * FIXME: Perhaps better split this into TECO_DEFINE_UNDO_OBJECT() and TECO_DEFINE_UNDO_OBJECT_OWN() */ #define __TECO_DEFINE_UNDO_OBJECT(NAME, TYPE, COPY, DELETE, DELETE_IF_DISABLED, DELETE_ON_RUN) \ typedef struct { \ @@ -125,24 +124,30 @@ gpointer teco_undo_push_size(teco_undo_action_t action_cb, gsize size) * * @param NAME C identifier used for name mangling. * @param TYPE Type of variable to restore. - * @param COPY A global function/expression to execute in order to copy VAR. - * If left empty, this is an identity operation and ownership - * of the variable is passed to the undo token. - * @param DELETE A global function/expression to execute in order to destruct - * objects of TYPE. Leave empty if destruction is not necessary. + * @param COPY A global function/macro to execute in order to copy VAR. + * If left empty, this is an identity operation and ownership + * of the variable is passed to the undo token. + * @param DELETE A global function/macro to execute in order to destruct + * objects of TYPE. Leave empty if destruction is not necessary. * * @ingroup undo_objects */ #define TECO_DEFINE_UNDO_OBJECT(NAME, TYPE, COPY, DELETE) \ __TECO_DEFINE_UNDO_OBJECT(NAME, TYPE, COPY, DELETE, /* don't delete if disabled */, DELETE) + /** - * @fixme _OWN variants will invalidate the variable pointer, so perhaps - * it will be clearer to have _SET variants instead. + * This is like TECO_DEFINE_UNDO_OBJECT(), but passes the ownership + * of the variable to the undo token. + * + * If DELETE is specified, this will invalidate the variable after calling the push-function. + * At undo-time however, it is expected to be valid. + * Therefore it is crucial to set the variable afterwards or at least to nullify it. + * The DELETE function may consequently have to deal with NULL pointers. * * @ingroup undo_objects */ #define TECO_DEFINE_UNDO_OBJECT_OWN(NAME, TYPE, DELETE) \ - __TECO_DEFINE_UNDO_OBJECT(NAME, TYPE, /* pass ownership */, DELETE, DELETE, /* don't delete if run */) + __TECO_DEFINE_UNDO_OBJECT(NAME, TYPE, /* pass ownership */, DELETE, DELETE, DELETE) /** @ingroup undo_objects */ #define TECO_DECLARE_UNDO_OBJECT(NAME, TYPE) \ @@ -150,7 +155,7 @@ gpointer teco_undo_push_size(teco_undo_action_t action_cb, gsize size) /** @ingroup undo_objects */ #define TECO_DEFINE_UNDO_SCALAR(TYPE) \ - TECO_DEFINE_UNDO_OBJECT_OWN(TYPE, TYPE, /* don't delete */) + TECO_DEFINE_UNDO_OBJECT(TYPE, TYPE, /* don't copy */, /* don't delete */) /** @ingroup undo_objects */ #define TECO_DECLARE_UNDO_SCALAR(TYPE) \ @@ -211,13 +216,13 @@ TECO_DECLARE_UNDO_SCALAR(gconstpointer); * "constant" parameters. * * @param FNC Name of a global function or macro to execute. - * It must be a plain C identifier. + * It must be a plain C identifier. * @param ... The parameter types of FNC (signature). - * Only the types without any variable names must be specified. + * Only the types without any variable names must be specified. * * @fixme Sometimes, the push-function is used only in a single compilation unit, - * so it should be declared `static` or `static inline`. - * Is it worth complicating our APIs in order to support that? + * so it should be declared `static` or `static inline`. + * Is it worth complicating our APIs in order to support that? */ #define TECO_DEFINE_UNDO_CALL(FNC, ...) \ typedef struct { \ diff --git a/tests/testsuite.at b/tests/testsuite.at index 05ec2e3..86f8331 100644 --- a/tests/testsuite.at +++ b/tests/testsuite.at @@ -287,6 +287,9 @@ AT_CHECK([$SCITECO -e '2'], 0, ignore, stderr) AT_FAIL_IF([$GREP "^Warning:" stderr]) # Will print a warning about label redefinition, though... AT_CHECK([$SCITECO -e "!foo! Qa\"S^C' !foo! Qa\"S(0/0)' -Ua @O/foo/"], 0, ignore, ignore) +# This should not leak memory or cause memory corruptions when running under +# Valgrind or Asan: +AT_CHECK([$SCITECO_CMDLINE "!foo!{-5D}"], 0, ignore, stderr) AT_CLEANUP # -- cgit v1.2.3