On Sun, 29 Nov 2015 17:40:06 +0200
Urja Rannikko <[email protected]> wrote:
> Hi,
>
> On Sun, Nov 22, 2015 at 7:43 PM, Stefan Tauner
> > @@ -1255,36 +1256,39 @@ int read_buf_from_file(unsigned char *buf, unsigned
> > long size,
> > […]
> > +out:
> > + if (fclose(image)) {
> > + msg_gerr("Error: closing file \"%s\" failed: %s\n",
> > filename, strerror(errno));
> > + ret = 1;
> > + }
> > + return ret;
> > #endif
> > }
> Why do this goto stuff and fclose fail reporting to read_buf_from_file?
> I'd suggest same handling as below for layout files etc (= (void)fclose()).
>
> If you fix that (or convince me that you want to print reading fclose
> failure), this is acked by me.
>
Fair enough... I'd rather convert all of them to report errors but I
agree that consistency is important, and the checking for errors in
that case is highly debatable anyway... so I have void-casted this one
and then discovered that fsync is not implemented in MinGW :(
I have #if-guarded the respective part in the attached patch. This
however does not commit the file as we intend to on Windows. OTOH just
fsync()ing on Unix does not provide 100% certainty either so I think
this should still get in because it is a clear improvement.
It builds fine one the build bot (where expected).
--
Kind regards/Mit freundlichen Grüßen, Stefan Tauner
>From 49d5e98b9cfb629eba05e243e001a3413125873f Mon Sep 17 00:00:00 2001
From: Stefan Tauner <[email protected]>
Date: Thu, 17 Dec 2015 00:39:21 +0100
Subject: [PATCH] Rigorously check integrity of I/O stream data.
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() (on regular files only)
to do the best we can to guarantee the read image reaches the disk safely
and at least inform the user if it did not work.
Signed-off-by: Stefan Tauner <[email protected]>
Acked-by: Urja Rannikko <[email protected]>
---
flashrom.c | 66 +++++++++++++++++++++++++++++++++++++-----------------
layout.c | 6 ++---
processor_enable.c | 4 ++--
3 files changed, 50 insertions(+), 26 deletions(-)
diff --git a/flashrom.c b/flashrom.c
index c9c7e31..5f4d654 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,36 @@ 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:
+ (void)fclose(image);
+ return ret;
#endif
}
@@ -1294,8 +1295,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 +1307,37 @@ 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;
+ goto out;
}
- return 0;
+ if (fflush(image)) {
+ msg_gerr("Error: flushing file \"%s\" failed: %s\n", filename, strerror(errno));
+ ret = 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));
+ ret = 1;
+ goto out;
+ }
+ // Try to fsync() only regular files if that function is available at all (e.g. not provided by MinGW).
+#if defined(_POSIX_FSYNC) && (_POSIX_FSYNC != -1)
+ if (S_ISREG(image_stat.st_mode)) {
+ if (fsync(fileno(image))) {
+ msg_gerr("Error: fsyncing file \"%s\" failed: %s\n", filename, strerror(errno));
+ ret = 1;
+ }
+ }
+#endif
+out:
+ 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
--
Kind regards, Stefan Tauner
_______________________________________________
flashrom mailing list
[email protected]
http://www.flashrom.org/mailman/listinfo/flashrom
