Bug #13223: `File.join` will segv if File::SEPARATOR and File::Separator are set.

• Status: Closed
• Assignee: -
• Target version: -
• ruby -v: ruby 2.5.0dev (2017-02-17 trunk 57648) [x86_64-darwin16]
• Backport: 2.2: DONTNEED, 2.3: DONTNEED, 2.4: DONTNEED

[tenderlovemaking (Aaron Patterson)]

The program below will crash with a segv:

File.const_set :Separator, "hello"
File.const_set :SEPARATOR, "hello"

GC.start

File.join "hello", "world"

This is because the separator object is referenced from a global variable and setting the constants allows the object to be GC'd and the global will go bad.

I've attached a patch that contains a test to demonstrate the problem along with a fix.

[nobu (Nobuyoshi Nakada)]

• Description updated (diff)

I doubt that the tests are expected results.

And a similar pattern:

$; = " "
$a = nil
alias $; $a
alias $-F $a
GC.start
"".split

[tenderlovemaking (Aaron Patterson)]

@nobu (Nobuyoshi Nakada) I went by what the documentation says:

/*
 *  call-seq:
 *     File.join(string, ...)  ->  string
 *
 *  Returns a new string formed by joining the strings using
 *  <code>File::SEPARATOR</code>.
 *
 *     File.join("usr", "mail", "gumby")   #=> "usr/mail/gumby"
 *
 */

The current implementation doesn't seem to care what the value of File::SEPARATOR is, so I think it's a bug. Also JRuby has this behavior:

$ jruby -v -e'File.const_set(:SEPARATOR, "$"); p File.join("a","b")'
jruby 9.1.6.0 (2.3.1) 2016-11-09 0150a76 Java HotSpot(TM) 64-Bit Server VM 25.77-b03 on 1.8.0_77-b03 +jit [darwin-x86_64]
-e:1: warning: already initialized constant SEPARATOR
"a$b"

[nobu (Nobuyoshi Nakada)]

• Status changed from Open to Closed

Applied in changeset r57960.

---

file.c: join with /

• file.c (rb_file_join): join using "/" always, not a constant.
  and fix the document. [ruby-core:79579] [Bug #13223]

[nagachika (Tomoyuki Chikanaga)]

• Backport changed from 2.2: UNKNOWN, 2.3: UNKNOWN, 2.4: UNKNOWN to 2.2: REQUIRED, 2.3: REQUIRED, 2.4: REQUIRED

[nagachika (Tomoyuki Chikanaga)]

• Backport changed from 2.2: REQUIRED, 2.3: REQUIRED, 2.4: REQUIRED to 2.2: DONTNEED, 2.3: DONTNEED, 2.4: DONTNEED

Ugh, I have marked this as backport required for r57958, not r57960.
I'll create another ticket for r57958.

[shyouhei (Shyouhei Urabe)]

Nobu already made changes but, yes we looked at this issue in today's developer meeting.

File::SEPARATOR has been there, but not always used. For instance File.split does not honor that constant. JRuby is well-doing to carefully read our document but, we agreed that this is not somewhere we have to hustle.

Let us define File::SEPARATOR to be the default value of the file path separator; redefinition of it may or may not affect further invokations of methods, depending on implementations.

[Eregon (Benoit Daloze)]

shyouhei (Shyouhei Urabe) wrote:

Nobu already made changes but, yes we looked at this issue in today's developer meeting.

File::SEPARATOR has been there, but not always used. For instance File.split does not honor that constant. JRuby is well-doing to carefully read our document but, we agreed that this is not somewhere we have to hustle.

Let us define File::SEPARATOR to be the default value of the file path separator; redefinition of it may or may not affect further invokations of methods, depending on implementations.

It is much better to not leave semantics to be implementation-defined, as it's essentially the same as undefined behavior and C people know how bad that is.
So I think we should settle to only one of the two possibilities here.
Otherwise any program using File.join and modifying File::SEPARATOR will produce different results based on the Ruby implementation, which is inacceptable for such a method.

[shyouhei (Shyouhei Urabe)]

I'd personally like to vote for ignoring redefinitions of constants. In general redefinitions prevent optimizations. That doesn't make me happy.

[Eregon (Benoit Daloze)]

shyouhei (Shyouhei Urabe) wrote:

I'd personally like to vote for ignoring redefinitions of constants. In general redefinitions prevent optimizations. That doesn't make me happy.

I agree in that it is good to not change basic functionality such as File.join based on some global state.

Redefinition of constants is part of Ruby though.
With a JIT, the assumption that constants do not change can be embedded with the existence of the JIT'd code itself,
and deoptimize if the value change. Therefore redefinition of constants has no cost in compiled code, unless they are frequently changed.

Overall I think these decisions should not be based only on performance concerns,
especially in this case where the performance of File.join is likely insignificantly changed by looking up the constant.