[ruby-core:113487] [Ruby master Bug#19635] errno may be overwritten by rb_sprintf if it triggers GC

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/
 ______________________________________________
 ruby-core mailing list -- [e-mail]
 To unsubscribe send an email to [e-mail]
 ruby-core info -- https://ml.ruby-lang.org/mailman3/postorius/lists/ruby-core.ml.ruby-lang.org/