Accelerate fetching attributes from an object handle - cleaned up version

[keldonin]

As suggested by @Jakuje, this is a cleaned up version of PR #322, with legacy code removed and benchmark example stripped off.

See PR #322 for discussions.

[Copilot]

Pull request overview

This PR optimizes the get_attributes() method to accelerate fetching attributes from PKCS#11 object handles. The optimization reduces the number of API calls by pre-allocating buffers for attributes with known fixed sizes and using a two-pass approach for variable-length attributes.

Key changes:

• Implemented a new two-pass attribute fetching strategy that pre-allocates buffers for fixed-size attributes
• Added fixed_size() method to AttributeType to identify attributes with known fixed sizes
• Added as_cptr! macro to safely handle empty vector pointer conversions
• Enhanced test utilities to support library behavior simulation and handle already-initialized errors

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File	Description
cryptoki/src/session/object_management.rs	Rewrote get_attributes() to use optimized two-pass fetching with pre-allocated buffers
cryptoki/src/object.rs	Added as_cptr! macro for safe pointer conversion and fixed_size() method for attribute type sizing
cryptoki/tests/basic.rs	Added comprehensive test get_attributes_test() covering multiple attribute retrieval scenarios
cryptoki/tests/common/mod.rs	Added test utility functions for library behavior simulation and improved initialization error handling

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Jakuje]

looks good! Squashing the commit history and fixup commits eventually would be good, but ok for now for more reviews.

[keldonin]

Sure, will do that once the review process is finished. BTW this can be conveniently performed at merge time, so maybe not needed from my end.

[wiktor-k]

Looks good 👌 the logic is a bit heavy but I'm happy that this improves the performance 👏

[wiktor-k]

🤔 What happens when someone adds a new AttributeType and forgets to add a branch here?

(That is I wonder if we should explicitly enumerate all None variants here or would that be a small concern...)

[Jakuje]

I think it will just go through the slow path as before -- first query the size from the token and then query the value. I agree though that having all of them enumerate would make it more nudging for the people adding new attributes to add them correctly here.

[wiktor-k]

Ack! Let's see what @keldonin thinks about it, I'm fine either way 👍

[hug-dev]

Nice that this is future-proof but agree that listing them all could be good!

[keldonin]

I think we can make this, no big deal. Removing the catch-all requires the user to adjust this method.

$ python3 -m this | grep Explicit   
Explicit is better than implicit.

[hug-dev]

Very good improvement, thank you!

[hug-dev]

Thank for also updating those 🙏

[hug-dev]

Great test, thanks!

[wiktor-k]

Thank you! 🙇

[keldonin]

hmmm, seems like I am facing some erratic errors during testing - and not always with the same test suite. I can't seem to reproduce these locally.

 ---- cryptoki/src/context/session_management.rs - context::session_management::Pkcs11::open_ro_session (line 47) stdout ----
Test executable failed (exit status: 101).

stderr:

thread 'main' (4188) panicked at cryptoki/src/context/session_management.rs:14:1:
error: cryptoki::error::Error - Function::Initialize: PKCS11 error: This value can only be returned by C_Login.  It indicates that the normal user’s PIN has not yet been initialized with C_InitPIN.
stack backtrace:
   0: __rustc::rust_begin_unwind
             at /builddir/build/BUILD/rust-1.92.0-build/rustc-1.92.0-src/library/std/src/panicking.rs:698:5
   1: core::panicking::panic_fmt
             at /builddir/build/BUILD/rust-1.92.0-build/rustc-1.92.0-src/library/core/src/panicking.rs:80:14
   2: <testresult::TestError as core::convert::From<T>>::from
   3: <core::result::Result<T,F> as core::ops::try_trait::FromResidual<core::result::Result<core::convert::Infallible,E>>>::from_residual
   4: rust_out::main
   5: core::ops::function::FnOnce::call_once
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

These issues seem unrelated to the change itself. Something you guys have already experienced?

[hug-dev]

Weird because I don't think C_Login is even called by this example test...
Because of recent changes, we added explicit session.close() and pkcs11.finalize() at the end of all tests. You could try out to add those in the example tests as well to see if it's related to some kind of race...

[Jakuje]

I think this is the test running the example from the following documentation block:

https://github.com/keldonin/rust-cryptoki/blob/accelerate_fetching_attributes_clean/cryptoki/src/context/session_management.rs#L48

This sounds quite odd, as the C_Initialize returns UserPinNotInitialized as it should not . That sounds like a bug in kryoptic. It looks like it returns the CKR_USER_PIN_NOT_INITIALIZED when it somehow fails to locate the PIN in database, which is not the right thing to do at this moment. Let me see if I can reproduce it reliably somehow.

[Jakuje]

This sounds like intermittent/timing issue. Rerun fixed this. But filled kryoptic issue to investigate it further: latchset/kryoptic#391