Bug #7050

encoding of String#unpack for 'H', 'h', 'B' and 'b'

Added by Benoit Daloze over 2 years ago. Updated over 2 years ago.

[ruby-core:47653]
Status:Closed
Priority:Normal
Assignee:-
ruby -v:ruby 2.0.0dev (2012-07-15 trunk 36395) [x86_64-darwin10.8.0] Backport:

Description

Originally mentioned in #6799, I think the encoding of the resulting Strings for the 'H', 'h', 'B' and 'b' directives of String#unpack should be encoded in US-ASCII, because they will always contain 0-9a-f (0-1 for 'B','b') chars.

This is already done for the 'm', 'M' and 'u' directives in Array#pack.

It would also fix SecureRandom hexdigests as mentioned in #6799.

P.S.: Does anyone know the rationale behind which method to use for 'u','m','M' and 'H','h','B','b' ?
To have a base64 representation you need [s].pack('m') (and so #pack is used for 'u','m','M' to obtain the specified format), but for 'H','h','B','b' you need s.unpack(directive). I think the latter makes more sense when compared to other types such as 'l', in which #unpack is used to obtain the desired format.

0001-set-encoding-to-ASCII-for-appropriate-String-unpack-.patch Magnifier (2.74 KB) Benoit Daloze, 09/23/2012 05:03 AM

Associated revisions

Revision 37269
Added by Benoit Daloze over 2 years ago

set encoding to ASCII for appropriate String#unpack modifiers

  • pack.c (pack_unpack): set encoding of the 'H','h','B' and 'B' modifiers to US-ASCII.
  • test/ruby/test_pack.rb: tests for the above. [Bug #7050]
  • test/test_securerandom.rb: tests for SecureRandom.hex from tenderlove. [Bug #6799]

Revision 37269
Added by Benoit Daloze over 2 years ago

set encoding to ASCII for appropriate String#unpack modifiers

  • pack.c (pack_unpack): set encoding of the 'H','h','B' and 'B' modifiers to US-ASCII.
  • test/ruby/test_pack.rb: tests for the above. [Bug #7050]
  • test/test_securerandom.rb: tests for SecureRandom.hex from tenderlove. [Bug #6799]

History

#1 Updated by Benoit Daloze over 2 years ago

I would of course also add Aaron's test for SecureRandom hexdigests if this is fine to be merged.

#2 Updated by Aaron Patterson over 2 years ago

I would really like this to be added! (Thank you for writing the patch)

#3 Updated by Benoit Daloze over 2 years ago

Ping!

I would like to merge this.
If there are no objections, may I merge this in a week?

#4 Updated by Yui NARUSE over 2 years ago

Eregon (Benoit Daloze) wrote:

Ping!

I would like to merge this.
If there are no objections, may I merge this in a week?

OK, here you go

#5 Updated by Benoit Daloze over 2 years ago

  • Status changed from Open to Closed
  • % Done changed from 0 to 100

This issue was solved with changeset r37269.
Benoit, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.


set encoding to ASCII for appropriate String#unpack modifiers

  • pack.c (pack_unpack): set encoding of the 'H','h','B' and 'B' modifiers to US-ASCII.
  • test/ruby/test_pack.rb: tests for the above. [Bug #7050]
  • test/test_securerandom.rb: tests for SecureRandom.hex from tenderlove. [Bug #6799]

#6 Updated by Benoit Daloze over 2 years ago

On 19 October 2012 07:47, naruse (Yui NARUSE) naruse@airemix.jp wrote:

Eregon (Benoit Daloze) wrote:

Ping!

I would like to merge this.
If there are no objections, may I merge this in a week?

OK, here you go

Thank you!

Also available in: Atom PDF