rewriting most of the asn1 init code in ruby #740

HoneyryderChuck · 2024-04-16T17:11:31Z

This is an initial proposal towards the goal of having as much of the lib in ruby as possible. While most of the code must be in C, some of the code in C is essentially calling back to ruby, and writing "ruby in C", if that makes any sense.

Lemme know what you think. I understand there may also be some cognitive load from jumping from C to ruby and back, so there's certainly a trade-off.

lib/openssl/asn1.rb

ext/openssl/ossl_asn1.c

rhenium · 2024-04-26T17:49:53Z

I'm all for it!

Apart from methods for accessing OpenSSL's OID registry (OpenSSL::ASN1::ObjectID.register, #sn, #ln, and #oid), OpenSSL::ASN1 could have been entirely written in Ruby, and that would definitely make maintenance easier.

It would be very nice if we can port the BER decoding code to Ruby.

HoneyryderChuck · 2024-05-02T08:34:38Z

@rhenium thx for the feedback!

Some follow-up where I need your input as well:

If you see the ruby code, you'll find the initialize method copy-pasted both for Primitive and Constructive, which is a direct port from the deleted C code, where both classes were sharing the same underlying impl in C. I'd like to address, and I can see the following options:

Create base class for both, inheriting from ASN1Data, which implements common initialize;
Create module with initialize, inject on both classes;
Create __initialize_with.. method in ASN1Data class, call for both on initialize;

Essentially, the first 2 options will require a potential breaking(?) change in the ancestor chain. Not sure how you feel about that. The third option doesn't do that, at the cost of adding an additional "hidden" method. All of the above will add smth new to document (or maybe we can :nodoc: all of them?). Which one do you think suits this the best?

It would be very nice if we can port the BER decoding code to Ruby.

You mean OpenSSL::ASN1.decode? I can try, but AFAIR there was some dependency on some openssl function calls, so don't know how feasible will that be.

HoneyryderChuck · 2024-05-17T14:06:00Z

@rhenium ping for feedback.

I also don't think it's possible to rewrite the decoding in ruby (or I may not have understood your suggestion).

lib/openssl/asn1.rb

rhenium · 2024-05-08T10:33:52Z

ext/openssl/ossl_asn1.c

