Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix memory leaks when tokens are missing #463

Merged
merged 1 commit into from
Nov 7, 2024
Merged

Conversation

simo5
Copy link
Member

@simo5 simo5 commented Nov 6, 2024

Description

In case we have slots advertized but the driver fails to return information or the token is not present we were leaking memory as the slot is not added to the array and the number of slots is not incremented.

Ensure the slot struct is freed in this case.

Fixes #462

Checklist

  • Code modified for feature
  • Test suite updated with functionality tests
  • Test suite updated with negative tests
  • Documentation updated

Reviewer's checklist:

  • Any issues marked for closing are addressed
  • There is a test suite reasonably covering new functionality or modifications
  • This feature/change has adequate documentation added
  • Code conform to coding style that today cannot yet be enforced via the check style test
  • Commits have short titles and sensible commit messages
  • Coverity Scan has run if needed (code PR) and no new defects were found

@simo5 simo5 requested a review from Jakuje November 6, 2024 16:00
src/slot.c Show resolved Hide resolved
Jakuje
Jakuje previously approved these changes Nov 7, 2024
src/slot.c Show resolved Hide resolved
@Jakuje Jakuje added the covscan Triggers Coverity Scanner label Nov 7, 2024
@github-actions github-actions bot removed the covscan Triggers Coverity Scanner label Nov 7, 2024
@DEX1999
Copy link

DEX1999 commented Nov 7, 2024

And what about leak when you allocate num elements on string 169, which equals to numbers of slots in system, and then if sctx->num == 0, which is equals to number of slots with tokens, you just return without freeing.

@Jakuje
Copy link
Contributor

Jakuje commented Nov 7, 2024

Good catch! I did not go deep into the p11prov_free_slots() but this condition needs to be probably removed to make sure the sctx->slots and sctx gets freed also when we fail before allocating the slots context:

pkcs11-provider/src/slot.c

Lines 351 to 353 in e064032

if (sctx->num == 0) {
return;
}

@simo5
Copy link
Member Author

simo5 commented Nov 7, 2024

And what about leak when you allocate num elements on string 169, which equals to numbers of slots in system, and then if sctx->num == 0, which is equals to number of slots with tokens, you just return without freeing.

It is a corner case not worth optimizing for IMHO.
Especially because I have a plan, in the future, to keep slots around that do not have token present as we have an interest in supporting token insertion in the future.

In case we have slots advertized but the driver fails to return
information or the token is not present we were leaking memory as the
slot is not added to the array and the number of slots is not
incremented.

Ensure the slot struct is freed in this case.
And ensure the slot is assigned and count incremented at the same time
to avoid leaks from other error conditions.

Signed-off-by: Simo Sorce <simo@redhat.com>
@simo5
Copy link
Member Author

simo5 commented Nov 7, 2024

And what about leak when you allocate num elements on string 169, which equals to numbers of slots in system, and then if sctx->num == 0, which is equals to number of slots with tokens, you just return without freeing.

It is a corner case not worth optimizing for IMHO. Especially because I have a plan, in the future, to keep slots around that do not have token present as we have an interest in supporting token insertion in the future.

I misread the comment, sorry, pushed another change that drops the early return in the freeing function.

@simo5 simo5 added covscan-ok Coverity scan passed covscan Triggers Coverity Scanner and removed covscan-ok Coverity scan passed labels Nov 7, 2024
@github-actions github-actions bot removed the covscan Triggers Coverity Scanner label Nov 7, 2024
@simo5 simo5 added the covscan-ok Coverity scan passed label Nov 7, 2024
@simo5 simo5 requested a review from Jakuje November 7, 2024 15:50
Copy link
Contributor

@Jakuje Jakuje left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@simo5 simo5 merged commit 1b42f26 into latchset:main Nov 7, 2024
52 checks passed
@simo5
Copy link
Member Author

simo5 commented Nov 7, 2024

ty!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
covscan-ok Coverity scan passed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Memory leak in slot.c
3 participants