Bug #19635: errno may be overwritten by rb_sprintf if it triggers GC

• Status: Closed
• Assignee: -
• Target version: -
• Backport: 3.0: UNKNOWN, 3.1: UNKNOWN, 3.2: UNKNOWN

[wks (Kunshan Wang)]

Here is an excerpt in the trap function in signal.c

        oldfunc = ruby_signal(sig, func);
        if (oldfunc == SIG_ERR) rb_sys_fail_str(rb_signo2signm(sig));

ruby_signal tries to register a signal handling function. If it fails, it returns SIG_ERR, and errno is set to by sigaction to indicate the error.

However, the snippet above calls rb_signo2signm(sig) before calling rb_sys_fail_str. rb_signo2signm allocates a Ruby heap object using rb_sprintf. The problem is, if this rb_sprintf triggers GC, the garbage collector may perform may complex operations, including executing obj_free, calling mmap to allocate more memory from the OS, etc. They may overwrite the errno.

So if GC is triggered in the very unfortunate time, the caller of the trap function will receive an exception, but with the wrong reason. This may cause some tests, such as the test_trap_uncatchable_#{sig} test cases in test_signal.rb, to fail.

There are similar use cases in hash.c, for example:

        if (ret) rb_sys_fail_str(rb_sprintf("setenv(%s)", name));

The good practice is always loading from errno immediately after calling functions that may set errno, such as sigaction and setenv.

[nobu (Nobuyoshi Nakada)]

• Status changed from Open to Closed

Applied in changeset git|e3385f87831f036eaba96558cb4d83c8d5c43901.

---

[Bug #19635] Preserve errno

The following functions are turned into macros and no longer can be
used as expressions in core.

• rb_sys_fail
• rb_sys_fail_str
• rb_sys_fail_path

[wks (Kunshan Wang)]

nobu (Nobuyoshi Nakada) wrote in #note-1:

Applied in changeset git|e3385f87831f036eaba96558cb4d83c8d5c43901.

---

[Bug #19635] Preserve errno

The following functions are turned into macros and no longer can be
used as expressions in core.

• rb_sys_fail
• rb_sys_fail_str
• rb_sys_fail_path

@nobu (Nobuyoshi Nakada), thank you for your fix, but the bug related to the trap function in signal.c still exists.

gcc -E shows that when compiling signal.c, the rb_sys_fail_str comes from the declaration in include/ruby/internal/error.h instead of internal/error.h. From the source code of signal.c, it includes debug_counter.h (or any other Ruby headers) which includes ruby/ruby.h which includes ruby/internal/error.h

FYI, I am working on replacing CRuby's GC with MMTk, and the errno is quite often overwritten in this case in some configuration.

But there is an easy way to make it easily testable with vanilla CRuby. The following patch simply adds a statement to overwrite errno in rb_sprintf. Since rb_sprintf may trigger GC, it may still overwrite errno in normal execution anyway, so adding a manual errno to rb_sprintf` should not affect the correctness of the execution.

diff --git a/sprintf.c b/sprintf.c
index b2d89617aa..6d65bc20c2 100644
--- a/sprintf.c
+++ b/sprintf.c
@@ -1225,6 +1225,7 @@ rb_sprintf(const char *format, ...)
     result = rb_vsprintf(format, ap);
     va_end(ap);
 
+    errno = EEXIST;
     return result;
 }

You can test it by running

make test-all TESTS=./tests/ruby/test_signal.rb

or running the following program with miniruby

Signal.trap("KILL") do
  puts "Should not reach me"
end

I created a PR (https://github.com/ruby/ruby/pull/7812) that simply adds #include "internal/error.h" to signal.c. The problem disappeared with this change. But I still feel it is a bit confusing because the semantics of rb_sys_fail_str in signal.c now depends on which header file is included.

Kunshan Wang

[nobu (Nobuyoshi Nakada)]

Thank you, I missed it.
Please update the dependency in common.mk too.