114 lines
3.9 KiB
Diff
114 lines
3.9 KiB
Diff
From 50f703f25914254d2a545f52f504dfa5a6f65546 Mon Sep 17 00:00:00 2001
|
|
From: Alexey Tikhonov <atikhono@redhat.com>
|
|
Date: Tue, 4 Feb 2025 18:59:36 +0100
|
|
Subject: [PATCH] KCM: fix memory leak
|
|
MIME-Version: 1.0
|
|
Content-Type: text/plain; charset=UTF-8
|
|
Content-Transfer-Encoding: 8bit
|
|
|
|
The copy of 'secret' argument - `secret_val.data` - was left hanging
|
|
on `sss_sec_ctx`, effectively resulting in a memory leak.
|
|
But this copy isn't actually required as this data isn't modified in
|
|
below operations.
|
|
Skipping alloc+memcpy+erase is also beneficial performance wise.
|
|
|
|
:fixes:'sssd_kcm' memory leak was fixed.
|
|
|
|
Reviewed-by: Alejandro López <allopez@redhat.com>
|
|
Reviewed-by: Justin Stephenson <jstephen@redhat.com>
|
|
(cherry picked from commit 7f1b7c9689827df92e8b2166423d4e80688dbacb)
|
|
---
|
|
src/responder/kcm/secrets/secrets.c | 34 ++++++++++-------------------
|
|
1 file changed, 12 insertions(+), 22 deletions(-)
|
|
|
|
diff --git a/src/responder/kcm/secrets/secrets.c b/src/responder/kcm/secrets/secrets.c
|
|
index 625a09f39..fe7410cb3 100644
|
|
--- a/src/responder/kcm/secrets/secrets.c
|
|
+++ b/src/responder/kcm/secrets/secrets.c
|
|
@@ -979,7 +979,7 @@ errno_t sss_sec_put(struct sss_sec_req *req,
|
|
size_t secret_len)
|
|
{
|
|
struct ldb_message *msg;
|
|
- struct ldb_val secret_val = { .data = NULL };
|
|
+ const struct ldb_val secret_val = { .length = secret_len, .data = secret };
|
|
bool erase_msg = false;
|
|
int ret;
|
|
|
|
@@ -1029,13 +1029,11 @@ errno_t sss_sec_put(struct sss_sec_req *req,
|
|
goto done;
|
|
}
|
|
|
|
- secret_val.length = secret_len;
|
|
- secret_val.data = talloc_memdup(req->sctx, secret, secret_len);
|
|
- if (!secret_val.data) {
|
|
- ret = ENOMEM;
|
|
- goto done;
|
|
- }
|
|
-
|
|
+ /* `ldb_msg_add_value()` does NOT make a copy of secret_val::*data
|
|
+ * but rather copies a pointer under the hood.
|
|
+ * This is fine since no operations modifying this data are performed
|
|
+ * below and 'msg' is freed before function returns.
|
|
+ */
|
|
ret = ldb_msg_add_value(msg, SEC_ATTR_SECRET, &secret_val, NULL);
|
|
if (ret != EOK) {
|
|
DEBUG(SSSDBG_OP_FAILURE,
|
|
@@ -1069,9 +1067,6 @@ errno_t sss_sec_put(struct sss_sec_req *req,
|
|
|
|
ret = EOK;
|
|
done:
|
|
- if (secret_val.data != NULL) {
|
|
- sss_erase_mem_securely(secret_val.data, secret_val.length);
|
|
- }
|
|
if (erase_msg) {
|
|
db_result_erase_message_securely(msg, SEC_ATTR_SECRET);
|
|
}
|
|
@@ -1084,7 +1079,7 @@ errno_t sss_sec_update(struct sss_sec_req *req,
|
|
size_t secret_len)
|
|
{
|
|
struct ldb_message *msg;
|
|
- struct ldb_val secret_val = { .data = NULL };
|
|
+ const struct ldb_val secret_val = { .length = secret_len, .data = secret };
|
|
bool erase_msg = false;
|
|
int ret;
|
|
|
|
@@ -1134,13 +1129,6 @@ errno_t sss_sec_update(struct sss_sec_req *req,
|
|
goto done;
|
|
}
|
|
|
|
- secret_val.length = secret_len;
|
|
- secret_val.data = talloc_memdup(req->sctx, secret, secret_len);
|
|
- if (!secret_val.data) {
|
|
- ret = ENOMEM;
|
|
- goto done;
|
|
- }
|
|
-
|
|
/* FIXME - should we have a lastUpdate timestamp? */
|
|
ret = ldb_msg_add_empty(msg, SEC_ATTR_SECRET, LDB_FLAG_MOD_REPLACE, NULL);
|
|
if (ret != LDB_SUCCESS) {
|
|
@@ -1150,6 +1138,11 @@ errno_t sss_sec_update(struct sss_sec_req *req,
|
|
goto done;
|
|
}
|
|
|
|
+ /* `ldb_msg_add_value()` does NOT make a copy of secret_val::*data
|
|
+ * but rather copies a pointer under the hood.
|
|
+ * This is fine since no operations modifying this data are performed
|
|
+ * below and 'msg' is freed before function returns.
|
|
+ */
|
|
ret = ldb_msg_add_value(msg, SEC_ATTR_SECRET, &secret_val, NULL);
|
|
if (ret != LDB_SUCCESS) {
|
|
DEBUG(SSSDBG_MINOR_FAILURE,
|
|
@@ -1174,9 +1167,6 @@ errno_t sss_sec_update(struct sss_sec_req *req,
|
|
|
|
ret = EOK;
|
|
done:
|
|
- if (secret_val.data != NULL) {
|
|
- sss_erase_mem_securely(secret_val.data, secret_val.length);
|
|
- }
|
|
if (erase_msg) {
|
|
db_result_erase_message_securely(msg, SEC_ATTR_SECRET);
|
|
}
|
|
--
|
|
2.47.0
|
|
|