https://bugs.ruby-lang.org/https://bugs.ruby-lang.org/favicon.ico?17113305112021-11-01T10:44:22ZRuby Issue Tracking SystemRuby master - Feature #18280: Allow rb_utf8_str_new_cstr(NULL)https://bugs.ruby-lang.org/issues/18280?journal_id=944252021-11-01T10:44:22Zxtkoba (Tee KOBAYASHI)
<ul></ul><p>The SEGV reproduces for me with the following test case:</p>
<pre><code class="ruby syntaxhl" data-language="ruby"><span class="nb">require</span> <span class="s2">"fiddle"</span>
<span class="kp">include</span> <span class="no">Fiddle</span>
<span class="n">h</span> <span class="o">=</span> <span class="n">dlopen</span><span class="p">(</span><span class="kp">nil</span><span class="p">)</span>
<span class="n">f</span> <span class="o">=</span> <span class="no">Function</span><span class="p">.</span><span class="nf">new</span><span class="p">(</span><span class="n">h</span><span class="p">[</span><span class="s1">'rb_str_new_cstr'</span><span class="p">],</span> <span class="p">[</span><span class="no">TYPE_VOIDP</span><span class="p">],</span> <span class="no">TYPE_LONG</span><span class="p">)</span>
<span class="n">f</span><span class="p">.</span><span class="nf">call</span><span class="p">(</span><span class="no">NULL</span><span class="p">)</span>
</code></pre>
<p>Ruby version:</p>
<pre><code>$ ruby -v
ruby 3.1.0dev (2021-10-31T17:01:52Z master 266c90eaf9) [x86_64-linux]
</code></pre>
<p>It should be worth mentioning that for building ruby here I used Clang/LLVM 13.0.0 with optlevel <code>-Oz</code>.</p>
<p>Disassembly of the function <code>rb_str_new_cstr</code> implies that <code>must_not_null</code> is somehow optimized away:</p>
<pre><code>$ 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>
</code></pre> Ruby master - Feature #18280: Allow rb_utf8_str_new_cstr(NULL)https://bugs.ruby-lang.org/issues/18280?journal_id=944262021-11-01T12:41:07Zxtkoba (Tee KOBAYASHI)
<ul></ul><p><code>rb_str_new_cstr</code> seems to be attributed as <code>nonnull</code> since <a class="changeset" title="include/ruby/internal/intern/string.h: add doygen Must not be a bad idea to improve documents. [..." href="https://bugs.ruby-lang.org/projects/ruby-master/repository/git/revisions/091faca99ca92cb1146b3c4d8ebba67f4822561c">091faca99ca92cb1146b3c4d8ebba67f4822561c</a>. Thus it is absolutely legal for a compiler to optimize away the <code>must_not_null</code> check.</p> Ruby master - Feature #18280: Allow rb_utf8_str_new_cstr(NULL)https://bugs.ruby-lang.org/issues/18280?journal_id=944272021-11-01T12:58:04Zeightbitraptor (Matthew Valentine-House)matt@eightbitraptor.com
<ul></ul><p>Compiling with <code>-fno-delete-null-pointer-checks</code> prevents this check from being optimised away. Using the test script above and the following compile options</p>
<pre><code> 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
</code></pre>
<p>I can disassemble to see this:</p>
<pre><code>❯ 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>
</code></pre>
<p>I believe GCC enables this compile flag by default on most platforms. It doesn't look like clang does it by default.</p> Ruby master - Feature #18280: Allow rb_utf8_str_new_cstr(NULL)https://bugs.ruby-lang.org/issues/18280?journal_id=944282021-11-01T13:08:07Zukolovda (Dmitry Ukolov)
<ul></ul><p>Thank you!</p>
<p>I've got this error when test gem <code>addressable</code> + <code>idn-ruby</code>.<br>
See <a href="https://github.com/ukolovda/addressable/runs/4060648877?check_suite_focus=true" class="external">https://github.com/ukolovda/addressable/runs/4060648877?check_suite_focus=true</a><br>
I guess, it's CI use default settings for compiler.</p>
<p>I'm afraid, we got many errors with other gems, which use clang and give NULL parameters. This will have very bad results...</p> Ruby master - Feature #18280: Allow rb_utf8_str_new_cstr(NULL)https://bugs.ruby-lang.org/issues/18280?journal_id=944292021-11-01T13:10:05Zxtkoba (Tee KOBAYASHI)
<ul></ul><p>It is documented that the parameter for <code>rb_utf8_str_new_cstr</code> (and also for its friends) <a href="https://bugs.ruby-lang.org/projects/ruby-master/repository/git/revisions/091faca99ca92cb1146b3c4d8ebba67f4822561c/entry/include/ruby/internal/intern/string.h#L375" class="external">must not be a null pointer</a>. So this should not be considered as a bug but instead as a feature request that <code>rb_str_new_cstr</code> should allow <code>NULL</code> as its argument.</p> Ruby master - Feature #18280: Allow rb_utf8_str_new_cstr(NULL)https://bugs.ruby-lang.org/issues/18280?journal_id=944302021-11-01T13:39:18Zpeterzhu2118 (Peter Zhu)peter@peterzhu.ca
<ul><li><strong>Status</strong> changed from <i>Open</i> to <i>Feedback</i></li></ul> Ruby master - Feature #18280: Allow rb_utf8_str_new_cstr(NULL)https://bugs.ruby-lang.org/issues/18280?journal_id=944312021-11-01T15:04:09Zdentarg (Patrik Ragnarsson)patrik@starkast.net
<ul></ul><p>That documentation was recently added: <a href="https://github.com/ruby/ruby/pull/4815" class="external">https://github.com/ruby/ruby/pull/4815</a> (hasn't been part of a release yet AFAICT)</p> Ruby master - Feature #18280: Allow rb_utf8_str_new_cstr(NULL)https://bugs.ruby-lang.org/issues/18280?journal_id=944322021-11-01T15:21:04Zjeremyevans0 (Jeremy Evans)merch-redmine@jeremyevans.net
<ul><li><strong>Tracker</strong> changed from <i>Bug</i> to <i>Feature</i></li><li><strong>Subject</strong> changed from <i>Segmentation Fault in rb_utf8_str_new_cstr(NULL)</i> to <i>Allow rb_utf8_str_new_cstr(NULL)</i></li><li><strong>ruby -v</strong> deleted (<del><i>ruby 3.1.0dev (2021-10-31T09:27:55Z master 13a9597c7c) [x86_64-linux]</i></del>)</li><li><strong>Backport</strong> deleted (<del><i>2.6: UNKNOWN, 2.7: UNKNOWN, 3.0: UNKNOWN</i></del>)</li></ul> Ruby master - Feature #18280: Allow rb_utf8_str_new_cstr(NULL)https://bugs.ruby-lang.org/issues/18280?journal_id=944342021-11-01T16:02:31Znobu (Nobuyoshi Nakada)nobu@ruby-lang.org
<ul></ul><p>ukolovda (Dmitry Ukolov) wrote in <a href="#note-4">#note-4</a>:</p>
<blockquote>
<p>I'm afraid, we got many errors with other gems, which use clang and give NULL parameters. This will have very bad results...</p>
</blockquote>
<p>I guess the compiler warned that NULL may be passed, didn't it?</p> Ruby master - Feature #18280: Allow rb_utf8_str_new_cstr(NULL)https://bugs.ruby-lang.org/issues/18280?journal_id=944352021-11-01T17:14:28ZEregon (Benoit Daloze)
<ul></ul><p>IMHO the gem should be fixed so it doesn't call <code>rb_utf8_str_new_cstr()</code> with NULL.</p> Ruby master - Feature #18280: Allow rb_utf8_str_new_cstr(NULL)https://bugs.ruby-lang.org/issues/18280?journal_id=944372021-11-02T00:59:18Zshyouhei (Shyouhei Urabe)shyouhei@ruby-lang.org
<ul></ul><p>Hello, I'm the one who wrote the comment in string.h.</p>
<ul>
<li>OK to remove <code>__attribute__((__nonnull__))</code> to revive <code>must_not_null()</code>. Runtime check is a nice-to-have feature.</li>
<li>But that just raises ruby level exception instead of SEGV. Makes no sense either. The document itself must still be valid I believe.</li>
</ul> Ruby master - Feature #18280: Allow rb_utf8_str_new_cstr(NULL)https://bugs.ruby-lang.org/issues/18280?journal_id=944382021-11-02T02:05:36Zshyouhei (Shyouhei Urabe)shyouhei@ruby-lang.org
<ul></ul><p>Hmm, looking at the source code there are those functions that thoughtlessly pass their arguments to <code>strlen()</code> without checking anything (e.g. <code>rb_interned_str_cstr()</code>), and those which politely call <code>must_not_null</code> before dereferencing anything (e.g. <code>rb_utf8_str_new_cstr()</code>). It seems to me that our handling of null pointers are half-baked at best.</p>
<p>Maybe we want to allow or disallow null pointers consistently.</p> Ruby master - Feature #18280: Allow rb_utf8_str_new_cstr(NULL)https://bugs.ruby-lang.org/issues/18280?journal_id=946082021-11-11T08:30:10Zshyouhei (Shyouhei Urabe)shyouhei@ruby-lang.org
<ul></ul><p>Fix merged ATM to delete the attribute, for the sake of compatibility.</p>
<p>What to do with those other functions shall be discussed though.</p> Ruby master - Feature #18280: Allow rb_utf8_str_new_cstr(NULL)https://bugs.ruby-lang.org/issues/18280?journal_id=946122021-11-11T10:28:16Zukolovda (Dmitry Ukolov)
<ul></ul><p>shyouhei (Shyouhei Urabe) wrote in <a href="#note-13">#note-13</a>:</p>
<blockquote>
<p>Fix merged ATM to delete the attribute, for the sake of compatibility.</p>
<p>What to do with those other functions shall be discussed though.</p>
</blockquote>
<p>Thank you! I'm agree, compatibility is very important.</p>