IPA: memory leak fixed#8632
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the memory context used for base64 decoding in get_extra_attrs to use resp_attrs->sysdb_attrs. Feedback highlights memory leaks in error paths where allocated resources like name and values are not freed, and suggests manual cleanup of decoded data to avoid memory accumulation within the request context.
| v.data = sss_base64_decode(resp_attrs->sysdb_attrs, values[c]->bv_val, &v.length); | ||
| if (v.data == NULL) { | ||
| DEBUG(SSSDBG_OP_FAILURE, "sss_base64_decode failed.\n"); | ||
| return EINVAL; |
There was a problem hiding this comment.
The error path when sss_base64_decode fails (line 659) leaks the memory allocated for name and values by ber_scanf. These resources should be explicitly freed before returning EINVAL. Note that a similar leak exists on the error path at line 653.
Additionally, while using resp_attrs->sysdb_attrs as a context prevents a permanent leak to the global context, it results in memory accumulation within the request context because sysdb_attrs_add_val_safe duplicates the data. For better efficiency, consider freeing v.data once it has been successfully added to the attributes.
v.data = sss_base64_decode(resp_attrs->sysdb_attrs, values[c]->bv_val, &v.length);
if (v.data == NULL) {
DEBUG(SSSDBG_OP_FAILURE, "sss_base64_decode failed.\n");
ldap_memfree(name);
ber_bvecfree(values);
return EINVAL;
}There was a problem hiding this comment.
For better efficiency, consider freeing v.data once it has been successfully added to the attributes.
That would be cumbersome because mem is allocated only for SYSDB_USER_CERT.
alexey-tikhonov
force-pushed
the
base64-memleak
branch
from
ffc441d to
8ccf4e5
Compare
sumit-bose
left a comment
sumit-bose
left a comment
There was a problem hiding this comment.
Hi,
thank you for the fix, ACK.
bye,
Sumit
|
Failing |
alexey-tikhonov
force-pushed
the
base64-memleak
branch
from
8ccf4e5 to
759662f
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request fixes memory leaks in src/providers/ipa/ipa_s2n_exop.c by ensuring that name and values are properly freed in error paths. It also updates the memory context used for base64 decoding. A review comment suggests that using resp_attrs->sysdb_attrs as the context for sss_base64_decode may lead to memory accumulation because the data is copied later in the function, recommending that the decoded buffer be freed explicitly or handled via a temporary context.
|
@arrowd, Not sure if this is tmp infra issue or what... |
|
Yep, this is a problem on our side. Let's ignore FreeBSD failures in the CI for now until we fix our stuff. |
Result allocated by `sss_base64_decode()` on NULL context was never freed. Reviewed-by: Alejandro López <allopez@redhat.com> Reviewed-by: Sumit Bose <sbose@redhat.com>
sssd-bot
force-pushed
the
base64-memleak
branch
from
759662f to
36b98fc
Compare
5 participants
Result allocated by
sss_base64_decode()on NULL context was never freed.