From 28a85b6e31bb67e353dd34c237bbeecc7a026fb3 Mon Sep 17 00:00:00 2001 From: Glenn Hickey Date: Mon, 10 Aug 2026 10:56:04 -0400 Subject: [PATCH 1/2] add st_fcheck and st_fclose, for output whose write can fail stdio reports a failed write -- ENOSPC, a quota, a read-only mount -- by setting the stream's error indicator, and nothing is obliged to look at it. A program that writes an output file and never checks therefore cannot tell a complete file from a truncated one, and still exits successfully. When the format is line oriented, as most of ours are, the short file also still parses: a truncated fasta is a valid fasta with less sequence in it. This is not hypothetical. Red hit exactly this in a VGP 577-way cactus alignment, silently losing up to 473 Mb from a 2.1 Gb genome across 9 genomes, all preprocessed on one node inside a 42 minute window. st_fcheck flushes and then tests the indicator; the flush is part of the check rather than an optimisation, because the buffer is not necessarily handed to the operating system until it happens, so a stream that has already lost data can still look clean beforehand. st_fclose adds a close whose return value is also checked, closing being the last point at which buffered data reaches the operating system and so the only place a failure there is ever reported. Both die via the existing st_errAbort/st_errnoAbort path, matching st_fopen, which is the neighbouring wrapper they are meant to pair with. Purely additive: no existing caller changes behaviour. Callers get converted separately. Tested with a child process writing to /dev/full, where every write fails with ENOSPC. Before, the write returned and the stream looked fine; now the child dies with status 1 naming the file. The happy path is asserted to be untouched. Co-Authored-By: Claude Opus 5 (1M context) --- C/impl/sonLibFile.c | 22 ++++++++++++ C/inc/sonLibFile.h | 23 ++++++++++++ C/tests/sonLibFileTest.c | 76 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 121 insertions(+) diff --git a/C/impl/sonLibFile.c b/C/impl/sonLibFile.c index 987dc45..ade4dc7 100644 --- a/C/impl/sonLibFile.c +++ b/C/impl/sonLibFile.c @@ -220,3 +220,25 @@ FILE *st_fopen(const char *fileName, const char *mode) { } return ret; } + +void st_fcheck(FILE *fileHandle, const char *fileName) { + // the flush is what pushes the buffer at the operating system, so it has + // to happen before the error indicator is worth reading + if (fflush(fileHandle) != 0) { + st_errnoAbort("Failed to write the file %s, so its contents are incomplete", fileName); + } + // the indicator is sticky, so this catches a failure at any earlier point + // in the write, including one whose errno has since been overwritten + if (ferror(fileHandle)) { + st_errAbort("Failed to write the file %s, so its contents are incomplete. " + "Check the free space, the quota and the permissions on the " + "file system holding it", fileName); + } +} + +void st_fclose(FILE *fileHandle, const char *fileName) { + st_fcheck(fileHandle, fileName); + if (fclose(fileHandle) != 0) { + st_errnoAbort("Failed to close the file %s, so its contents may be incomplete", fileName); + } +} diff --git a/C/inc/sonLibFile.h b/C/inc/sonLibFile.h index 8b515d8..8ac1b08 100644 --- a/C/inc/sonLibFile.h +++ b/C/inc/sonLibFile.h @@ -106,6 +106,29 @@ void stFile_rmtree(const char *fileName); */ FILE *st_fopen(const char *fileName, const char *mode); +/* + * Die if an output file has failed to write. stdio reports a failed write -- + * a full disk, a quota, a read-only mount -- by setting the stream's error + * indicator, and nothing is obliged to look at it, so without a check a + * truncated output file is indistinguishable from a complete one and the + * program still exits successfully. A short fasta is still a valid fasta. + * + * The flush is part of the check rather than an optimisation: the buffer is + * not necessarily handed to the operating system until it happens, so a + * stream that has already lost data can still look clean beforehand. + * + * Use on stdout, or mid-stream, wherever the file is not being closed here. + */ +void st_fcheck(FILE *fileHandle, const char *fileName); + +/* + * st_fcheck followed by a close whose return value is also checked. Closing + * is the last point at which buffered data reaches the operating system, so a + * write that fails there is reported nowhere else. Prefer this to a bare + * fclose on any file the program has written. + */ +void st_fclose(FILE *fileHandle, const char *fileName); + #ifdef __cplusplus } #endif diff --git a/C/tests/sonLibFileTest.c b/C/tests/sonLibFileTest.c index 8d494df..631722e 100644 --- a/C/tests/sonLibFileTest.c +++ b/C/tests/sonLibFileTest.c @@ -4,8 +4,27 @@ * Released under the MIT license, see LICENSE.txt */ +// fork and waitpid are POSIX.1-2001. On Linux/glibc they are suppressed when +// compiling with -std=c99 (which sets __STRICT_ANSI__); _GNU_SOURCE restores +// them and must be defined before any system headers. _GNU_SOURCE is used +// rather than _POSIX_C_SOURCE because it expands the Darwin C level instead of +// restricting it, matching what sonLibFile.c does. +#if defined(__linux__) || (defined(__unix__) && !defined(__APPLE__)) +#ifndef _GNU_SOURCE +#define _GNU_SOURCE +#endif +#endif + #include "sonLibGlobalsTest.h" +// fork and waitpid exist on every Unix-like platform; the test that needs them +// is compiled out only where they do not (e.g. Windows). +#if defined(__unix__) || defined(__unix) || (defined(__APPLE__) && defined(__MACH__)) +#define ST_TEST_HAVE_FORK 1 +#include +#include +#endif + static char *tempFileDir = "sonLibFileTestTempDir"; static char *tempFileName1 = "sonLibFileTestTempDir/sonLibFileTestTempFile1.txt"; @@ -150,6 +169,59 @@ static void test_stFile_pathJoin(CuTest *testCase) { free(s); } +static void test_st_fclose(CuTest *testCase) { + setup(); + // a write that succeeds must be left completely alone by the checks + FILE *fileHandle = st_fopen(tempFileName1, "w"); + fprintf(fileHandle, "hello world\n"); + st_fcheck(fileHandle, tempFileName1); + fprintf(fileHandle, "bye bye\n"); + st_fclose(fileHandle, tempFileName1); + + fileHandle = st_fopen(tempFileName1, "r"); + char *s = stFile_getLineFromFile(fileHandle); + CuAssertStrEquals(testCase, "hello world", s); + free(s); + s = stFile_getLineFromFile(fileHandle); + CuAssertStrEquals(testCase, "bye bye", s); + free(s); + CuAssertPtrEquals(testCase, NULL, stFile_getLineFromFile(fileHandle)); + fclose(fileHandle); + teardown(); +} + +#ifdef ST_TEST_HAVE_FORK +/* + * The failure the checks exist for: a write that cannot land. Every write to + * /dev/full fails with ENOSPC, which is the disk-full case without needing a + * full disk. st_fcheck must not return, so the check runs in a child process. + * /dev/full is a Linux thing; where it is absent the child reports a skip. + */ +static void test_st_fcheck_detectsFailedWrite(CuTest *testCase) { + fflush(stdout); + fflush(stderr); + pid_t pid = fork(); + CuAssertTrue(testCase, pid >= 0); + if (pid == 0) { + // no CuAssert in here -- the parent judges this child by its exit status + FILE *fileHandle = fopen("/dev/full", "w"); + if (fileHandle == NULL) { + _exit(66); // no /dev/full, reported as a skip below + } + fprintf(fileHandle, "this cannot possibly be written\n"); + st_fcheck(fileHandle, "/dev/full"); + _exit(0); // st_fcheck returned, which is the bug this test is for + } + int status = 0; + CuAssertTrue(testCase, waitpid(pid, &status, 0) == pid); + CuAssertTrue(testCase, WIFEXITED(status)); + if (WEXITSTATUS(status) == 66) { + return; // /dev/full not present, nothing to assert + } + CuAssertIntEquals(testCase, 1, WEXITSTATUS(status)); +} +#endif + CuSuite* sonLibFileTestSuite(void) { CuSuite* suite = CuSuiteNew(); SUITE_ADD_TEST(suite, test_stFile_getLineFromFile); @@ -157,6 +229,10 @@ CuSuite* sonLibFileTestSuite(void) { SUITE_ADD_TEST(suite, test_stFile_exists); SUITE_ADD_TEST(suite, test_stFile_isDir); SUITE_ADD_TEST(suite, test_stFile_getFileNamesInDirectory); + SUITE_ADD_TEST(suite, test_st_fclose); +#ifdef ST_TEST_HAVE_FORK + SUITE_ADD_TEST(suite, test_st_fcheck_detectsFailedWrite); +#endif return suite; } From 07cff97fc481a258b20994e93465ffa1679039b1 Mon Sep 17 00:00:00 2001 From: Glenn Hickey Date: Mon, 10 Aug 2026 16:05:01 -0400 Subject: [PATCH 2/2] skip the failed-write test where /dev/full is not the real device The test distinguished only "no /dev/full" from "the real /dev/full". A third case exists: a rootfs unpacked without device nodes, or a sandbox that stubs /dev, leaves a plain writable file at that path. There the write succeeds, st_fcheck correctly returns, and the test reports a failure that says nothing about st_fcheck. Prove the device is the one we think it is, with a raw write that has to fail with ENOSPC, and treat anything else as a skip. Co-Authored-By: Claude Opus 5 (1M context) --- C/tests/sonLibFileTest.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/C/tests/sonLibFileTest.c b/C/tests/sonLibFileTest.c index 631722e..7cb9e81 100644 --- a/C/tests/sonLibFileTest.c +++ b/C/tests/sonLibFileTest.c @@ -22,6 +22,7 @@ #if defined(__unix__) || defined(__unix) || (defined(__APPLE__) && defined(__MACH__)) #define ST_TEST_HAVE_FORK 1 #include +#include #include #endif @@ -208,6 +209,14 @@ static void test_st_fcheck_detectsFailedWrite(CuTest *testCase) { if (fileHandle == NULL) { _exit(66); // no /dev/full, reported as a skip below } + // Prove the path really is the always-full device before asserting + // anything about it. A rootfs unpacked without device nodes, or a + // sandbox that stubs /dev, can leave a plain writable file here, and + // then the write below would succeed and the test would report a + // failure that says nothing about st_fcheck. + if (write(fileno(fileHandle), "x", 1) != -1 || errno != ENOSPC) { + _exit(66); + } fprintf(fileHandle, "this cannot possibly be written\n"); st_fcheck(fileHandle, "/dev/full"); _exit(0); // st_fcheck returned, which is the bug this test is for