Issue #19635 has been updated by wks (Kunshan Wang). nobu (Nobuyoshi Nakada) wrote in #note-1:
Applied in changeset commit: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, 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](https://www.mmtk.io/), 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 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` ```rb 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 ---------------------------------------- Bug #19635: errno may be overwritten by rb_sprintf if it triggers GC https://bugs.ruby-lang.org/issues/19635#change-103073 * Author: wks (Kunshan Wang) * Status: Closed * Priority: Normal * Backport: 3.0: UNKNOWN, 3.1: UNKNOWN, 3.2: UNKNOWN ---------------------------------------- Here is an excerpt in the `trap` function in `signal.c` ``` 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: ```c 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`. -- https://bugs.ruby-lang.org/