Re: Bug in some textutils handling of fread()/EOF

[Jim Meyering]

Michael Bacarella <address@hidden> wrote:
> How to reproduce:
>
>     1. Run md5sum interactively.  Make sure you're using textutils
>     md5sum, some GNU/Linux distributions (like Debian) have their own
>     md5sum and rename the one from textutils to something like 
> md5sum.textutils.
>         $ md5sum
>
>     2. Type one line of text into md5sum, then hit CR. "foobar" works.
>         $ md5sum
>         foobar
>
>     3. Now hit EOF.  Nothing should happen (the bug)
>         $ md5sum
>         foobar
>
>     4. Hit EOF again.  Now it should exit.
>         $ md5sum
>         foobar
>         14758f1afd44c09b7992073ccf00b43d  -
>         $

Thank you for the fine report and patches.
I've applied your cksum patch.
I chose to make a more invasive (but functionally equivalent)
change to md5.c and sha.c.

        * md5.c (md5_stream) [BLOCKSIZE]: Move definition to top of file.
        Ensure that it is a multiple of 64.
        Rearrange loop exit tests so as to avoid performing an
        additional fread after encountering an error or EOF.
        * sha.c (sha_stream): Likewise.
        Reported by Michael Bacarella.

Index: md5.c
===================================================================
RCS file: /fetish/cu/lib/md5.c,v
retrieving revision 1.13
retrieving revision 1.14
diff -u -p -u -r1.13 -r1.14
--- md5.c       17 Nov 2001 13:29:42 -0000      1.13
+++ md5.c       18 Feb 2003 19:00:16 -0000      1.14
@@ -52,6 +52,12 @@
 # define SWAP(n) (n)
 #endif
 
+#define BLOCKSIZE 4096
+/* Ensure that BLOCKSIZE is a multiple of 64.  */
+#if BLOCKSIZE % 64 != 0
+/* FIXME-someday (soon?): use #error instead of this kludge.  */
+"invalid BLOCKSIZE"
+#endif
 
 /* This array contains the bytes used to pad the buffer to the next
    64-byte boundary.  (RFC 1321, 3.1: Step 1)  */
@@ -132,8 +138,6 @@ md5_stream (stream, resblock)
      FILE *stream;
      void *resblock;
 {
-  /* Important: BLOCKSIZE must be a multiple of 64.  */
-#define BLOCKSIZE 4096
   struct md5_ctx ctx;
   char buffer[BLOCKSIZE + 72];
   size_t sum;
@@ -151,19 +155,31 @@ md5_stream (stream, resblock)
       sum = 0;
 
       /* Read block.  Take care for partial reads.  */
-      do
+      while (1)
        {
          n = fread (buffer + sum, 1, BLOCKSIZE - sum, stream);
 
          sum += n;
+
+         if (sum == BLOCKSIZE)
+           break;
+
+         if (n == 0)
+           {
+             /* Check for the error flag IFF N == 0, so that we don't
+                exit the loop after a partial read due to e.g., EAGAIN
+                or EWOULDBLOCK.  */
+             if (ferror (stream))
+               return 1;
+             goto process_partial_block;
+           }
+
+         /* We've read at least one byte, so ignore errors.  But always
+            check for EOF, since feof may be true even though N > 0.
+            Otherwise, we could end up calling fread after EOF.  */
+         if (feof (stream))
+           goto process_partial_block;
        }
-      while (sum < BLOCKSIZE && n != 0);
-      if (n == 0 && ferror (stream))
-        return 1;
-
-      /* If end of file is reached, end the loop.  */
-      if (n == 0)
-       break;
 
       /* Process buffer with BLOCKSIZE bytes.  Note that
                        BLOCKSIZE % 64 == 0
@@ -171,7 +187,9 @@ md5_stream (stream, resblock)
       md5_process_block (buffer, BLOCKSIZE, &ctx);
     }
 
-  /* Add the last bytes if necessary.  */
+ process_partial_block:;
+
+  /* Process any remaining bytes.  */
   if (sum > 0)
     md5_process_bytes (buffer, sum, &ctx);