Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Let signals interrupt fgets unless SA_RESTART set #1152

Merged
merged 3 commits into from
May 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions libc/stdio/fgets_unlocked.c
Original file line number Diff line number Diff line change
Expand Up @@ -55,11 +55,7 @@ char *fgets_unlocked(char *s, int size, FILE *f) {
if (t) break;
} else {
if ((c = fgetc_unlocked(f)) == -1) {
if (ferror_unlocked(f) == EINTR) {
continue;
} else {
break;
}
break;
}
*p++ = c & 255;
if (c == '\n') break;
Expand Down
138 changes: 138 additions & 0 deletions test/libc/stdio/fgets_interrupt_test.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,138 @@
/*-*- mode:c;indent-tabs-mode:nil;c-basic-offset:2;tab-width:8;coding:utf-8 -*-│
│ vi: set et ft=c ts=2 sts=2 sw=2 fenc=utf-8 :vi │
╞══════════════════════════════════════════════════════════════════════════════╡
│ Copyright 2024 Cadence Ember │
│ │
│ Permission to use, copy, modify, and/or distribute this software for │
│ any purpose with or without fee is hereby granted, provided that the │
│ above copyright notice and this permission notice appear in all copies. │
│ │
│ THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL │
│ WARRANTIES WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED │
│ WARRANTIES OF MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE │
│ AUTHOR BE LIABLE FOR ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL │
│ DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR │
│ PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER │
│ TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR │
│ PERFORMANCE OF THIS SOFTWARE. │
╚─────────────────────────────────────────────────────────────────────────────*/
#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"
Comment on lines +19 to +24
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

#include "libc/testlib/testlib.h"

#define MY_TEST_STRING_1 "He"
#define MY_TEST_STRING_2 "llo world!"

char buf[20] = {0};
int pipes[2];
int pid;
int got_sigusr1 = 0;

// -=- these get called for each test ------------------------------------------

void sigusr1_handler(int) {
got_sigusr1 = 1;
}

void write_pipe(int send_signal_before_end) {
// Set up pipe for writing
close(pipes[0]);
FILE *stream = fdopen(pipes[1], "w");

// Start writing the first part of the stream
fputs(MY_TEST_STRING_1, stream);

// Send SIGUSR1 to parent (if we're currently testing that)
if (send_signal_before_end) {
kill(getppid(), SIGUSR1);
}

// Send rest of stream
fputs(MY_TEST_STRING_2, stream);
// Close stream - this will cause the parent's fgets to end
fclose(stream);
}

void read_pipe() {
// Set up pipe for reading
close(pipes[1]);
FILE *stream = fdopen(pipes[0], "r");

// Read with fgets
fgets(buf, 20, stream);

// Tidy up
fclose(stream);
}

// -=- these set up the tests --------------------------------------------------

void SetUpOnce(void) {
cpu_set_t set;
CPU_ZERO(&set);
CPU_SET(1, &set);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (sched_setaffinity(0, sizeof set, &set) == -1) {
perror("sched_setaffinity");
fprintf(stderr, "single core affinity is needed for test reliability\n");
_exit(1);
}
}

void setup_signal_and_pipe(uint64_t sa_flags) {
// Set up SIGUSR1 handler
struct sigaction sa = {.sa_handler = sigusr1_handler, .sa_flags = sa_flags};
if (sigaction(SIGUSR1, &sa, NULL) == -1) {
perror("sigaction");
_exit(1);
}
got_sigusr1 = 0;

// Set up pipe between parent and child
if (pipe(pipes) == -1) {
perror("pipe");
_exit(1);
}
}

// -=- these are the tests -----------------------------------------------------

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (!pid) {
write_pipe(0); // 0 = no signal
_exit(0);
}
read_pipe();
EXPECT_STREQ(MY_TEST_STRING_1 MY_TEST_STRING_2, buf);
}

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);
}
Comment on lines +114 to +138
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading