From f1dcf6b707a5e7834de38c595f29461537df9e81 Mon Sep 17 00:00:00 2001 From: Albert Chu Date: Tue, 22 Jan 2019 11:53:48 -0800 Subject: [PATCH 1/3] modules/kvs/kvstxn: Fix mem-leak in kvs merge In kvstxn_create_empty(), the 'keys' array was created that could be overwritten without being destroyed. This was a corner case in PR #1859, when key generation was altered. Fixes #1944 --- src/modules/kvs/kvstxn.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/modules/kvs/kvstxn.c b/src/modules/kvs/kvstxn.c index 144e4ef71a5e..2191138abcca 100644 --- a/src/modules/kvs/kvstxn.c +++ b/src/modules/kvs/kvstxn.c @@ -1236,8 +1236,6 @@ static kvstxn_t *kvstxn_create_empty (kvstxn_mgr_t *ktm, int flags) goto error_enomem; if (!(ktnew->ops = json_array ())) goto error_enomem; - if (!(ktnew->keys = json_array ())) - goto error_enomem; if (!(ktnew->names = json_array ())) goto error_enomem; if (!(ktnew->missing_refs_list = zlist_new ())) From f8dacb598b8001407dff17a710168112427d605f Mon Sep 17 00:00:00 2001 From: Albert Chu Date: Tue, 22 Jan 2019 16:01:10 -0800 Subject: [PATCH 2/3] modules/kvs: Remove kvstxn_create_empty() Allow kvstxn_create() to take several NULL pointers, so that kvstxn_create_empty() can be removed. --- src/modules/kvs/kvstxn.c | 41 ++++++++++++---------------------------- 1 file changed, 12 insertions(+), 29 deletions(-) diff --git a/src/modules/kvs/kvstxn.c b/src/modules/kvs/kvstxn.c index 2191138abcca..3139ffa34317 100644 --- a/src/modules/kvs/kvstxn.c +++ b/src/modules/kvs/kvstxn.c @@ -96,9 +96,17 @@ static kvstxn_t *kvstxn_create (kvstxn_mgr_t *ktm, saved_errno = ENOMEM; goto error; } - if (!(kt->ops = json_copy (ops))) { - saved_errno = ENOMEM; - goto error; + if (ops) { + if (!(kt->ops = json_copy (ops))) { + saved_errno = ENOMEM; + goto error; + } + } + else { + if (!(kt->ops = json_array ())) { + saved_errno = ENOMEM; + goto error; + } } if (!(kt->names = json_array ())) { saved_errno = ENOMEM; @@ -1228,31 +1236,6 @@ static int kvstxn_merge (kvstxn_t *dest, kvstxn_t *src) return 1; } -static kvstxn_t *kvstxn_create_empty (kvstxn_mgr_t *ktm, int flags) -{ - kvstxn_t *ktnew; - - if (!(ktnew = calloc (1, sizeof (*ktnew)))) - goto error_enomem; - if (!(ktnew->ops = json_array ())) - goto error_enomem; - if (!(ktnew->names = json_array ())) - goto error_enomem; - if (!(ktnew->missing_refs_list = zlist_new ())) - goto error_enomem; - if (!(ktnew->dirty_cache_entries_list = zlist_new ())) - goto error_enomem; - ktnew->flags = flags; - ktnew->ktm = ktm; - ktnew->state = KVSTXN_STATE_INIT; - return ktnew; - -error_enomem: - kvstxn_destroy (ktnew); - errno = ENOMEM; - return NULL; -} - /* Merge ready transactions that are mergeable, where merging consists * creating a new kvstxn_t, and merging the other transactions in the * ready queue and appending their ops/names to the new transaction. @@ -1295,7 +1278,7 @@ int kvstxn_mgr_merge_ready_transactions (kvstxn_mgr_t *ktm) || (first->flags != second->flags)) return 0; - if (!(new = kvstxn_create_empty (ktm, first->flags))) + if (!(new = kvstxn_create (ktm, NULL, NULL, first->flags))) return -1; new->internal_flags |= KVSTXN_MERGED; From ff7a4a27cdc3c2602115e983d3b8686b1f669f02 Mon Sep 17 00:00:00 2001 From: Albert Chu Date: Tue, 22 Jan 2019 16:03:42 -0800 Subject: [PATCH 3/3] modules/kvs: Refactor kvstxn_create error handling All "goto error" paths were ENOMEM errnos, so cleanup code by not setting "errno = ENOMEM" in all paths. --- src/modules/kvs/kvstxn.c | 50 ++++++++++++++-------------------------- 1 file changed, 17 insertions(+), 33 deletions(-) diff --git a/src/modules/kvs/kvstxn.c b/src/modules/kvs/kvstxn.c index 3139ffa34317..7a5761472c27 100644 --- a/src/modules/kvs/kvstxn.c +++ b/src/modules/kvs/kvstxn.c @@ -90,55 +90,39 @@ static kvstxn_t *kvstxn_create (kvstxn_mgr_t *ktm, int flags) { kvstxn_t *kt; - int saved_errno; - if (!(kt = calloc (1, sizeof (*kt)))) { - saved_errno = ENOMEM; - goto error; - } + if (!(kt = calloc (1, sizeof (*kt)))) + goto error_enomem; if (ops) { - if (!(kt->ops = json_copy (ops))) { - saved_errno = ENOMEM; - goto error; - } + if (!(kt->ops = json_copy (ops))) + goto error_enomem; } else { - if (!(kt->ops = json_array ())) { - saved_errno = ENOMEM; - goto error; - } - } - if (!(kt->names = json_array ())) { - saved_errno = ENOMEM; - goto error; + if (!(kt->ops = json_array ())) + goto error_enomem; } + if (!(kt->names = json_array ())) + goto error_enomem; if (name) { json_t *s; - if (!(s = json_string (name))) { - saved_errno = ENOMEM; - goto error; - } + if (!(s = json_string (name))) + goto error_enomem; if (json_array_append_new (kt->names, s) < 0) { json_decref (s); - saved_errno = ENOMEM; - goto error; + goto error_enomem; } } kt->flags = flags; - if (!(kt->missing_refs_list = zlist_new ())) { - saved_errno = ENOMEM; - goto error; - } - if (!(kt->dirty_cache_entries_list = zlist_new ())) { - saved_errno = ENOMEM; - goto error; - } + if (!(kt->missing_refs_list = zlist_new ())) + goto error_enomem; + if (!(kt->dirty_cache_entries_list = zlist_new ())) + goto error_enomem; kt->ktm = ktm; kt->state = KVSTXN_STATE_INIT; return kt; - error: + error_enomem: kvstxn_destroy (kt); - errno = saved_errno; + errno = ENOMEM; return NULL; }