[COMMITTED] debuginfod_find_section: Always update rc with most recent error code
Commit Message
debuginfod_find_section may attempt to download both the debuginfo
and executable matching the given build-id. If neither of these
files can be found, update rc to ensure that we always return an
accurate error code in this case.
Signed-off-by: Aaron Merey <amerey@redhat.com>
---
debuginfod/ChangeLog | 5 +++++
debuginfod/debuginfod-client.c | 6 +++++-
2 files changed, 10 insertions(+), 1 deletion(-)
Comments
Hi Aaron,
On Fri, 2022-11-04 at 17:40 -0400, Aaron Merey via Elfutils-devel
wrote:
> debuginfod_find_section may attempt to download both the debuginfo
> and executable matching the given build-id. If neither of these
> files can be found, update rc to ensure that we always return an
> accurate error code in this case.
Nicely spotted, otherwise we might have returned -EEXIST.
> diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-
> client.c
> index f48e32cc..99da05ef 100644
> --- a/debuginfod/debuginfod-client.c
> +++ b/debuginfod/debuginfod-client.c
> @@ -1944,7 +1944,8 @@ debuginfod_find_section (debuginfod_client *client,
>
> if (rc == -EEXIST)
> {
> - /* The section should be found in the executable. */
> + /* Either the debuginfo couldn't be found or the section should
> + be in the executable. */
> fd = debuginfod_find_executable (client, build_id,
> build_id_len, &tmp_path);
> if (fd > 0)
I know this is in existing code, so this might have missed in a
previous review. But shouldn't this be fd >= 0 ?
That is what is checked in the rest of the code. Except for the
debuginfod_find_section function which uses fd >0 twice.
It is unlikely, but I think fd can be zero if it (stdin) was closed by
the program for some reason. Then I think zero can be reused as new
file descriptor?
Cheers,
Mark
Hi Mark,
On Mon, Nov 7, 2022 at 9:19 AM Mark Wielaard <mark@klomp.org> wrote:
> > if (rc == -EEXIST)
> > {
> > - /* The section should be found in the executable. */
> > + /* Either the debuginfo couldn't be found or the section should
> > + be in the executable. */
> > fd = debuginfod_find_executable (client, build_id,
> > build_id_len, &tmp_path);
> > if (fd > 0)
>
> I know this is in existing code, so this might have missed in a
> previous review. But shouldn't this be fd >= 0 ?
>
> That is what is checked in the rest of the code. Except for the
> debuginfod_find_section function which uses fd >0 twice.
>
> It is unlikely, but I think fd can be zero if it (stdin) was closed by
> the program for some reason. Then I think zero can be reused as new
> file descriptor?
Thanks for catching this typo, will merge a fix.
Aaron
@@ -1,3 +1,8 @@
+2022-11-04 Aaron Merey <amerey@redhat.com>
+
+ * debuginfod-client.c (debuginfod_find_section): Ensure rc
+ is always updated with the most recent error code.
+
2022-11-03 Frank Ch. Eigler <fche@redhat.com>
* debuginfod.cxx (handle_buildid): Correctly manage lifetime
@@ -1944,7 +1944,8 @@ debuginfod_find_section (debuginfod_client *client,
if (rc == -EEXIST)
{
- /* The section should be found in the executable. */
+ /* Either the debuginfo couldn't be found or the section should
+ be in the executable. */
fd = debuginfod_find_executable (client, build_id,
build_id_len, &tmp_path);
if (fd > 0)
@@ -1952,6 +1953,9 @@ debuginfod_find_section (debuginfod_client *client,
rc = extract_section (fd, section, tmp_path, path);
close (fd);
}
+ else
+ /* Update rc so that we return the most recent error code. */
+ rc = fd;
}
free (tmp_path);