* [PATCH 1/4] libdwfl: Make sure mapped is always set in unzip @ 2024-06-22 23:50 Mark Wielaard 2024-06-22 23:50 ` [PATCH 2/4] libelf: elf32_getshdr might leak section header when out of memory Mark Wielaard ` (3 more replies) 0 siblings, 4 replies; 5+ messages in thread From: Mark Wielaard @ 2024-06-22 23:50 UTC (permalink / raw) To: elfutils-devel; +Cc: Mark Wielaard Found by GCC14 -Wanalyzer-null-argument. When unzip is called with mapped NULL, but *_whole not NULL, *_whole contains the first part of the input. But we check against mapped to make sure the MAGIC bytes are there. This only worked because this code path was never taken, unzip is currently always called with *_whole being NULL. * libdwfl/gzip.c (unzip): Set mapped = state.input_buffer when *whole is not NULL. Signed-off-by: Mark Wielaard <mark@klomp.org> --- libdwfl/gzip.c | 1 + 1 file changed, 1 insertion(+) diff --git a/libdwfl/gzip.c b/libdwfl/gzip.c index 002afc4e916b..9c74abdafc19 100644 --- a/libdwfl/gzip.c +++ b/libdwfl/gzip.c @@ -212,6 +212,7 @@ unzip (int fd, off_t start_offset, else { state.input_buffer = *state.whole; + mapped = state.input_buffer; state.input_pos = state.mapped_size = *whole_size; } } -- 2.45.2 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 2/4] libelf: elf32_getshdr might leak section header when out of memory 2024-06-22 23:50 [PATCH 1/4] libdwfl: Make sure mapped is always set in unzip Mark Wielaard @ 2024-06-22 23:50 ` Mark Wielaard 2024-06-22 23:50 ` [PATCH 3/4] debuginfod-client: Don't leak id/version with duplicate os-release entries Mark Wielaard ` (2 subsequent siblings) 3 siblings, 0 replies; 5+ messages in thread From: Mark Wielaard @ 2024-06-22 23:50 UTC (permalink / raw) To: elfutils-devel; +Cc: Mark Wielaard Found by GCC -fanalyzer. When allocating the notcvt buffer fails we leak the shdr. goto free_and_out on malloc failure. * libelf/elf32_getshdr.c (load_shdr_wrlock): goto free_and_out on second malloc failure. Signed-off-by: Mark Wielaard <mark@klomp.org> --- libelf/elf32_getshdr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libelf/elf32_getshdr.c b/libelf/elf32_getshdr.c index fc696302b8b6..19b690a8e87b 100644 --- a/libelf/elf32_getshdr.c +++ b/libelf/elf32_getshdr.c @@ -126,7 +126,7 @@ load_shdr_wrlock (Elf_Scn *scn) if (unlikely (notcvt == NULL)) { __libelf_seterrno (ELF_E_NOMEM); - goto out; + goto free_and_out; } memcpy (notcvt, ((char *) elf->map_address + elf->start_offset + ehdr->e_shoff), -- 2.45.2 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 3/4] debuginfod-client: Don't leak id/version with duplicate os-release entries 2024-06-22 23:50 [PATCH 1/4] libdwfl: Make sure mapped is always set in unzip Mark Wielaard 2024-06-22 23:50 ` [PATCH 2/4] libelf: elf32_getshdr might leak section header when out of memory Mark Wielaard @ 2024-06-22 23:50 ` Mark Wielaard 2024-06-22 23:50 ` [PATCH 4/4] ar, ranlib: Don't double close file descriptors Mark Wielaard 2024-06-25 20:18 ` [PATCH 1/4] libdwfl: Make sure mapped is always set in unzip Mark Wielaard 3 siblings, 0 replies; 5+ messages in thread From: Mark Wielaard @ 2024-06-22 23:50 UTC (permalink / raw) To: elfutils-devel; +Cc: Mark Wielaard Found by GCC14 -Wanalyzer-double-free. If the os-release file would contain multiple ID or VERSION_ID entries we would leak the originally parsed one. Fix by seeing whether id or version is already set and ignore any future entries. * debuginfod/debuginfod-client.c (add_default_headers): Check whether id or version is already set before resetting them. Signed-off-by: Mark Wielaard <mark@klomp.org> --- debuginfod/debuginfod-client.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c index 95f2a92b701c..24ede19af385 100644 --- a/debuginfod/debuginfod-client.c +++ b/debuginfod/debuginfod-client.c @@ -673,9 +673,9 @@ add_default_headers(debuginfod_client *client) v++; s[len - 1] = '\0'; } - if (strcmp (s, "ID") == 0) + if (id == NULL && strcmp (s, "ID") == 0) id = strdup (v); - if (strcmp (s, "VERSION_ID") == 0) + if (version == NULL && strcmp (s, "VERSION_ID") == 0) version = strdup (v); } fclose (f); -- 2.45.2 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 4/4] ar, ranlib: Don't double close file descriptors 2024-06-22 23:50 [PATCH 1/4] libdwfl: Make sure mapped is always set in unzip Mark Wielaard 2024-06-22 23:50 ` [PATCH 2/4] libelf: elf32_getshdr might leak section header when out of memory Mark Wielaard 2024-06-22 23:50 ` [PATCH 3/4] debuginfod-client: Don't leak id/version with duplicate os-release entries Mark Wielaard @ 2024-06-22 23:50 ` Mark Wielaard 2024-06-25 20:18 ` [PATCH 1/4] libdwfl: Make sure mapped is always set in unzip Mark Wielaard 3 siblings, 0 replies; 5+ messages in thread From: Mark Wielaard @ 2024-06-22 23:50 UTC (permalink / raw) To: elfutils-devel; +Cc: Mark Wielaard Found by GCC14 -Wanalyzer-fd-double-close. close always closes the given file descriptor even on error. So don't try to close a file descriptor again on error (even on EINTR). This could be bad in a multi-threaded environment. * src/ar.c (do_oper_extract): Call close and set newfd to -1. (do_oper_delete): Likewise. (do_oper_insert): Likewise. * src/ranlib.c (handle_file): Likewise. Signed-off-by: Mark Wielaard <mark@klomp.org> --- src/ar.c | 12 ++++++------ src/ranlib.c | 4 ++-- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/ar.c b/src/ar.c index fcb8bfb90a9f..9ace28b918d3 100644 --- a/src/ar.c +++ b/src/ar.c @@ -808,9 +808,9 @@ cannot rename temporary file to %.*s"), if (fchown (newfd, st.st_uid, st.st_gid) != 0) { ; } /* Set the mode of the new file to the same values the original file has. */ - if (fchmod (newfd, st.st_mode & ALLPERMS) != 0 - || close (newfd) != 0) + if (fchmod (newfd, st.st_mode & ALLPERMS) != 0) goto nonew_unlink; + close (newfd); newfd = -1; if (rename (tmpfname, arfname) != 0) goto nonew_unlink; @@ -1061,9 +1061,9 @@ do_oper_delete (const char *arfname, char **argv, int argc, setting the mode (which might be reset/ignored if the owner is wrong. */ if (fchown (newfd, st.st_uid, st.st_gid) != 0) { ; } - if (fchmod (newfd, st.st_mode & ALLPERMS) != 0 - || close (newfd) != 0) + if (fchmod (newfd, st.st_mode & ALLPERMS) != 0) goto nonew_unlink; + close (newfd); newfd = -1; if (rename (tmpfname, arfname) != 0) goto nonew_unlink; @@ -1547,9 +1547,9 @@ do_oper_insert (int oper, const char *arfname, char **argv, int argc, setting the modes, or they might be reset/ignored if the owner is wrong. */ if (fchown (newfd, st.st_uid, st.st_gid) != 0) { ; } - if (fchmod (newfd, st.st_mode & ALLPERMS) != 0 - || close (newfd) != 0) + if (fchmod (newfd, st.st_mode & ALLPERMS) != 0) goto nonew_unlink; + close (newfd); newfd = -1; if (rename (tmpfname, arfname) != 0) goto nonew_unlink; diff --git a/src/ranlib.c b/src/ranlib.c index 7838d69eaec6..073df8c551af 100644 --- a/src/ranlib.c +++ b/src/ranlib.c @@ -264,9 +264,9 @@ handle_file (const char *fname) if (fchown (newfd, st.st_uid, st.st_gid) != 0) { ; } /* Set the mode of the new file to the same values the original file has. */ - if (fchmod (newfd, st.st_mode & ALLPERMS) != 0 - || close (newfd) != 0) + if (fchmod (newfd, st.st_mode & ALLPERMS) != 0) goto nonew_unlink; + close (newfd); newfd = -1; if (rename (tmpfname, fname) != 0) goto nonew_unlink; -- 2.45.2 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/4] libdwfl: Make sure mapped is always set in unzip 2024-06-22 23:50 [PATCH 1/4] libdwfl: Make sure mapped is always set in unzip Mark Wielaard ` (2 preceding siblings ...) 2024-06-22 23:50 ` [PATCH 4/4] ar, ranlib: Don't double close file descriptors Mark Wielaard @ 2024-06-25 20:18 ` Mark Wielaard 3 siblings, 0 replies; 5+ messages in thread From: Mark Wielaard @ 2024-06-25 20:18 UTC (permalink / raw) To: elfutils-devel Hi, On Sun, Jun 23, 2024 at 01:50:10AM +0200, Mark Wielaard wrote: > Found by GCC14 -Wanalyzer-null-argument. Aaron took a quick look on irc and said all 4 patches looked reasonable. So I pushed them all to main now. Cheers, Mark ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-06-25 20:18 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-06-22 23:50 [PATCH 1/4] libdwfl: Make sure mapped is always set in unzip Mark Wielaard 2024-06-22 23:50 ` [PATCH 2/4] libelf: elf32_getshdr might leak section header when out of memory Mark Wielaard 2024-06-22 23:50 ` [PATCH 3/4] debuginfod-client: Don't leak id/version with duplicate os-release entries Mark Wielaard 2024-06-22 23:50 ` [PATCH 4/4] ar, ranlib: Don't double close file descriptors Mark Wielaard 2024-06-25 20:18 ` [PATCH 1/4] libdwfl: Make sure mapped is always set in unzip Mark Wielaard
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).