Bug #19635
closederrno may be overwritten by rb_sprintf if it triggers GC
Description
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
.
Updated by nobu (Nobuyoshi Nakada) over 1 year ago
- 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
Updated by wks (Kunshan Wang) over 1 year ago
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
Updated by nobu (Nobuyoshi Nakada) over 1 year ago
Thank you, I missed it.
Please update the dependency in common.mk too.