Even if fwrite() succeeds the data is not necessarily out of the clib's buffers and writing it eventually could fail. Even if the data is flushed out (explicitly by fflush() or implicitly by fclose()) the kernel might still hold a buffer.
Previously we have ignored this to a large extent - even in important cases like writing the flash contents to a file. The results can be truncated images that would brick the respective machine if written back as is (though flashrom would not allow that due to a size mismatch). flashrom would not indicate the problem in any output - so far we only check the return value of fwrite() that is not conclusive.
This patch checks the return values of all related system calls like fclose() unless we only read the file and are not really interested in output errors. In the latter case the return value is casted to void to document this fact. Additionally, this patch explicitly calls fflush() and fsync() to do the best we can to guarantee the read image reaches the disk safely.
Signed-off-by: Stefan Tauner stefan.tauner@alumni.tuwien.ac.at --- flashrom.c | 56 ++++++++++++++++++++++++++++++++++-------------------- layout.c | 6 +++--- processor_enable.c | 4 ++-- 3 files changed, 40 insertions(+), 26 deletions(-)
The reason why I was investigating this at all was a user report on IRC that showed that flashrom on OpenWRT would create a truncated file if there is not enough space left on the file system to store the flash image file. After adding support for uclibc on our buildbot I was able to reproduce the problem. Unfortunately, checking the return value of fclose() does not help with uclibc, nor does calling fflush() or fsync() - uclibc is simply broken in this regard, cf.: https://bugs.gentoo.org/show_bug.cgi?id=543972 https://bugs.uclibc.org/5156
glibc and musl are not affected. Both notice the problem at fwrite() time.
Nevertheless I think we should merge this patch ASAP.
diff --git a/flashrom.c b/flashrom.c index c9c7e31..3b2388b 100644 --- a/flashrom.c +++ b/flashrom.c @@ -28,6 +28,7 @@ #include <sys/stat.h> #endif #include <string.h> +#include <unistd.h> #include <stdlib.h> #include <errno.h> #include <ctype.h> @@ -1255,36 +1256,39 @@ int read_buf_from_file(unsigned char *buf, unsigned long size, msg_gerr("Error: No file I/O support in libpayload\n"); return 1; #else - unsigned long numbytes; - FILE *image; - struct stat image_stat; + int ret = 0;
+ FILE *image; if ((image = fopen(filename, "rb")) == NULL) { msg_gerr("Error: opening file "%s" failed: %s\n", filename, strerror(errno)); return 1; } + + struct stat image_stat; if (fstat(fileno(image), &image_stat) != 0) { msg_gerr("Error: getting metadata of file "%s" failed: %s\n", filename, strerror(errno)); - fclose(image); - return 1; + ret = 1; + goto out; } if (image_stat.st_size != size) { msg_gerr("Error: Image size (%jd B) doesn't match the flash chip's size (%lu B)!\n", (intmax_t)image_stat.st_size, size); - fclose(image); - return 1; - } - numbytes = fread(buf, 1, size, image); - if (fclose(image)) { - msg_gerr("Error: closing file "%s" failed: %s\n", filename, strerror(errno)); - return 1; + ret = 1; + goto out; } + + unsigned long numbytes = fread(buf, 1, size, image); if (numbytes != size) { msg_gerr("Error: Failed to read complete file. Got %ld bytes, " "wanted %ld!\n", numbytes, size); - return 1; + ret = 1; } - return 0; +out: + if (fclose(image)) { + msg_gerr("Error: closing file "%s" failed: %s\n", filename, strerror(errno)); + ret = 1; + } + return ret; #endif }
@@ -1294,8 +1298,8 @@ int write_buf_to_file(const unsigned char *buf, unsigned long size, const char * msg_gerr("Error: No file I/O support in libpayload\n"); return 1; #else - unsigned long numbytes; FILE *image; + int ret = 0;
if (!filename) { msg_gerr("No filename specified.\n"); @@ -1306,14 +1310,24 @@ int write_buf_to_file(const unsigned char *buf, unsigned long size, const char * return 1; }
- numbytes = fwrite(buf, 1, size, image); - fclose(image); + unsigned long numbytes = fwrite(buf, 1, size, image); if (numbytes != size) { - msg_gerr("File %s could not be written completely.\n", - filename); - return 1; + msg_gerr("Error: file %s could not be written completely.\n", filename); + ret = 1; } - return 0; + if (fflush(image)) { + msg_gerr("Error: flushing file "%s" failed: %s\n", filename, strerror(errno)); + ret = 1; + } + if (fsync(fileno(image))) { + msg_gerr("Error: fsyncing file "%s" failed: %s\n", filename, strerror(errno)); + ret = 1; + } + if (fclose(image)) { + msg_gerr("Error: closing file "%s" failed: %s\n", filename, strerror(errno)); + ret = 1; + } + return ret; #endif }
diff --git a/layout.c b/layout.c index 0b9f6e5..d039451 100644 --- a/layout.c +++ b/layout.c @@ -65,7 +65,7 @@ int read_romlayout(const char *name) if (num_rom_entries >= MAX_ROMLAYOUT) { msg_gerr("Maximum number of ROM images (%i) in layout " "file reached.\n", MAX_ROMLAYOUT); - fclose(romlayout); + (void)fclose(romlayout); return 1; } if (2 != fscanf(romlayout, "%s %s\n", tempstr, rom_entries[num_rom_entries].name)) @@ -80,7 +80,7 @@ int read_romlayout(const char *name) tstr2 = strtok(NULL, ":"); if (!tstr1 || !tstr2) { msg_gerr("Error parsing layout file. Offending string: "%s"\n", tempstr); - fclose(romlayout); + (void)fclose(romlayout); return 1; } rom_entries[num_rom_entries].start = strtol(tstr1, (char **)NULL, 16); @@ -95,7 +95,7 @@ int read_romlayout(const char *name) rom_entries[i].end, rom_entries[i].name); }
- fclose(romlayout); + (void)fclose(romlayout);
return 0; } diff --git a/processor_enable.c b/processor_enable.c index 1361dd5..117aa1e 100644 --- a/processor_enable.c +++ b/processor_enable.c @@ -57,11 +57,11 @@ static int is_loongson(void) ptr++; while (*ptr && isspace((unsigned char)*ptr)) ptr++; - fclose(cpuinfo); + (void)fclose(cpuinfo); return (strncmp(ptr, "ICT Loongson-2 V0.3", strlen("ICT Loongson-2 V0.3")) == 0) || (strncmp(ptr, "Godson2 V0.3 FPU V0.1", strlen("Godson2 V0.3 FPU V0.1")) == 0); } - fclose(cpuinfo); + (void)fclose(cpuinfo); return 0; } #endif