Multiple cr_assert_stderr_eq_str() calls: Unexpected signal caught below this line! CRASH!

[saladuit]

I'm trying to check the stderr and I keep getting the a crash. This example crashes:

Test(check_argc, possible_stack_sizes)
{
	check_argc_test(1, false, "");
	check_argc_test(2, false, "Error\n");
}

While this example works:

Test(check_argc, possible_stack_sizes)
{
	check_argc_test(1, false, "");
}
Test(check_argc, possible_stack_sizes)
{
	check_argc_test(2, false, "Error\n");
}

I call the following code block and I know that the cr_assrt_stderr_eq_str() causes the crash.

void	check_argc_test(const int argc, const bool expected, const char *error_output)
{
	bool submitted;

	submitted = count_check(argc);
	cr_assert(submitted == expected,
			"Called:\tcheck_argc_test()\nargc:\t\t%d\nexpected:\t%s \nsubmitted:\t%s\n",
			argc,
			formatBool(expected),
			formatBool(submitted));
	cr_assert_stderr_eq_str(error_output);
	return ;
}

The block of code that is being test is the following:

void	ft_error(void)
{
		ft_putendl_fd("Error", 2);
}

bool	count_check(int argc)
{
	if (argc == 2)
		ft_error();
	if (argc < 3)
		return (false);
	return (true);
}

Can someone explain to me why this happens?

[saladuit]

I also tried fflush()

Test(check_argc, possible_stack_sizes)
{
	check_argc_test(1, false, "");
	fflush(stderr);
	check_argc_test(2, false, "Error\n");
	fflush(stderr);
	check_argc_test(3, true, "");
}

[Snaipe]

cr_assert_stderr_eq_str can only work if cr_redirect_stderr() is called first. See the redirect.c example for an example on how to use it.

That said, it definitely shouldn't be crashing, but just failing with some better error message.

[saladuit]

Ah! I didn't include that, but i did .init=redirect_all_std beforehand. So that wouldn't be the problem.

[Snaipe]

Actually, re-reading this, I think I'm getting what's wrong -- calling cr_assert_stderr_eq_str multiple times in the test is technically not allowed, in the sense that the function consumes the entirety of stderr until EOF. stderr would have to get closed for the first call to finish, so calling it a second time would not work.

Can you backtrace where this crashes? This should probably be fixed so that it just fails the test rather than crashing.

[saladuit]

I've tried to figure this one out with 4 of my peers who couldn't find an answer and we finally discussed wether a test case should even test the stderr/stdout, or that I should just redesign the code in another way. There was one idea to fork cr_assert_stderr_eq() but that resulted in not being able to fail the case. That did however fix the crash. My peers were able to reporduce it.

[saladuit]

This was the trace of one of my peers

➜  push_swap git:(master) ✗ ./unit-test --debug=lldb
[----] Criterion v2.4.1-2-g9ecaf8b (bleeding)
[====] Running 3 tests from check_argc:
PLEASE submit a bug report to https://github.com/Homebrew/homebrew-core/issues and include the crash backtrace.
Stack dump:
0.      Program arguments: boxfort-worker gdbserver *:1234 /Users/thijs/codam/saladin/push_swap/unit-test
1.      Program arguments: boxfort-worker gdbserver *:1234 /Users/thijs/codam/saladin/push_swap/unit-test
Stack dump without symbol names (ensure you have llvm-symbolizer in your PATH or set the environment var `LLVM_SYMBOLIZER_PATH` to point to it):
0  libLLVM.dylib            0x00000001121c9894 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) + 56
1  libLLVM.dylib            0x00000001121c9e68 SignalHandler(int) + 324
2  libsystem_platform.dylib 0x00000001ba3914c4 _sigtramp + 56
3  lldb-server              0x0000000104b6aa74 lldb_private::process_gdb_remote::GDBRemoteCommunicationServerLLGS::LaunchProcess() + 296
4  lldb-server              0x0000000104b079a0 handle_launch(lldb_private::process_gdb_remote::GDBRemoteCommunicationServerLLGS&, llvm::ArrayRef<llvm::StringRef>) + 464
5  lldb-server              0x0000000104b09270 main_gdbserver(int, char**) + 3112
6  lldb-server              0x0000000104b0c950 main + 176
7  dyld                     0x0000000105275088 start + 516
criterion: Could not spawn test instance: Protocol error
[1]    36220 abort      ./unit-test --debug=lldb

