Skip to content
New issue

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

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

Already on GitHub? # to your account

Mark io target static variables as const to avoid race conditions #2557

Merged
merged 1 commit into from
Aug 20, 2024

Conversation

fhanau
Copy link
Collaborator

@fhanau fhanau commented Aug 20, 2024

Also cleans up remaining instances of uninitialized variables in that target. Some trivial cases of static variables elsewhere are also cleaned up.

  • In some cases using constexpr might be possible with little additional effort (in particular by making StringPtr constructors constexpr), but that was not the main objective here.

@fhanau fhanau requested a review from mikea August 20, 2024 01:45
@fhanau fhanau requested review from a team as code owners August 20, 2024 01:45
@fhanau fhanau requested a review from garrettgu10 August 20, 2024 01:45
@@ -90,7 +90,7 @@ kj::Array<kj::byte> AsymmetricKeyCryptoKeyImpl::exportKeyExt(
auto bio = OSSL_BIO_MEM();

struct EncDetail {
char* pass = &EMPTY_PASSPHRASE[0];
char* pass = const_cast<char*>(&EMPTY_PASSPHRASE[0]);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I haven't used const_cast before – let me know if this usage pattern is incorrect.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this case is ok since we don't expect anything to be written.

Also cleans up remaining instances of uninitialized variables in that
target.
@fhanau fhanau force-pushed the felix/static-const-io branch from a6951dc to c65a65a Compare August 20, 2024 01:57
@@ -51,13 +50,57 @@ class SqliteDatabase {
// expected.
operator sqlite3*() { return db; }

// Class which regulates a SQL query, especially to control how queries created in JavaScript
// application code are handled.
class Regulator {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The compiler insisted on Regulator being defined before the static constexpr Regulator variable, so I moved the subclass inside.

@fhanau fhanau merged commit bc35e3f into main Aug 20, 2024
9 checks passed
@fhanau fhanau deleted the felix/static-const-io branch August 20, 2024 14:12
};

InputGate(Hooks& hooks = Hooks::DEFAULT);
InputGate(Hooks& hooks = const_cast<Hooks&>(Hooks::DEFAULT));
Copy link
Member

Choose a reason for hiding this comment

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

Generally any const_cast should come with a comment saying why it's OK. In this case: "const_cast OK because the object has no members, it's just a vtable."

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

Successfully merging this pull request may close these issues.

5 participants