Project

General

Profile

Actions

Bug #13223

closed

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

Added by tenderlovemaking (Aaron Patterson) almost 8 years ago. Updated over 7 years ago.

Status:
Closed
Assignee:
-
Target version:
-
ruby -v:
ruby 2.5.0dev (2017-02-17 trunk 57648) [x86_64-darwin16]
[ruby-core:79579]

Description

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.


Files

Updated by nobu (Nobuyoshi Nakada) almost 8 years ago

  • 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

Updated by tenderlovemaking (Aaron Patterson) almost 8 years ago

@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"
Actions #3

Updated by nobu (Nobuyoshi Nakada) over 7 years ago

  • 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]
Actions #4

Updated by nagachika (Tomoyuki Chikanaga) over 7 years ago

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

Updated by nagachika (Tomoyuki Chikanaga) over 7 years ago

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

Updated by shyouhei (Shyouhei Urabe) over 7 years ago

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.

Updated by Eregon (Benoit Daloze) over 7 years ago

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.

Actions #8

Updated by shyouhei (Shyouhei Urabe) over 7 years ago

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

Updated by Eregon (Benoit Daloze) over 7 years ago

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.

Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0