Feature #18280
closedAllow rb_utf8_str_new_cstr(NULL)
Description
Ruby process crushed.
`
Addressable::URI when parsed from 'http://example.com/%E8'
/home/ukolovda/RubymineProjects/external/addressable/lib/addressable/idna/native.rb:34: [BUG] Segmentation fault at 0x0000000000000000
ruby 3.1.0dev (2021-10-31T09:27:55Z master 13a9597c7c) [x86_64-linux]
-- Control frame information -----------------------------------------------
c:0043 p:---- s:0231 e:000230 CFUNC :nfkc_normalize
c:0042 p:0019 s:0226 e:000225 METHOD /home/ukolovda/RubymineProjects/external/addressable/lib/addressable/idna/native.rb:34
c:0041 p:0323 s:0221 e:000219 METHOD /home/ukolovda/RubymineProjects/external/addressable/lib/addressable/uri.rb:583
c:0040 p:0038 s:0210 e:000209 BLOCK /home/ukolovda/RubymineProjects/external/addressable/lib/addressable/uri.rb:1559 [FINISH]
...
-- Machine register context ------------------------------------------------
RIP: 0x00007feea7aa9f35 RBP: 0x0000000000000000 RSP: 0x00007ffc0e4ffdf8
RAX: 0x0000000000000000 RBX: 0x0000000055550083 RCX: 0x0000000000000000
RDX: 0x0000000000000000 RDI: 0x0000000000000000 RSI: 0x0000000004f72d81
R8: 0x0000000004f72d83 R9: 0x00007fee95642a68 R10: 0x0000000000000001
R11: 0x0000000000000000 R12: 0x00007fee96663dc0 R13: 0x0000000000000001
R14: 0x00007fee9a414590 R15: 0x0000000002ce4b20 EFL: 0x0000000000010283
-- C level backtrace information -------------------------------------------
/home/ukolovda/.rvm/rubies/ruby-head/lib/libruby.so.3.1(rb_print_backtrace+0x11) [0x7feea80838d5] vm_dump.c:759
/home/ukolovda/.rvm/rubies/ruby-head/lib/libruby.so.3.1(rb_vm_bugreport) vm_dump.c:1045
/home/ukolovda/.rvm/rubies/ruby-head/lib/libruby.so.3.1(rb_bug_for_fatal_signal+0xf0) [0x7feea7e88cb0] error.c:820
/home/ukolovda/.rvm/rubies/ruby-head/lib/libruby.so.3.1(sigsegv+0x49) [0x7feea7fd99b9] signal.c:964
/lib64/libpthread.so.0(__restore_rt+0x0) [0x7feea7d601b0]
/lib64/libc.so.6(__strlen_avx2+0x15) [0x7feea7aa9f35]
/home/ukolovda/.rvm/rubies/ruby-head/lib/libruby.so.3.1(rb_str_new_cstr+0x9) [0x7feea7ff4999] string.c:958
/home/ukolovda/.rvm/rubies/ruby-head/lib/libruby.so.3.1(rb_utf8_str_new_cstr+0x7) [0x7feea7ff49f7] string.c:972
/home/ukolovda/.rvm/gems/ruby-head/gems/idn-ruby-0.1.2/lib/idn.so(nfkc_normalize+0x4d) [0x7fee96668a5d] stringprep.c:159
/home/ukolovda/.rvm/rubies/ruby-head/lib/libruby.so.3.1(vm_cfp_consistent_p+0x0) [0x7feea80636a4] vm_insnhelper.c:3025
/home/ukolovda/.rvm/rubies/ruby-head/lib/libruby.so.3.1(vm_call_cfunc_with_frame) vm_insnhelper.c:3027
/home/ukolovda/.rvm/rubies/ruby-head/lib/libruby.so.3.1(vm_sendish+0x4e) [0x7feea80688e9] vm_insnhelper.c:4651
...
`
Lalest ruby version (rvm install ruby-head
)
In previous version it give exception:
ArgumentError: NULL pointer given
Updated by xtkoba (Tee KOBAYASHI) about 3 years ago
The SEGV reproduces for me with the following test case:
require "fiddle"
include Fiddle
h = dlopen(nil)
f = Function.new(h['rb_str_new_cstr'], [TYPE_VOIDP], TYPE_LONG)
f.call(NULL)
Ruby version:
$ ruby -v
ruby 3.1.0dev (2021-10-31T17:01:52Z master 266c90eaf9) [x86_64-linux]
It should be worth mentioning that for building ruby here I used Clang/LLVM 13.0.0 with optlevel -Oz
.
Disassembly of the function rb_str_new_cstr
implies that must_not_null
is somehow optimized away:
$ llvm-objdump --disassemble-symbols=rb_str_new_cstr libruby.so.3.1.0
libruby.so.3.1.0: file format elf64-x86-64
Disassembly of section .text:
000000000027d0ba <rb_str_new_cstr>:
27d0ba: 53 pushq %rbx
27d0bb: 48 89 fb movq %rdi, %rbx
27d0be: e8 6d d7 06 00 callq 0x2ea830 <strlen@plt>
27d0c3: 48 89 df movq %rbx, %rdi
27d0c6: 48 89 c6 movq %rax, %rsi
27d0c9: 5b popq %rbx
27d0ca: e9 51 da 06 00 jmp 0x2eab20 <rb_str_new@plt>
Updated by xtkoba (Tee KOBAYASHI) about 3 years ago
rb_str_new_cstr
seems to be attributed as nonnull
since 091faca99ca92cb1146b3c4d8ebba67f4822561c. Thus it is absolutely legal for a compiler to optimize away the must_not_null
check.
Updated by eightbitraptor (Matt V-H) about 3 years ago
Compiling with -fno-delete-null-pointer-checks
prevents this check from being optimised away. Using the test script above and the following compile options
set -lx debugflags '-g'
set -lx optflags '-Oz -fno-delete-null-pointer-checks'
set -lx RUBY_DEVEL 'yes'
./configure --prefix=$HOME/.rbenv/versions/main --disable-install-doc
I can disassemble to see this:
❯ llvm-objdump --disassemble-symbols=rb_str_new_cstr miniruby
miniruby: file format elf64-x86-64
Disassembly of section .text:
000000000017456d <rb_str_new_cstr>:
17456d: 53 pushq %rbx
17456e: 48 89 fb movq %rdi, %rbx
174571: e8 14 00 00 00 callq 0x17458a <must_not_null>
174576: 48 89 df movq %rbx, %rdi
174579: e8 f2 70 eb ff callq 0x2b670 <strlen@plt>
17457e: 48 89 df movq %rbx, %rdi
174581: 48 89 c6 movq %rax, %rsi
174584: 5b popq %rbx
174585: e9 09 fe ff ff jmp 0x174393 <rb_str_new>
I believe GCC enables this compile flag by default on most platforms. It doesn't look like clang does it by default.
Updated by ukolovda (Dmitry Ukolov) about 3 years ago
Thank you!
I've got this error when test gem addressable
+ idn-ruby
.
See https://github.com/ukolovda/addressable/runs/4060648877?check_suite_focus=true
I guess, it's CI use default settings for compiler.
I'm afraid, we got many errors with other gems, which use clang and give NULL parameters. This will have very bad results...
Updated by xtkoba (Tee KOBAYASHI) about 3 years ago
It is documented that the parameter for rb_utf8_str_new_cstr
(and also for its friends) must not be a null pointer. So this should not be considered as a bug but instead as a feature request that rb_str_new_cstr
should allow NULL
as its argument.
Updated by peterzhu2118 (Peter Zhu) about 3 years ago
- Status changed from Open to Feedback
Updated by dentarg (Patrik Ragnarsson) about 3 years ago
That documentation was recently added: https://github.com/ruby/ruby/pull/4815 (hasn't been part of a release yet AFAICT)
Updated by jeremyevans0 (Jeremy Evans) about 3 years ago
- Tracker changed from Bug to Feature
- Subject changed from Segmentation Fault in rb_utf8_str_new_cstr(NULL) to Allow rb_utf8_str_new_cstr(NULL)
- ruby -v deleted (
ruby 3.1.0dev (2021-10-31T09:27:55Z master 13a9597c7c) [x86_64-linux]) - Backport deleted (
2.6: UNKNOWN, 2.7: UNKNOWN, 3.0: UNKNOWN)
Updated by nobu (Nobuyoshi Nakada) about 3 years ago
ukolovda (Dmitry Ukolov) wrote in #note-4:
I'm afraid, we got many errors with other gems, which use clang and give NULL parameters. This will have very bad results...
I guess the compiler warned that NULL may be passed, didn't it?
Updated by Eregon (Benoit Daloze) about 3 years ago
IMHO the gem should be fixed so it doesn't call rb_utf8_str_new_cstr()
with NULL.
Updated by shyouhei (Shyouhei Urabe) about 3 years ago
Hello, I'm the one who wrote the comment in string.h.
- OK to remove
__attribute__((__nonnull__))
to revivemust_not_null()
. Runtime check is a nice-to-have feature. - But that just raises ruby level exception instead of SEGV. Makes no sense either. The document itself must still be valid I believe.
Updated by shyouhei (Shyouhei Urabe) about 3 years ago
Hmm, looking at the source code there are those functions that thoughtlessly pass their arguments to strlen()
without checking anything (e.g. rb_interned_str_cstr()
), and those which politely call must_not_null
before dereferencing anything (e.g. rb_utf8_str_new_cstr()
). It seems to me that our handling of null pointers are half-baked at best.
Maybe we want to allow or disallow null pointers consistently.
Updated by shyouhei (Shyouhei Urabe) almost 3 years ago
Fix merged ATM to delete the attribute, for the sake of compatibility.
What to do with those other functions shall be discussed though.
Updated by ukolovda (Dmitry Ukolov) almost 3 years ago
shyouhei (Shyouhei Urabe) wrote in #note-13:
Fix merged ATM to delete the attribute, for the sake of compatibility.
What to do with those other functions shall be discussed though.
Thank you! I'm agree, compatibility is very important.