+        asn1data = rb_funcall(cASN1EndOfContent, rb_intern("new"), 0);
+    else {
+	    VALUE args[4] = { value, INT2NUM(tag), Qnil, tc };
+        asn1data = rb_funcall3(klass, rb_intern("new"), 4, args);


The indentation seems broken throughout the diff. Please set the (hard)tab size to 8 spaces and the indent size to 4 spaces in your editor (https://github.com/ruby/ruby/blob/master/.editorconfig)

ossl_asn1.c is an old file and uses the old style of mixed tabs and spaces, but you can ignore it and use 4 spaces for indentation.

This is not fixed yet. A hard tab has an equal width to 8 spaces.

ext/openssl/ossl_asn1.c

lib/openssl/asn1.rb

rhenium · 2024-06-08T15:45:16Z

I'm sorry for the delay.

If you see the ruby code, you'll find the initialize method copy-pasted both for Primitive and Constructive, which is a direct port from the deleted C code, where both classes were sharing the same underlying impl in C. I'd like to address, and I can see the following options:
* Create base class for both, inheriting from `ASN1Data`, which implements common `initialize`;

* Create module with `initialize`, inject on both classes;

* Create `__initialize_with..` method in `ASN1Data` class, call for both on `initialize`;
Essentially, the first 2 options will require a potential breaking(?) change in the ancestor chain. Not sure how you feel about that. The third option doesn't do that, at the cost of adding an additional "hidden" method. All of the above will add smth new to document (or maybe we can :nodoc: all of them?). Which one do you think suits this the best?

Adding a class or module in the ancestor chain is normally a compatible change. Private/hidden method also seems fine. At the same time, the current diff doesn't seem to contain too much amount of code duplication to me. Please use whatever you're comfortable with.

rhenium · 2024-06-08T15:55:42Z

It would be very nice if we can port the BER decoding code to Ruby.

You mean OpenSSL::ASN1.decode? I can try, but AFAIR there was some dependency on some openssl function calls, so don't know how feasible will that be.

It uses very low-level OpenSSL functions such as ASN1_get_object() or d2i_ASN1_INTEGER() which I hope aren't too difficult to replace. However this is a completely separate topic and not a blocker for this PR>

HoneyryderChuck · 2024-06-11T17:45:36Z

@rhenium done.

lib/openssl/asn1.rb

ext/openssl/ossl_asn1.c

lib/openssl/asn1.rb

rhenium · 2024-06-12T11:49:25Z

GitHub doesn't let me add comments to the collapsed part of the diff, but we can cleanup some definitions in the C file:

ossl_asn1_set_*() macros except for ossl_asn1_set_indefinite_length() are no longer used and should be removed.
id_each can also be removed.

rhenium · 2024-06-12T11:51:12Z

Run bundle exec rake compile
rake aborted!
NameError: undefined method `indefinite_length=' for class `OpenSSL::ASN1::Primitive'
/home/runner/work/openssl/openssl/lib/openssl/asn1.rb:128:in `undef_method'
/home/runner/work/openssl/openssl/lib/openssl/asn1.rb:128:in `<class:Primitive>'
/home/runner/work/openssl/openssl/lib/openssl/asn1.rb:125:in `<module:ASN1>'
/home/runner/work/openssl/openssl/lib/openssl/asn1.rb:12:in `<module:OpenSSL>'
/home/runner/work/openssl/openssl/lib/openssl/asn1.rb:11:in `<top (required)>'
/home/runner/work/openssl/openssl/lib/openssl.rb:16:in `require_relative'
/home/runner/work/openssl/openssl/lib/openssl.rb:16:in `<top (required)>'
/home/runner/work/openssl/openssl/vendor/bundle/ruby/2.7.0/gems/rake-compiler-1.2.7/lib/rake/extensiontask.rb:5:in `require'
/home/runner/work/openssl/openssl/vendor/bundle/ruby/2.7.0/gems/rake-compiler-1.2.7/lib/rake/extensiontask.rb:5:in `<top (required)>'
/home/runner/work/openssl/openssl/Rakefile:6:in `require'
/home/runner/work/openssl/openssl/Rakefile:6:in `<top (required)>'
/home/runner/work/openssl/openssl/vendor/bundle/ruby/2.7.0/gems/rake-13.2.1/exe/rake:27:in `<top (required)>'
/opt/hostedtoolcache/Ruby/2.7.8/x64/bin/bundle:23:in `load'
/opt/hostedtoolcache/Ruby/2.7.8/x64/bin/bundle:23:in `<main>'
(See full trace by running task with --trace)
Error: Process completed with exit code 1.

GitHub Actions is failing with this error with Ruby 2.7, but I'm not sure why this occurs. I can't reproduce it locally.

HoneyryderChuck · 2024-06-12T17:36:07Z

GitHub Actions is failing with this error with Ruby 2.7, but I'm not sure why this occurs.

This was a bug, which was fixed in ruby 3: attr_accessor does not register #a and #a= as instance methods, so undef_method call does not find them.

I've resorted to not undeffing them in ruby 2.7 . WDYT? I'm assuming you'll be dropping 2.7 one of these days, and that's less work than figuring out a more invasive patch than this one, but I don't know if there are other implications.

(there is a test asserting this accessor, which I'll have to edit if you agree).

rhenium · 2024-06-13T10:29:43Z

I haven't investigated why it raises an exception in Ruby 2.7 only, but the root cause of this particular error in bundle exec rake compile is that it's loading the Ruby part from local lib/ and the C part from Ruby's default gem. This was happening with all Ruby versions. #768 should fix it.

Please rebase on top of master. It should now work with 2.7 too.

rhenium

Please check the indentation settings in the C file.

Apart from these style issues, this looks good to me!

rhenium · 2024-06-13T10:42:04Z

ext/openssl/ossl_asn1.c

+	}
+	else {
+		VALUE args[3] = { value, INT2NUM(tag), tc };
+		asn1data = rb_funcall3(cASN1Data, rb_intern("new"), 3, args);


Please use rb_funcallv_public() everywhere.

Could you update other rb_funcall3() usage too?

lib/openssl/asn1.rb

HoneyryderChuck · 2024-06-13T17:52:01Z

Done.

Please check the indentation settings in the C file.

I'm struggling a bit with this one. I've changed them back to tabs, as most of the rest of the file is. Isn't it correct?

rhenium · 2024-06-14T05:34:01Z

Please check the indentation settings in the C file.

I'm struggling a bit with this one. I've changed them back to tabs, as most of the rest of the file is. Isn't it correct?

Please use 4 spaces for indentation. You can see the changed lines currently have an inconsistent indentation here: https://github.com/ruby/openssl/blob/bbacdfa19370d98212ecf2aae0eb7a9b544966e2/ext/openssl/ossl_asn1.c

Historically 8 consecutive spaces of indentation was replaced by a hard tab, which is why this file contains hard tabs, but this step is not needed anymore.

HoneyryderChuck · 2024-06-14T10:08:54Z

@rhenium hopefully it's better now.

ext/openssl/ossl_asn1.c

rhenium · 2024-06-17T08:03:41Z

@rhenium hopefully it's better now.

Yes, it looks good! Thanks.

to have as much of the lib in ruby as possible

rhenium · 2024-06-27T16:01:44Z

Merged now. Thank you!

junaruga reviewed Apr 16, 2024

View reviewed changes

lib/openssl/asn1.rb Outdated Show resolved Hide resolved

junaruga reviewed Apr 16, 2024

View reviewed changes

ext/openssl/ossl_asn1.c Outdated Show resolved Hide resolved

HoneyryderChuck force-pushed the asn1-in-ruby branch 2 times, most recently from ca0ee61 to 655d144 Compare April 17, 2024 14:48

HoneyryderChuck force-pushed the asn1-in-ruby branch from 655d144 to 4bc0523 Compare May 17, 2024 14:07

rhenium reviewed Jun 8, 2024

View reviewed changes

HoneyryderChuck force-pushed the asn1-in-ruby branch 2 times, most recently from 4dc17e6 to 20e21d4 Compare June 11, 2024 17:44

rhenium reviewed Jun 12, 2024

View reviewed changes

lib/openssl/asn1.rb Outdated Show resolved Hide resolved

ext/openssl/ossl_asn1.c Outdated Show resolved Hide resolved

ext/openssl/ossl_asn1.c Outdated Show resolved Hide resolved

lib/openssl/asn1.rb Show resolved Hide resolved

HoneyryderChuck force-pushed the asn1-in-ruby branch 3 times, most recently from 801cbf1 to 33ba02c Compare June 12, 2024 17:34

rhenium requested changes Jun 13, 2024

View reviewed changes

HoneyryderChuck force-pushed the asn1-in-ruby branch from 33ba02c to bbacdfa Compare June 13, 2024 17:50

HoneyryderChuck force-pushed the asn1-in-ruby branch 2 times, most recently from acdb64f to b83c64e Compare June 14, 2024 10:07

rhenium requested changes Jun 17, 2024

View reviewed changes

ext/openssl/ossl_asn1.c Outdated Show resolved Hide resolved

rewriting most of the asn1 init code in ruby

8305051

to have as much of the lib in ruby as possible

HoneyryderChuck force-pushed the asn1-in-ruby branch from b83c64e to 8305051 Compare June 17, 2024 10:48

rhenium approved these changes Jun 27, 2024

View reviewed changes

rhenium merged commit bb45527 into ruby:master Jun 27, 2024
54 checks passed

HoneyryderChuck deleted the asn1-in-ruby branch June 27, 2024 17:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rewriting most of the asn1 init code in ruby #740

rewriting most of the asn1 init code in ruby #740

HoneyryderChuck commented Apr 16, 2024 •

edited

Loading

rhenium commented Apr 26, 2024

HoneyryderChuck commented May 2, 2024

HoneyryderChuck commented May 17, 2024

rhenium May 8, 2024

rhenium Jun 12, 2024

rhenium commented Jun 8, 2024

rhenium commented Jun 8, 2024

HoneyryderChuck commented Jun 11, 2024

rhenium commented Jun 12, 2024

rhenium commented Jun 12, 2024

HoneyryderChuck commented Jun 12, 2024 •

edited

Loading

rhenium commented Jun 13, 2024 •

edited

Loading

rhenium left a comment

rhenium Jun 13, 2024

rhenium Jun 14, 2024

HoneyryderChuck commented Jun 13, 2024

rhenium commented Jun 14, 2024

HoneyryderChuck commented Jun 14, 2024

rhenium commented Jun 17, 2024

rhenium commented Jun 27, 2024

rewriting most of the asn1 init code in ruby #740

rewriting most of the asn1 init code in ruby #740

Conversation

HoneyryderChuck commented Apr 16, 2024 • edited Loading

rhenium commented Apr 26, 2024

HoneyryderChuck commented May 2, 2024

HoneyryderChuck commented May 17, 2024

rhenium May 8, 2024

Choose a reason for hiding this comment

rhenium Jun 12, 2024

Choose a reason for hiding this comment

rhenium commented Jun 8, 2024

rhenium commented Jun 8, 2024

HoneyryderChuck commented Jun 11, 2024

rhenium commented Jun 12, 2024

rhenium commented Jun 12, 2024

HoneyryderChuck commented Jun 12, 2024 • edited Loading

rhenium commented Jun 13, 2024 • edited Loading

rhenium left a comment

Choose a reason for hiding this comment

rhenium Jun 13, 2024

Choose a reason for hiding this comment

rhenium Jun 14, 2024

Choose a reason for hiding this comment

HoneyryderChuck commented Jun 13, 2024

rhenium commented Jun 14, 2024

HoneyryderChuck commented Jun 14, 2024

rhenium commented Jun 17, 2024

rhenium commented Jun 27, 2024

HoneyryderChuck commented Apr 16, 2024 •

edited

Loading

HoneyryderChuck commented Jun 12, 2024 •

edited

Loading

rhenium commented Jun 13, 2024 •

edited

Loading