Re: [flashrom] [PATCH] Rigorously check integrity of I/O stream data.

[Stefan Tauner]

On Sun, 29 Nov 2015 17:40:06 +0200
Urja Rannikko  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).

[PATCH] Rigorously check integrity of I/O stream data.

[Urja Rannikko]

Hi,

A small nitpick here:
+    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

Do we want to fstat if we're not using the results (on mingw), meaning
i suggest move the ifdef above struct stat definition.
Atleast with that moved,
Acked-by: Urja Rannikko