Skip to content

feat/ Allow users to select if they want to cache or consume the value of the cypher key depending on their crypto module implementation#246

Open
Xavrax wants to merge 2 commits intomasterfrom
fix/copy-keys
Open

feat/ Allow users to select if they want to cache or consume the value of the cypher key depending on their crypto module implementation#246
Xavrax wants to merge 2 commits intomasterfrom
fix/copy-keys

Conversation

@Xavrax
Copy link
Copy Markdown
Contributor

@Xavrax Xavrax commented Apr 2, 2026

feat: Allow users to select if they want to cache or consume the value of the cypher key depending on their crypto module implementation

Allow users to select if they want to cache or consume the value of the cypher key depending on their crypto module implementation

Xavrax added 2 commits April 2, 2026 12:53
The C crypto module stores the cipher key as a raw pointer without
copying it. If the std::string passed to aes_cbc()/legacy() went out
of scope, the C module was left with a dangling pointer, causing
decryption failures.

Fix by having crypto_module own a heap-allocated copy of the key via
std::shared_ptr<std::string>, and transferring that ownership to the
pubnub::context through a new virtual cipher_key() method on the
into_crypto_provider_ptr interface.

Made-with: Cursor
Same fix as the C++ wrapper, but using Qt-idiomatic types:
QSharedPointer<QByteArray> instead of std::shared_ptr<std::string>.

The original Qt code had an even more severe bug where
cipher_key.toStdString().c_str() created an immediately dangling
pointer from a temporary.

Made-with: Cursor
@KGronek-Pubnub
Copy link
Copy Markdown
Contributor

I'm not sure is it still relevant for us to support C++ 98? Some concepts like std::move, are C++ 11, and they are in main wrapper file pubnub_common.hpp, so I guess it would mean - end of C++ 98 support? Maybe we should plan to update some comments/flags accordingly if so.

@Xavrax
Copy link
Copy Markdown
Contributor Author

Xavrax commented Apr 2, 2026

I'm not sure is it still relevant for us to support C++ 98? Some concepts like std::move, are C++ 11, and they are in main wrapper file pubnub_common.hpp, so I guess it would mean - end of C++ 98 support? Maybe we should plan to update some comments/flags accordingly if so.

Good catch. I forgot about the compatibility of other versions but the Crypto module itself is no longer C++98 compatible. Should I make it compatible again?
@KGronek-Pubnub
@parfeon

@parfeon
Copy link
Copy Markdown
Contributor

parfeon commented Apr 2, 2026

@Xavrax Do we actually need C++98 support? Event C99 currently exists mostly to support older toolchains for IoT devices but they already don't need it and can normally operate with latest standards.
@KGronek-Pubnub what do you think?

@KGronek-Pubnub
Copy link
Copy Markdown
Contributor

KGronek-Pubnub commented Apr 2, 2026

@Xavrax @parfeon I don't see any real problem with skipping C++ 98 support, just wanted to mention that if we do it, let's do it intentionally :)

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.

3 participants