Project

General

Profile

Actions

Bug #18152

open

Fix theoretical bug with signals + qsort

Added by eggert (Paul Eggert) about 2 months ago. Updated about 2 months ago.

Status:
Open
Priority:
Normal
Assignee:
-
Target version:
-
ruby -v:
ruby 3.1.0dev (2021-09-06T18:23:33Z z102 b4d9126e43) [x86_64-linux]
[ruby-core:105164]

Description

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.


Files

0001-Fix-theoretical-bug-with-signals-qsort-b.patch (3.56 KB) 0001-Fix-theoretical-bug-with-signals-qsort-b.patch Scalable patch for theoretical qsort async-signal bug eggert (Paul Eggert), 09/06/2021 11:59 PM
0001-Fix-theoretical-bug-with-signals-qsort-a.patch (2.08 KB) 0001-Fix-theoretical-bug-with-signals-qsort-a.patch Simple, easy-to-audit patch for theoretical qsort async-signal bug eggert (Paul Eggert), 09/06/2021 11:59 PM

Updated by nobu (Nobuyoshi Nakada) about 2 months ago

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

Updated by eggert (Paul Eggert) about 2 months ago

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.)

Updated by nobu (Nobuyoshi Nakada) about 2 months ago

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?

Updated by eggert (Paul Eggert) about 2 months ago

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.

Actions

Also available in: Atom PDF