Fix for ASN1::Constructive 'each' implementation #96
Conversation
There was a problem hiding this comment.
@CBonnell @tidoublemy Oops, that commit was completely broken. Thank you for reporting and fixing!
| @@ -566,6 +566,19 @@ def test_decode_constructed_overread | |||
| assert_equal 17, ret[0][6] | |||
| end | |||
|
|
|||
| def test_constructive_each | |||
| data = ['test','data'] | |||
| seq = OpenSSL::ASN1::Sequence.new data | |||
There was a problem hiding this comment.
'data' should be an array of OpenSSL::ASN1::ASN1Data instances rather than of String.
There was a problem hiding this comment.
Whoops, stupid mistake on my part. Should be fixed now.
| def test_constructive_nonarray_each | ||
| seq = OpenSSL::ASN1::Sequence.new 5 | ||
|
|
||
| assert_raise(NoMethodError) { seq.each } |
There was a problem hiding this comment.
Thanks for being careful, but I think raising NoMethodError is an implementation detail and is not part of the spec that should be tested.
There was a problem hiding this comment.
I removed the NoMethodError exception type specification from assert_raise.
There was a problem hiding this comment.
It really hassles but we can't use assert_raise without specifying the exact exception type, because this file is shared with the ruby tree (https://github.com/ruby/ruby/blob/trunk/test/openssl/test_asn1.rb) where the test framework behaves differently. I think not testing is fine this time.
There was a problem hiding this comment.
Thanks for pointing that out. I was unaware of the code sharing with the Ruby tree. I removed this test in the latest commit.
|
@CBonnell Could you change target branch to 'maint'? This needs to be included in the next 2.0 release. |
|
@CBonnell Thank you! It looks like changing target branch rolled up irrelevant changes from master. Could you rebase on top of 'maint', and squash the three commits? |
|
Merged, thanks! |
Don't blindy assume that the value which can be modified from Ruby code is always an Array, and just call its #each method.
|
@rhenium, thanks for reviewing and merging this PR so quickly! |
This change fixes the
ASN1::Constructive.eachimplementation that was broken with commit 36bf7f4 (see @tidoublemy's comment on that commit for code to reproduce the issue).Included are two small tests to test for correct behavior of the
eachimplementation and that an appropriate exception will be raised (instead of SIGSEGV'ing) when non-array data is specified in Ruby for thevalueattribute.