diff mbox series

debug: Fix read error handling in pcprofiledump

Message ID 87v7z5nm7b.fsf@oldenburg.str.redhat.com
State New
Headers show
Series debug: Fix read error handling in pcprofiledump | expand

Commit Message

Florian Weimer Sept. 9, 2024, 10:41 a.m. UTC 
The reading loops did not check for read failures.  Addresses
a static analysis report.

Manually tested by compiling a program with the GCC's
-finstrument-functions option, running it with
“LD_PRELOAD=debug/libpcprofile.so PCPROFILE_OUTPUT=output-file”,
and reviewing the output of “debug/pcprofiledump output-file”.

---
 debug/pcprofiledump.c | 77 +++++++++++++++++++++++++++------------------------
 1 file changed, 41 insertions(+), 36 deletions(-)


base-commit: 43669fcf7315f494bbbc2c040cedeb0fa8416a5f

Comments

Andreas Schwab Sept. 9, 2024, 11:09 a.m. UTC | #1 
On Sep 09 2024, Florian Weimer wrote:

> +	  if (!read_exactly (fd, ptrs, sizeof (ptrs),
> +			    _("cannot read pointer pair"), true))

I think it is better to mark the message with N_ and call gettext only
when the error actually happens in read_exactly.
diff mbox series

Patch

diff --git a/debug/pcprofiledump.c b/debug/pcprofiledump.c
index 049a9c2744..e4e5d2855a 100644
--- a/debug/pcprofiledump.c
+++ b/debug/pcprofiledump.c
@@ -75,6 +75,36 @@  static struct argp argp =
   options, parse_opt, args_doc, doc, NULL, more_help
 };
 
+/* Try to read SIZE bytes from FD and store them on BUF.  Terminate
+   the processes with ERROR_MESSAGE upon read error.  Also terminate
+   the process if less than SIZE bytes are remaining in the file and
+   !MAYBE_EOF.  If MAYBE_EOF, do not terminate the process if the end
+   of the file is encountered immediately.
+
+   Returns true if SIZE bytes have been read, and false if no bytes
+   have been read due to an end-of-file condition.  */
+static bool
+read_exactly (int fd, void *buffer, size_t size, const char *error_message,
+	      bool maybe_eof)
+{
+  char *p = buffer;
+  char *end = p + size;
+  while (p < end)
+    {
+      ssize_t ret = TEMP_FAILURE_RETRY (read (fd, p, end - p));
+      if (ret < 0)
+	error (EXIT_FAILURE, errno, error_message);
+      if (ret == 0)
+	{
+	  if (p == buffer && maybe_eof)
+	    /* Nothing has been read.  */
+	    return false;
+	  error (EXIT_FAILURE, 0, _("unexpected end of file"));
+	}
+      p += ret;
+    }
+  return true;
+}
 
 int
 main (int argc, char *argv[])
@@ -110,8 +140,7 @@  main (int argc, char *argv[])
   /* Read the first 4-byte word.  It contains the information about
      the word size and the endianness.  */
   uint32_t word;
-  if (TEMP_FAILURE_RETRY (read (fd, &word, 4)) != 4)
-    error (EXIT_FAILURE, errno, _("cannot read header"));
+  read_exactly (fd, &word, sizeof (word), _("cannot read header"), false);
 
   /* Check whether we have to swap the byte order.  */
   int must_swap = (word & 0x0fffffff) == bswap_32 (0xdeb00000);
@@ -121,56 +150,32 @@  main (int argc, char *argv[])
   /* We have two loops, one for 32 bit pointers, one for 64 bit pointers.  */
   if (word == 0xdeb00004)
     {
-      union
-      {
-	uint32_t ptrs[2];
-	char bytes[8];
-      } pair;
+      uint32_t ptrs[2];
 
       while (1)
 	{
-	  size_t len = sizeof (pair);
-	  size_t n;
-
-	  while (len > 0
-		 && (n = TEMP_FAILURE_RETRY (read (fd, &pair.bytes[8 - len],
-						   len))) != 0)
-	    len -= n;
-
-	  if (len != 0)
-	    /* Nothing to read.  */
+	  if (!read_exactly (fd, ptrs, sizeof (ptrs),
+			    _("cannot read pointer pair"), true))
 	    break;
 
 	  printf ("this = %#010" PRIx32 ", caller = %#010" PRIx32 "\n",
-		  must_swap ? bswap_32 (pair.ptrs[0]) : pair.ptrs[0],
-		  must_swap ? bswap_32 (pair.ptrs[1]) : pair.ptrs[1]);
+		  must_swap ? bswap_32 (ptrs[0]) : ptrs[0],
+		  must_swap ? bswap_32 (ptrs[1]) : ptrs[1]);
 	}
     }
   else if (word == 0xdeb00008)
     {
-      union
-      {
-	uint64_t ptrs[2];
-	char bytes[16];
-      } pair;
+      uint64_t ptrs[2];
 
       while (1)
 	{
-	  size_t len = sizeof (pair);
-	  size_t n;
-
-	  while (len > 0
-		 && (n = TEMP_FAILURE_RETRY (read (fd, &pair.bytes[8 - len],
-						   len))) != 0)
-	    len -= n;
-
-	  if (len != 0)
-	    /* Nothing to read.  */
+	  if (!read_exactly (fd, ptrs, sizeof (ptrs),
+			    _("cannot read pointer pair"), true))
 	    break;
 
 	  printf ("this = %#018" PRIx64 ", caller = %#018" PRIx64 "\n",
-		  must_swap ? bswap_64 (pair.ptrs[0]) : pair.ptrs[0],
-		  must_swap ? bswap_64 (pair.ptrs[1]) : pair.ptrs[1]);
+		  must_swap ? bswap_64 (ptrs[0]) : ptrs[0],
+		  must_swap ? bswap_64 (ptrs[1]) : ptrs[1]);
 	}
     }
   else