Re: [COMMITTED] debuginfod_find_section: Always update rc with most recent error code
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
Re: [COMMITTED] debuginfod_find_section: Always update rc with most recent error code
Hi Mark,
On Mon, Nov 7, 2022 at 9:19 AM Mark Wielaard 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
[COMMITTED] debuginfod-client.c: Don't treat 0 as an error code.
Replace 'fd > 0' with 'fd >= 0' to avoid treating a possible file
descriptor as an error code.
Signed-off-by: Aaron Merey
---
debuginfod/ChangeLog | 5 +
debuginfod/debuginfod-client.c | 4 ++--
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
index 5678002a..a17bc5ab 100644
--- a/debuginfod/ChangeLog
+++ b/debuginfod/ChangeLog
@@ -1,3 +1,8 @@
+2022-11-07 Aaron Merey
+
+ * debuginfod-client.c (debuginfod_find_section): Don't treat 0 as an
+ error code.
+
2022-11-04 Aaron Merey
* debuginfod-client.c (debuginfod_find_section): Ensure rc
diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index 99da05ef..f9f26fd5 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -1936,7 +1936,7 @@ debuginfod_find_section (debuginfod_client *client,
}
return -ENOENT;
}
- if (fd > 0)
+ if (fd >= 0)
{
rc = extract_section (fd, section, tmp_path, path);
close (fd);
@@ -1948,7 +1948,7 @@ debuginfod_find_section (debuginfod_client *client,
be in the executable. */
fd = debuginfod_find_executable (client, build_id,
build_id_len, &tmp_path);
- if (fd > 0)
+ if (fd >= 0)
{
rc = extract_section (fd, section, tmp_path, path);
close (fd);
--
2.37.3
☺ Buildbot (GNU Toolchain): elfutils - build successful (master)
A restored build has been detected on builder elfutils-centos-x86_64 while building elfutils. Full details are available at: https://builder.sourceware.org/buildbot/#builders/39/builds/106 Build state: build successful Revision: 88bab837c1d96a1f9a589dca74101ff378a0ff65 Worker: centos-x86_64 Build Reason: (unknown) Blamelist: Aaron Merey Steps: - 0: worker_preparation ( success ) - 1: set package name ( success ) - 2: git checkout ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/39/builds/106/steps/2/logs/stdio - 3: autoreconf ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/39/builds/106/steps/3/logs/stdio - 4: configure ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/39/builds/106/steps/4/logs/stdio - 5: get version ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/39/builds/106/steps/5/logs/stdio - property changes: https://builder.sourceware.org/buildbot/#builders/39/builds/106/steps/5/logs/property_changes - 6: make ( warnings ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/39/builds/106/steps/6/logs/stdio - warnings (2): https://builder.sourceware.org/buildbot/#builders/39/builds/106/steps/6/logs/warnings__2_ - 7: make check ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/39/builds/106/steps/7/logs/stdio - test-suite.log: https://builder.sourceware.org/buildbot/#builders/39/builds/106/steps/7/logs/test-suite_log - 8: prep ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/39/builds/106/steps/8/logs/stdio - 9: build bunsen.cpio.gz ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/39/builds/106/steps/9/logs/stdio - 10: fetch bunsen.cpio.gz ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/39/builds/106/steps/10/logs/stdio - 11: unpack bunsen.cpio.gz ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/39/builds/106/steps/11/logs/stdio - 12: pass .bunsen.source.gitname ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/39/builds/106/steps/12/logs/stdio - 13: pass .bunsen.source.gitdescribe ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/39/builds/106/steps/13/logs/stdio - 14: pass .bunsen.source.gitbranch ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/39/builds/106/steps/14/logs/stdio - 15: pass .bunsen.source.gitrepo ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/39/builds/106/steps/15/logs/stdio - 16: upload to bunsen ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/39/builds/106/steps/16/logs/stdio - 17: clean up ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/39/builds/106/steps/17/logs/stdio - 18: make distclean ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/39/builds/106/steps/18/logs/stdio A new failure has been detected on builder elfutils-opensusetw-x86_64 while building elfutils. Full details are available at: https://builder.sourceware.org/buildbot/#builders/88/builds/75 Build state: failed test (failure) Revision: 88bab837c1d96a1f9a589dca74101ff378a0ff65 Worker: bb2-2 Build Reason: (unknown) Blamelist: Aaron Merey Steps: - 0: worker_preparation ( success ) - 1: set package name ( success ) - 2: git checkout ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/88/builds/75/steps/2/logs/stdio - 3: autoreconf ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/88/builds/75/steps/3/logs/stdio - 4: configure ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/88/builds/75/steps/4/logs/stdio - 5: get version ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/88/builds/75/steps/5/logs/stdio - property changes: https://builder.sourceware.org/buildbot/#builders/88/builds/75/steps/5/logs/property_changes - 6: make ( warnings ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/88/builds/75/steps/6/logs/stdio - warnings (3): https://builder.sourceware.org/buildbot/#builders/88/builds/75/steps/6/logs/warnings__3_ - 7: make check ( success ) Logs: - stdio: https://builder.sourceware.org/buildbot/#builders/88/builds/75/steps/7/logs/stdio - test-suite.log: https://builder.sourceware.org/buildbot/#builders/88/builds/75/steps/7/logs/test-suite_log - 8: make distcheck ( failure ) Logs: - stdio: https://builder.sourceware.org/buildbot/#b
