Bug #18152: Fix theoretical bug with signals + qsort

• Status: Open
• Assignee: -
• Target version: -
• ruby -v: ruby 3.1.0dev (2021-09-06T18:23:33Z z102 b4d9126e43) [x86_64-linux]
• Backport: 2.6: UNKNOWN, 2.7: UNKNOWN, 3.0: UNKNOWN

[eggert (Paul Eggert)]

Ruby assumes that qsort is async-signal-safe, but POSIX does not guarantee this and it's not true of some qsort implementations, notably glibc. This is not a practical problem with glibc, since glibc qsort is async-signal-safe with small sorts and in practice Ruby's use of qsort is invariably small enough. However, it's better to be absolutely async-signal-safe, if only to pacify static checkers and the like.

I am attaching two alternative patches for the problem. Either will suffice. The first is simple and easier to audit, but does not scale well (though that is not important here). The second patch should scale, but is harder to audit.

It would be difficult to write test cases illustrating the bug that these patches fix, as they'd be timing dependent.

[nobu (Nobuyoshi Nakada)]

Can't qsort_r be considered async-signal-safe?
And qsort in the else needs the same patch too, I think.

[eggert (Paul Eggert)]

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

Can't qsort_r be considered async-signal-safe?

No. POSIX's list of async-signal-safe functions can be found here:

https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_04_03_03

(look for the list starting with _Exit). qsort_r is not on the list, which means portable code cannot assume that qsort_r is async-signal-safe. For example, Glibc's implementation of qsort_r is not async-signal-safe because it can call malloc.

And qsort in the else needs the same patch too, I think.

Although it wouldn't hurt for the else to have a similar patch, I didn't think it necessary because the else is not labeled async-signal-safe. The comment at the start of run_exec_dup2 says that function should be async-signal-safe when sargp is NULL, and since sargp is not NULL in the else part I thought it unnecessary to make changes to the else part. If the comment is incorrect I can submit a revised patch. (I have not reviewed the overall structure of Ruby for async-signal-safety; I am relying on its comments.)

[nobu (Nobuyoshi Nakada)]

Since qsort_r isn't a standard, POSIX would not include it.
Now we use it on some limited platforms, glibc, some BSDs, and Windows (no signals), I searched info if it's async-signal-safe or not on such platforms but vain.

As for else part, I've missed the comment and you are correct.

How do you think about another "hopefully" comment for bsearch?

[eggert (Paul Eggert)]

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

How do you think about another "hopefully" comment for bsearch?

Yes, a comment is needed for bsearch since POSIX doesn't say that bsearch is async-signal-safe. However, that call to bsearch already has a "hopefully" comment. (I don't know of any platforms where bsearch is not async-signal-safe.)

In practice, I expect qsort_r to be async-signal-safe on platforms where qsort is. However, as you say, portable code cannot assume that either function is async-signal-safe.