Project

General

Profile

Actions

Bug #15635

closed

Inconsistent handling of dummy encodings and code range

Added by nirvdrum (Kevin Menard) about 2 years ago. Updated about 2 years ago.

Status:
Closed
Priority:
Normal
Assignee:
-
Target version:
-
ruby -v:
ruby 2.6.1p33 (2019-01-30 revision 66950) [x86_64-linux]
[ruby-core:91662]

Description

It's hard to write code that works properly with dummy encodings, so they should really be avoided altogether. However, I've come across a code path that I think yields inconsistent results when it comes to dummy encodings with a minimum character length > 1 (i.e., "UTF-16" and "UTF-32"). To illustrate the issue, run the following program:

s = "abc"
puts s.encoding
puts "Dummy: #{s.encoding.dummy?}"
puts "Valid: #{s.valid_encoding?}"
puts s.bytes.inspect
puts

s.encode!("UTF-32")
puts s.encoding
puts "Dummy: #{s.encoding.dummy?}"
puts "Valid: #{s.valid_encoding?}"
puts s.bytes.inspect
puts

s.force_encoding("UTF-32")
puts s.encoding
puts "Dummy: #{s.encoding.dummy?}"
puts "Valid: #{s.valid_encoding?}"
puts s.bytes.inspect

The output on Ruby 2.6.1p33 for me is:

UTF-8
Dummy: false
Valid: true
[97, 98, 99]

UTF-32
Dummy: true
Valid: true
[0, 0, 254, 255, 0, 0, 0, 97, 0, 0, 0, 98, 0, 0, 0, 99]

UTF-32
Dummy: true
Valid: false
[0, 0, 254, 255, 0, 0, 0, 97, 0, 0, 0, 98, 0, 0, 0, 99]

Basically, we start with a UTF-8 string and convert it to UTF-32. Without an explicit indication of endianness, the encoding is considered dummy, but internally big endian is used (i.e., UTF-32BE). The new byte pattern for the successfully encoded string is shown. After calling force_encoding on the string with the same encoding, suddenly the string is no longer considered valid. I think many people would expect force_encoding using the string's current encoding to be a no-op. Even setting that aside, we can see the byte sequence and the encoding for the string didn't change, but its validity has. I believe this is wrong.

The problematic lines are https://github.com/ruby/ruby/blob/e6d1c72bec5c6544e9ae82501a6cdd7460222096/string.c#L660-L662 from rb_enc_str_coderange:

if (rb_enc_mbminlen(enc) > 1 && rb_enc_dummy_p(enc)) {
        cr = ENC_CODERANGE_BROKEN;
}

This unconditionally sets the code range to CR_BROKEN, but only for dummy encodings with a minimum length > 1. I think this is designed to specifically target UTF-16 and UTF-32, while leaving UTF-7 alone.

It may be the case that the correct behavior here is to always mark the string as invalid. After all, the dummy encodings could change endianness based on platform (although, I don't think they ever do). If that's the case, then the issue is with the code path from String#encode.

The inconsistency presents both correctness and performance issues. The former is because end user code may use a method like String#valid_encoding? for branching decisions. The latter because the runtime generally takes a slower path for operations on CR_BROKEN strings.

Updated by nirvdrum (Kevin Menard) about 2 years ago

I also tested some older Ruby releases. The issue is also present in ruby 2.4.4p296 (2018-03-28 revision 63013) [x86_64-linux] and ruby 2.5.1p57 (2018-03-29 revision 63029) [x86_64-linux].

Actions #2

Updated by nobu (Nobuyoshi Nakada) about 2 years ago

  • Status changed from Open to Closed

Applied in changeset trunk|r67167.


string.c: respect the actual encoding

  • string.c (rb_enc_str_coderange): respect the actual encoding of if a BOM presents, and scan for the actual code range. [ruby-core:91662] [Bug #15635]
Actions #3

Updated by nagachika (Tomoyuki Chikanaga) about 2 years ago

  • Backport changed from 2.4: UNKNOWN, 2.5: UNKNOWN, 2.6: UNKNOWN to 2.4: REQUIRED, 2.5: REQUIRED, 2.6: REQUIRED

Updated by naruse (Yui NARUSE) about 2 years ago

  • Backport changed from 2.4: REQUIRED, 2.5: REQUIRED, 2.6: REQUIRED to 2.4: REQUIRED, 2.5: REQUIRED, 2.6: DONE

ruby_2_6 r67181 merged revision(s) 67167.

Updated by nagachika (Tomoyuki Chikanaga) about 2 years ago

  • Backport changed from 2.4: REQUIRED, 2.5: REQUIRED, 2.6: DONE to 2.4: REQUIRED, 2.5: DONE, 2.6: DONE

ruby_2_5 r67192 merged revision(s) 67167.

Actions

Also available in: Atom PDF