[saladuit]

This was on of the findings of another peer: "I ran it with gdb and I get a SIGPIPE (broken pipe), I'm guessing it means the tester closes stdout or stderr somewhere after piping it, your program then tries to write to it but it's closed (broken) and receives the SIGPIPE"

[Snaipe]

That SIGPIPE seems consistent with what the function does. cr_assert_stderr_eq_str consumes the entire stream until EOF, which means that the writer needs to close the stream -- calling it twice without reopening stderr will necessarily break.

That being said cr_assert_stderr_eq_str is part of the only remaining assert functions without an equivalent in the new API. I expect that cr_stream will provide better semantics, but we should probably provide some helper for this specific use case -- see #250.

If using cr_stream, it could look something like this:

Test(example, test) {
    cr_redirect_stderr();

    cr_stream actual   = cr_stream_open("fd://stderr");
    cr_stream expected = cr_stream_fromstring("hello, world");

    fprintf(stderr, "hello, world");
    fclose(stderr);

    cr_assert(eq(stream, expected, actual));
    cr_stream_close(actual);

    stderr = fdopen(STDERR_FILENO);
    cr_redirect_stderr();

    actual   = cr_stream_open("fd://stderr");
    expected = cr_stream_fromstring("foobar");

    fprintf(stderr, "foobar");
    fclose(stderr);

    cr_assert(eq(stream, expected, actual));
    cr_stream_close(actual);
}

That said, there's no magic bullet. You have to close the stream to guarantee that the assertion never blocks in the original API, and that assumption holds in the new API. And as you can see above, it's easy to shoot yourself in the foot.

[MrAnno]

@Snaipe Considering the original API (cr_assert_{stdout, stderr}_*()), I think Criterion could behave a bit more intuitively. My understanding is that a fdopen()-ed FILE * will not block after EOF, it will just consistently report the same status with each repetition.

I think we could allow multiple stdout/stderr assertion calls within the same test by removing fclose() from the stdout/stderr matchers, moving them to an automatic destructor, which runs after the test. Since the error and eof indicators are "cached", an additional clearerr() call might be necessary for each assert.

The following seems to be working after implementing the above:

Test(redirect, test_outputs, .init = cr_redirect_stdout) {
    fprintf(stdout, "foo");
    fflush(stdout);
    cr_assert_stdout_eq_str("foo");

    fprintf(stdout, "bar");
    fflush(stdout);
    cr_assert_stdout_eq_str("bar");
    cr_assert_stdout_eq_str("");
    cr_assert_stdout_eq_str("");

    fclose(stdout);
    cr_assert_stdout_eq_str(""); // we could probably make this fail
    cr_assert_stdout_eq_str("");
}

[MrAnno]

To be precise, fread() produces an ferror (Resource temporarily unavailable) instead of feof in those cases.

[Snaipe]

The semantics you propose seem OK, but I'd rather have this be fully replaced with the cr_stream API (and even have cr_{assert,expect}_{stdout,err}_eq_{str,file} be implemented under the covers with an appropriate cr_stream construct. So if we can get consistent semantics with these backward-compat macros and cr_streams, I'm all for it.

I think something that threw me a bit for a loop was that streams may block on reads, so it's a bit dangerous (in the sense that you might hang) to rely on just flush before asserting on stdout/stderr, however the way we have this implemented we have to store standard streams in growing buffers (or a tempfile) which means we don't have to close them in order to perform assertions on them.

If testing stdout/err is common enough, it might be worth adding a couple of new tags or criteria in the assertion API. Not sure what's worth doing right now. A weakness of the current use of the stream APIs is that you have to setup a bunch of boilerplate to create a stream from a string because eq requires you to compare two streams together.