debug: Fix read error handling in pcprofiledump
Checks
Context |
Check |
Description |
redhat-pt-bot/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 |
fail
|
Build failed
|
redhat-pt-bot/TryBot-32bit |
success
|
Build for i686
|
linaro-tcwg-bot/tcwg_glibc_build--master-arm |
fail
|
Build failed
|
Commit Message
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
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.
@@ -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