Let signals interrupt fgets unless SA_RESTART set #1152

cloudrac3r · 2024-04-23T02:35:28Z

See investigation in #1130.

fgets internally calls readv. readv is a @restartable function that understands SA_RESTART. If SA_RESTART is set, readv already handles restarting the system call and eventually the string is transparently returned to the fgets caller.

When SA_RESTART is not set, -1 EINTR should bubble up to the fgets caller. However, until this commit, fgets itself would detect EINTR and keep retrying until it read an entire line.

This commit fixes this behaviour so that fgets understands SA_RESTART.

G4Vi · 2024-04-24T02:45:57Z

Thank you for the thorough investigation and fix.

To ensure SA_RESTART is working as expected / the EINTR hack is not needed, could you please write some tests? I was thinking something to the effect of having a thread run fgets in a loop and having another thread raise signals and verifying the contents are read as expected (with SA_RESTART), and EINTR is returned sometimes otherwise. I realize this isn't the easiest thing to test, but suspect the current behavior is there for a reason.

I'm not sure if waiving copyright/putting it in public domain is enough to merge this. If you don't have any objections could you assign copyright to Justine as described here: https://github.com/jart/cosmopolitan/blob/master/CONTRIBUTING.md#copyright-assignment ? Also please note it here so I can confirm.

@restartable

See investigation in jart#1130. fgets internally calls readv. readv is a @restartable function that understands SA_RESTART. If SA_RESTART is set, readv already handles restarting the system call and eventually the string is transparently returned to the fgets caller. When SA_RESTART is not set, -1 EINTR should bubble up to the fgets caller. However, until this commit, fgets itself would detect EINTR and keep retrying until it read an entire line. This commit fixes this behaviour so that fgets understands SA_RESTART. I hereby assign copyright for this commit to Justine Tunney. Signed-off-by: Cadence Ember <cadence@disroot.org>

The new test only passes thanks to the previous commit.

cloudrac3r · 2024-04-26T01:52:54Z

I have now done the copyright assignment process, and have checked in my test file with a copyright notice under my own name as encouraged.

I haven't written C since university so I hope my test file is doing the right thing. I will welcome any suggestions.

I did clang-format and valgrind, both tools are happy with my code. I also made sure that the test only passes when fgets_unlocked.c has the fix. If you undo the fix in fgets_unlocked.c, you'll see the test fail.

I was not able to figure out how to run the entire test suite and couldn't find any instructions. The output of make test looked like I need to have 7 computers with the correct operating systems already installed, which I'm not able to do. In theory my change should be good across platforms, so fingers crossed.

G4Vi · 2024-04-26T06:11:27Z

You can run the test suite just by making items under o/$MODE/test for example make MODE= o//test/libc/stdio builds and runs the stdio tests or make MODE= o//test/libc/stdio/fgets_interrupt_test.runs builds and runs just your new test.

Unfortunately it looks like your test needs tweaks as it fails on my Linux system and on the CI runs above:

sample@frontierjustice:~/repos/cosmopolitan$ make MODE= o//test/libc/stdio
error:test/libc/stdio/fgets_interrupt_test.c:104: assertStringNotEquals() in fgets_eintr_testThatTheSignalInterruptsFgets() on frontierjustice pid 32939 tid 32939
        buf
                need "Hello world!" =
                 got "Hello world!"
        No error information
        o//test/libc/stdio/fgets_interrupt_test
error:test/libc/stdio/fgets_interrupt_test.c:105: fgets_eintr_testThatTheSignalInterruptsFgets() on frontierjustice pid 32939 tid 32939
        EXPECT_EQ(EINTR, errno)
                need 4 (or 0x04 or '\4' or EINTR) =
                 got 0 (or 0x0 or '\0')
        No error information
        o//test/libc/stdio/fgets_interrupt_test @ frontierjustice
2 / 9 tests failed

`make MODE= -j16 o//test/libc/stdio/fgets_interrupt_test.runs` exited with 2:
o//test/libc/stdio/fgets_interrupt_test
consumed 817µs wall time
ballooned to 596kb in size
needed 675us cpu (0% kernel)
caused 82 page faults (100% memcpy)
4 context switches (100% consensual)

