Feature #18280: Allow rb_utf8_str_new_cstr(NULL)

• Status: Feedback
• Assignee: -
• Target version: -

[ukolovda (Dmitry Ukolov)]

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

[xtkoba (Tee KOBAYASHI)]

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>

[xtkoba (Tee KOBAYASHI)]

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.

[eightbitraptor (Matthew Valentine-House)]

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.

[ukolovda (Dmitry Ukolov)]

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...

[xtkoba (Tee KOBAYASHI)]

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.

[peterzhu2118 (Peter Zhu)]

• Status changed from Open to Feedback

[dentarg (Patrik Ragnarsson)]

That documentation was recently added: https://github.com/ruby/ruby/pull/4815 (hasn't been part of a release yet AFAICT)

[jeremyevans0 (Jeremy Evans)]

• 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)

[nobu (Nobuyoshi Nakada)]

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?

[Eregon (Benoit Daloze)]

IMHO the gem should be fixed so it doesn't call rb_utf8_str_new_cstr() with NULL.

[shyouhei (Shyouhei Urabe)]

Hello, I'm the one who wrote the comment in string.h.

• OK to remove __attribute__((__nonnull__)) to revive must_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.

[shyouhei (Shyouhei Urabe)]

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.

[shyouhei (Shyouhei Urabe)]

Fix merged ATM to delete the attribute, for the sake of compatibility.

What to do with those other functions shall be discussed though.

[ukolovda (Dmitry Ukolov)]

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.