-
Notifications
You must be signed in to change notification settings - Fork 81
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix ClassCastException in X509Store.verify #114
Fix ClassCastException in X509Store.verify #114
Conversation
This commit changes the logic of the `X509StoreContext.initialize` method, to address some test failures that were showing up in `test_x509store.rb`. The failures would occur during calls to the `verify` method of `X509Store`. It seems that the failures were introduced by the addition of the `X509Cert.toJava` method in ecc1a05 . The `toJava` method seems to be implicitly called when iterating over a RubyArray in Java code, and the implementation of that method now returns a BC object rather than the expected JRuby `X509Cert` object. This commit changes the iteration to use the `.eltOk` method to access the underlying objects w/o the implicit call to `toJava`.
This is an attempt to address the issues described in #113 . @kares I'm not sure if this is the right approach, and I don't fully understand how With this change, I can run the entire MRI I think that, if this change is accepted, it would also be a good idea to fix the CI for |
If this doesn't seem like the best approach, and if someone who is more familiar than I am with this code isn't going to have time to look into this soon, I might be able to find a little more time to work on it next week if someone can give me some hints on a better implementation approach. Given that the |
This is excellent Chris, thank you! I mostly introduced the I haven't looked at the MRI test that identified this bug (somehow assumed its not that crucial since 9K passed fine) - would you consider copy-ing the test to our end to make sure we have a regression test? |
The test that failed is, I believe, directly copied from the MRI test suite, and it already exists in both the https://github.com/jruby/jruby-openssl/blob/master/src/test/ossl/1.9/test_x509store.rb#L36 But for whatever reason, this test is being run in CI for JRuby 1.7, and is not being run in CI for You can run it in the
|
unfortunately the src/test/ossl is not run - never finished the setup (needs working excludes) and we do rely on running the full JRuby suite before the final release push. anyways, I can handle things from here just fine. thanks for the effort! |
on master as b5bec3e |
This commit changes the logic of the
X509StoreContext.initialize
method, to address some test failures that were showing up in
test_x509store.rb
. The failures would occur during calls tothe
verify
method ofX509Store
.It seems that the failures were introduced by the addition of the
X509Cert.toJava
method in ecc1a05 .The
toJava
method seems to be implicitly called when iterating overa RubyArray in Java code, and the implementation of that method
now returns a BC object rather than the expected JRuby
X509Cert
object.
This commit changes the iteration to use the
.eltOk
method to accessthe underlying objects w/o the implicit call to
toJava
.