Skip to content

Commit

Permalink
KCM: fix memory leak
Browse files Browse the repository at this point in the history
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.

This is a backport of #7823

:fixes:'sssd_kcm' memory leak was fixed.

Reviewed-by: Alejandro López <[email protected]>
Reviewed-by: Justin Stephenson <[email protected]>
(cherry picked from commit 6aba9a7)
  • Loading branch information
alexey-tikhonov committed Feb 7, 2025
1 parent 402c9a0 commit 58547f0
Showing 1 changed file with 12 additions and 16 deletions.
28 changes: 12 additions & 16 deletions src/responder/kcm/secrets/secrets.c
Original file line number Diff line number Diff line change
Expand Up @@ -953,7 +953,7 @@ errno_t sss_sec_put(struct sss_sec_req *req,
size_t secret_len)
{
struct ldb_message *msg;
struct ldb_val secret_val;
const struct ldb_val secret_val = { .length = secret_len, .data = secret };
int ret;

if (req == NULL || secret == NULL) {
Expand Down Expand Up @@ -1002,13 +1002,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,
Expand Down Expand Up @@ -1050,7 +1048,7 @@ errno_t sss_sec_update(struct sss_sec_req *req,
size_t secret_len)
{
struct ldb_message *msg;
struct ldb_val secret_val;
const struct ldb_val secret_val = { .length = secret_len, .data = secret };
int ret;

if (req == NULL || secret == NULL) {
Expand Down Expand Up @@ -1099,13 +1097,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) {
Expand All @@ -1115,6 +1106,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,
Expand Down

0 comments on commit 58547f0

Please sign in to comment.