Skip to content

ossl config: shareable when frozen#809

Merged
rhenium merged 3 commits into
ruby:masterfrom
HoneyryderChuck:issue-521-3
Dec 7, 2024
Merged

ossl config: shareable when frozen#809
rhenium merged 3 commits into
ruby:masterfrom
HoneyryderChuck:issue-521-3

Conversation

@HoneyryderChuck

Copy link
Copy Markdown
Contributor

should probably be initialized as frozen, given no state modifications

@rhenium rhenium left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

NCONF is not a reference counted object, and is used in only one location outside ossl_config.c, in ossl_x509ext.c. It seems read-only.

This looks good to merge, once we've defined RUBY_TYPED_FROZEN_SHAREABLE on older Ruby versions (#808).

@HoneyryderChuck

Copy link
Copy Markdown
Contributor Author

@rhenium done

@rhenium

rhenium commented Nov 11, 2024

Copy link
Copy Markdown
Member

Could you add a test case for this class, too?

@HoneyryderChuck HoneyryderChuck force-pushed the issue-521-3 branch 2 times, most recently from fd3b246 to 33be1ef Compare November 12, 2024 13:51
@HoneyryderChuck

Copy link
Copy Markdown
Contributor Author

@rhenium added test.

FWIW I couldn't make the config frozen when initialized, as that interferes with .dup, which should return an unfrozen copy. Not sure whether it's worth it to force this though, as it seems to be a breaking change, and I have no particular use-case for it either.

@rhenium

rhenium commented Nov 13, 2024

Copy link
Copy Markdown
Member

FWIW I couldn't make the config frozen when initialized, as that interferes with .dup, which should return an unfrozen copy. Not sure whether it's worth it to force this though, as it seems to be a breaking change, and I have no particular use-case for it either.

I think it's acceptable that a #dup returns a frozen object since it is what some core classes like Interger do in Ruby >= 2.4.

I think making OpenSSL::Config frozen by default makes sense. At the same time, I think this class is mostly only useful when working with OpenSSL::X509::ExtensionFactory, and it wouldn't be an interesting object to share between ractors. Either way, such a change should be out of scope of this PR.

Comment thread ext/openssl/ossl_config.c
@HoneyryderChuck

Copy link
Copy Markdown
Contributor Author

managed to make it frozen when initialized. turns out that .parse wasn't calling config_initialize as I thought, and that got me blindsided.

@rhenium

rhenium commented Nov 22, 2024

Copy link
Copy Markdown
Member

It seems strange to me that Config.new returns a frozen object and #dup returns a non-frozen object. Shouldn't the latter return a frozen object, too?

I think making it shareable between ractors and making it frozen by default are two separate changes even if both make sense. Can you split them into two commits?

@HoneyryderChuck

Copy link
Copy Markdown
Contributor Author

Indeed, .dup should return a frozen config, fixed.

Can you split them into two commits?

done.

@rhenium

rhenium commented Dec 7, 2024

Copy link
Copy Markdown
Member

Fixed some style issues in tests.

@rhenium

rhenium commented Dec 7, 2024

Copy link
Copy Markdown
Member

I've extracted the change that made OpenSSL::Config::DEFAULT_CONFIG_FILE frozen into a separate commit.

@rhenium rhenium merged commit 3f7b722 into ruby:master Dec 7, 2024
@HoneyryderChuck HoneyryderChuck deleted the issue-521-3 branch December 7, 2024 08:24
@HoneyryderChuck HoneyryderChuck restored the issue-521-3 branch December 7, 2024 08:24
@HoneyryderChuck HoneyryderChuck deleted the issue-521-3 branch December 7, 2024 08:24
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