Skip to content

Fix #559: Skip caching key with zero object handle #560

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bukka
Copy link
Contributor

@bukka bukka commented Apr 23, 2025

This is a solution for #559 .

It might happen that the object with zero object handle gets to cache_key which then result in unnecessary failure when caching the key. This skips the key caching if the object handle is 0.

Interestingly after this the handshake became much quicker so it might have some impact on performance as well. I can't say it for sure if it impacts production as I use everything with debug build but it should save some session logins for sure.

Checklist

  • Test suite updated with functionality tests
  • Test suite updated with negative tests

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

It might happen that the object with zero object handle gets to
cache_key which then result in unnecessary failure when caching
the key. This skips the key caching if the object handle is 0.
@bukka
Copy link
Contributor Author

bukka commented Apr 23, 2025

This will probably need some sort of test so it's just a draft for now.

@bukka bukka changed the title Skip caching key with zero object handle Fix #559: Skip caching key with zero object handle Apr 23, 2025
Copy link
Member

@simo5 simo5 left a comment

Choose a reason for hiding this comment

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

Minor nits, but otherwise LGTM!

@@ -388,6 +389,12 @@ static void cache_key(P11PROV_OBJ *obj)
return;
}

obj_handle = p11prov_obj_get_handle(obj);
if (obj_handle == 0) {
Copy link
Member

Choose a reason for hiding this comment

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

please us CK_INVALID_HANDLE here.

@@ -388,6 +389,12 @@ static void cache_key(P11PROV_OBJ *obj)
return;
}

obj_handle = p11prov_obj_get_handle(obj);
if (obj_handle == 0) {
P11PROV_debug("Skip caching key with zero object handle");
Copy link
Member

Choose a reason for hiding this comment

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

zero -> invalid

@simo5
Copy link
Member

simo5 commented Apr 23, 2025

Do you have a way to add a test that exercises this code path ?

@bukka
Copy link
Contributor Author

bukka commented Apr 23, 2025

Do you have a way to add a test that exercises this code path ?

I might be able to get it in some integration test with nginx. This is hopefully the last issue so once I get it all working, I would like to start looking into getting the fixes upstream which means adding the tests for the current patches where possible so will try then.

@bukka
Copy link
Contributor Author

bukka commented Apr 23, 2025

Interestingly after this change, the skip is not even happening. I have done some experiments and just purely getting the handle before the p11prov_take_login_session session login results in actual handle being returned so it seems there is some reset going on during the session login. Will need to check it out properly. This also speed up the whole thing. I also checked that CopyObject is happening and it's successful so it looks like some flow thing possibly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants