Bug #10453: NUM2CHR() does not perform additional bounds checks

• Status: Rejected
• Assignee: matz (Yukihiro Matsumoto)
• Target version: -
• ruby -v: ruby 2.1.4p265 (2014-10-27 revision 48166) [x86_64-linux]
• Backport: 2.0.0: UNKNOWN, 2.1: UNKNOWN

[silverhammermba (Max Anselm)]

NUM2CHR() just calls rb_num2int_inline() and masks off the high bytes. Consequently, passing any value larger than a char and no bigger than an int will return some garbage value (rather than raising RangeError).

To reproduce, compile and run:

#include <ruby.h>
#include <limits.h>

int main(int argc, char* argv[])
{
    ruby_init();
    
    VALUE y = INT2FIX(INT_MAX);
    char z = NUM2CHR(y);
    
    printf("%hhd\n", z);
    
    return ruby_cleanup(0);
}

Expected:
Segfault from uncaught RangeError.

Actual:
Prints -1

[nobu (Nobuyoshi Nakada)]

• Description updated (diff)
• Category set to core
• Status changed from Open to Assigned
• Assignee set to matz (Yukihiro Matsumoto)
• Target version set to 2.2.0

NUM2CHR rather should never raise RangeError for any arguments?

[silverhammermba (Max Anselm)]

I would expect it to raise RangeError if the num exceeds the size of a char. That is the behavior of the all of the other NUM2* macros.

[naruse (Yui NARUSE)]

• Target version deleted (2.2.0)

[jeremyevans0 (Jeremy Evans)]

• File num2chr-range-check-10453.patch num2chr-range-check-10453.patch added

Attached is a patch that will add a range check to NUM2CHR. However, it breaks a test:

1) Error:
TestStringIO#test_putc_nonascii:
RangeError: value to large to convert to char: 12356
    /home/jeremy/tmp/ruby/test/stringio/test_stringio.rb:567:in `putc'
    /home/jeremy/tmp/ruby/test/stringio/test_stringio.rb:567:in `test_putc_nonascii'

It is possibly, maybe even likely, that it will break third party C extensions as well (403 matches for NUM2CHR for Ruby on GitHub: https://github.com/search?l=Ruby&q=NUM2CHR&type=Code).

[nobu (Nobuyoshi Nakada)]

As CHR should stand for char type of C, so exceeding the limit of char will make confusion, I guess.

I'm curious for what purpose @silverhammermba (Max Anselm) needs the range check.
If it is to get a codepoint, I don't think extracting the first byte from a string argument reasonable.

[mame (Yusuke Endoh)]

• Related to Bug #15460: Behaviour of String#setbyte changed added

[mame (Yusuke Endoh)]

• Status changed from Assigned to Rejected

See #15460. String#setbyte has accepted not only 0..255 but also any integers. Once it had been limited only to 0..255, but it caused a compatibility issue, and eventually was reverted.

I don't think that this is a good idea in terms of compatibility, unless there is a special reason to make it strict.

I tentatively close this because there is no response from OP. If we have a response, I'd be happy to reopen.