make: *** [build/rules.mk:133: o//test/libc/stdio/fgets_interrupt_test.runs] Error 2

cloudrac3r · 2024-04-26T14:07:01Z

Looks like the o//test/libc/stdio/fgets_interrupt_test executable succeeds on my build server but fails on my laptop. Seems like adding --strace on my laptop can sometimes cause it to succeed. I think there might be a race condition in the test. I'll see if I can figure it out.

cloudrac3r · 2024-04-27T14:47:47Z

I couldn't figure out what was causing the race condition. Setting sched_setaffinity to a single core is the only way I could find to fix it reliably. Sorry for resorting to an inelegant hack.

G4Vi

Tests look good overall, thank you! No worries about sched_setaffinity, I'm not surprised something like it is needed to make this reproducible.

G4Vi · 2024-04-29T03:00:39Z

test/libc/stdio/fgets_interrupt_test.c

+#include "libc/isystem/errno.h"
+#include "libc/isystem/sched.h"
+#include "libc/isystem/signal.h"
+#include "libc/isystem/stddef.h"
+#include "libc/isystem/unistd.h"
+#include "libc/stdio/stdio.h"


Suggested change

#include "libc/isystem/errno.h"

#include "libc/isystem/sched.h"

#include "libc/isystem/signal.h"

#include "libc/isystem/stddef.h"

#include "libc/isystem/unistd.h"

#include "libc/stdio/stdio.h"

#include <errno.h>

#include <sched.h>

#include <signal.h>

#include <stddef.h>

#include <stdio.h>

#include <unistd.h>

Formatting: I don't see a precedent for including from isystem in tests. Using either the direct paths (such as libc/stdio/stdio.h) or using the standard include format <stdio.h> is both allowed, I would just stay consistent within the file.

See test/posix/... now that normal includes are possible I'm more encouraging of them for c standard library headers.

G4Vi · 2024-04-29T03:06:23Z

test/libc/stdio/fgets_interrupt_test.c

+void SetUpOnce(void) {
+  cpu_set_t set;
+  CPU_ZERO(&set);
+  CPU_SET(1, &set);


Suggested change

CPU_SET(1, &set);

CPU_SET(0, &set);

I had to set it 0 to run on my single core Windows 10 vm.

G4Vi · 2024-04-29T03:16:26Z

test/libc/stdio/fgets_interrupt_test.c

+
+TEST(fgets_eintr, testThatFgetsReadsFromPipeNormally) {
+  setup_signal_and_pipe(0);  // 0 = no SA_RESTART
+  ASSERT_NE(-1, (pid = fork()));


Consider rather than using fork, exit, etc directly use SPAWN, PARENT, and WAIT from #include "libc/testlib/subprocess.h" as a standard way to fork and wait on children hooked in with the test library. See test/libc/sock/recvfrom_test.c for an example.

On testThatTheSignalInterruptsFgets have the child EXPECT_SYS(0, 0, sigignore(SIGPIPE)); so that it exits successfully.

G4Vi · 2024-04-29T03:23:48Z

test/libc/stdio/fgets_interrupt_test.c

+TEST(fgets_eintr, testThatTheSignalInterruptsFgets) {
+  setup_signal_and_pipe(0);  // 0 = no SA_RESTART
+  ASSERT_NE(-1, (pid = fork()));
+  if (!pid) {
+    write_pipe(1);  // 1 = signal
+    _exit(0);
+  }
+  read_pipe();
+  EXPECT_STRNE(MY_TEST_STRING_1 MY_TEST_STRING_2, buf);
+  EXPECT_EQ(EINTR, errno);
+  EXPECT_EQ(1, got_sigusr1);
+}
+
+TEST(fgets_eintr, testThatFgetsRestartsWhenSaRestartIsSet) {
+  setup_signal_and_pipe(SA_RESTART);  // SA_RESTART
+  ASSERT_NE(-1, (pid = fork()));
+  if (!pid) {
+    write_pipe(1);  // 1 = signal
+    _exit(0);
+  }
+  read_pipe();
+  EXPECT_STREQ(MY_TEST_STRING_1 MY_TEST_STRING_2, buf);
+  EXPECT_NE(EINTR, errno);
+  EXPECT_EQ(1, got_sigusr1);
+}


Unfortunately neither of these tests run successfully on Windows. However, I think it's unrelated to this work. #1160

I would #include "libc/dce.h" and then at the start of these tests add something like:

// [Windows] process terminates when receiving signal during readv #1160 if (IsWindows()) return;

to bail out successfully noting why it's disabled on Windows.

Contributors are expected to make their best effort to write code that works across platforms, however they're only required to pass GitHub Actions to get merged. I'm happy to add if (IsWindows()) statements myself when I run this across my test fleet.

jart

Approved for copyright and for the change itself. Thank you!

cloudrac3r force-pushed the fgets-eintr branch from 666e8c7 to 4579edf Compare April 26, 2024 01:28

cloudrac3r force-pushed the fgets-eintr branch from 4579edf to 9df3689 Compare April 26, 2024 01:33

Test fgets, signals, and SA_RESTART interactions

e2bebc7

The new test only passes thanks to the previous commit.

fgets_interrupt_test needs single core affinity

7500b02

G4Vi requested changes Apr 29, 2024

View reviewed changes

jart approved these changes Apr 29, 2024

View reviewed changes

jart merged commit 8f6bc9d into jart:master May 3, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Let signals interrupt fgets unless SA_RESTART set #1152

Let signals interrupt fgets unless SA_RESTART set #1152

cloudrac3r commented Apr 23, 2024 •

edited

Loading

G4Vi commented Apr 24, 2024

cloudrac3r commented Apr 26, 2024 •

edited

Loading

G4Vi commented Apr 26, 2024

cloudrac3r commented Apr 26, 2024

cloudrac3r commented Apr 27, 2024

G4Vi left a comment

G4Vi Apr 29, 2024

jart May 3, 2024

G4Vi Apr 29, 2024

G4Vi Apr 29, 2024

G4Vi Apr 29, 2024

jart May 3, 2024

jart left a comment

Let signals interrupt fgets unless SA_RESTART set #1152

Let signals interrupt fgets unless SA_RESTART set #1152

Conversation

cloudrac3r commented Apr 23, 2024 • edited Loading

G4Vi commented Apr 24, 2024

cloudrac3r commented Apr 26, 2024 • edited Loading

G4Vi commented Apr 26, 2024

cloudrac3r commented Apr 26, 2024

cloudrac3r commented Apr 27, 2024

G4Vi left a comment

Choose a reason for hiding this comment

G4Vi Apr 29, 2024

Choose a reason for hiding this comment

jart May 3, 2024

Choose a reason for hiding this comment

G4Vi Apr 29, 2024

Choose a reason for hiding this comment

G4Vi Apr 29, 2024

Choose a reason for hiding this comment

G4Vi Apr 29, 2024

Choose a reason for hiding this comment

jart May 3, 2024

Choose a reason for hiding this comment

jart left a comment

Choose a reason for hiding this comment

cloudrac3r commented Apr 23, 2024 •

edited

Loading

cloudrac3r commented Apr 26, 2024 •

edited

Loading