Project

General

Profile

Actions

Feature #18280

closed

Allow rb_utf8_str_new_cstr(NULL)

Added by ukolovda (Dmitry Ukolov) 11 months ago. Updated 11 months ago.

Status:
Feedback
Priority:
Normal
Assignee:
-
Target version:
-
[ruby-core:105882]

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) 11 months 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) 11 months 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 (Matthew Valentine-House) 11 months 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) 11 months 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) 11 months 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.

Actions #6

Updated by peterzhu2118 (Peter Zhu) 11 months ago

  • Status changed from Open to Feedback

Updated by dentarg (Patrik Ragnarsson) 11 months ago

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

Actions #8

Updated by jeremyevans0 (Jeremy Evans) 11 months 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) 11 months 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) 11 months ago

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

Updated by shyouhei (Shyouhei Urabe) 11 months ago

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.

Updated by shyouhei (Shyouhei Urabe) 11 months 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) 11 months 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) 11 months 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.

Actions

Also available in: Atom PDF