Bug 30505 - unchecked "debugedit" "gdb-add-index" "strip_to_debug" "add_minidebug" invocation
Summary: unchecked "debugedit" "gdb-add-index" "strip_to_debug" "add_minidebug" invoca...
Status: NEW
Alias: None
Product: debugedit
Classification: Unclassified
Component: find-debuginfo (show other bugs)
Version: unspecified
: P2 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-06-01 11:04 UTC by Laszlo Ersek
Modified: 2024-08-30 23:21 UTC (History)
5 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Laszlo Ersek 2023-06-01 11:04:29 UTC
As of commit 5bade25a11b5, "scripts/find-debuginfo.in" contains:

>|   # Add .gdb_index if requested.
>|   if $include_gdb_index; then
>|     if type gdb-add-index >/dev/null 2>&1; then
>|       gdb-add-index "$f"
>|     else
>|       echo >&2 "*** ERROR: GDB index requested, but no gdb-add-index installed"
>|       exit 2
>|     fi
>|   fi

If the objcopy invocation underlying "gdb-add-index" fails, then
"gdb-add-index" propagates that issue through its exit status; however,
"find-debuginfo" ignores it (see above).

This allows RPM builds to succeed if debuginfo extraction is started
with "-i" (i.e., GDB index is being requested), gdb-add-index is
available, but gdb-add-index fails for some reason.

One example is a shared library object that contains Perl bindings for a
C-language library. Perl's ExtUtils::Install module installs artifacts
in read-only mode. On a read-only .so, objcopy fails, hence
gdb-add-index fails. Yet the RPM build succeeds, resulting in such
debuginfo (*.so.debug) files that lack the .gdb_index section. The
expected behavior is that the RPM build fail, similarly to how it
currently fails if "-i" is passed to "find-debuginfo", but
"gdb-add-index" is unavailable (see the"exit 2" statement in the quote).

Installing files in read-only mode is arguably a bug in
ExtUtils::Install itself
<https://rt.cpan.org/Public/Bug/Display.html?id=40976>; however, that
does not mean "find-debuginfo" should ignore "gdb-add-index" failures.

I figure something like the following might work:

>| diff --git a/scripts/find-debuginfo.in b/scripts/find-debuginfo.in
>| index 7dec3c3bfdc5..e0d931175cd9 100755
>| --- a/scripts/find-debuginfo.in
>| +++ b/scripts/find-debuginfo.in
>| @@ -470,6 +470,10 @@ do_file()
>|    if $include_gdb_index; then
>|      if type gdb-add-index >/dev/null 2>&1; then
>|        gdb-add-index "$f"
>| +      exit_status=$?
>| +      if test $exit_status -ne 0; then
>| +        exit $exit_status
>| +      fi
>|      else
>|        echo >&2 "*** ERROR: GDB index requested, but no gdb-add-index installed"
>|        exit 2

However, "find-debuginfo" seems to have an (undocumented) convention
where it uses only exit statuses 1 and 2. In that case, propagating the
exit status from gdb-add-index may not be right. Then one alternative
would be:

>| diff --git a/scripts/find-debuginfo.in b/scripts/find-debuginfo.in
>| index 7dec3c3bfdc5..99cb3b906a00 100755
>| --- a/scripts/find-debuginfo.in
>| +++ b/scripts/find-debuginfo.in
>| @@ -469,7 +469,7 @@ do_file()
>|    # Add .gdb_index if requested.
>|    if $include_gdb_index; then
>|      if type gdb-add-index >/dev/null 2>&1; then
>| -      gdb-add-index "$f"
>| +      gdb-add-index "$f" || exit 1
>|      else
>|        echo >&2 "*** ERROR: GDB index requested, but no gdb-add-index installed"
>|        exit 2

Note that the problem (not checking the exit status of gdb-add-index)
dates back to the original introduction of gdb-add-index to
find-debuginfo:

  https://github.com/rpm-software-management/rpm/commit/9570a7f6af15
  https://github.com/rpm-software-management/rpm/commit/41c4dcf507e2
  https://github.com/rpm-software-management/rpm/commit/67d3df338875
  https://github.com/rpm-software-management/rpm/commit/04b0805a756c
Comment 1 Mark Wielaard 2023-06-01 11:27:04 UTC
Thanks for reporting this (and providing different patches to fix it).

I think originally we ignored errors like these (same for add_minidebug) because these are "just nice to have extras". But given that people explicitly opt in to these "extras" (or at least not opt-out) we should probably do as you suggest and explicitly report errors.
Comment 2 Keith Seitz 2024-08-16 18:58:36 UTC
I've developed a more thorough patch for this.

Even with the patch herein applied, find-debuginfo.sh
will never exit with a non-zero status.

I've submitted a patch (which includes something for this
bug):

https://sourceware.org/pipermail/debugedit/2024-August/000290.html
Comment 3 Mark Wielaard 2024-08-20 14:56:21 UTC
commit dfe1f7ff30f4e0be538835fca1e6348723ea7aa7
Author: Keith Seitz <keiths@redhat.com>
Date:   Fri Aug 16 11:54:20 2024 -0700

    find-debuginfo.sh: Exit with real exit status in parallel jobs
    
    Currently, when the script is executed in parallel (-jN), the
    resulting exit status will always be 0.
    
    The script execs an appropriate number of clones of itself, calling
    run_job to run the actual workload. This then calls do_file(), saving
    the exit status into "res.$jobid".
    
    In do_file(), though, if an error occurs, exit is called. This causes
    the entire exec'd shell to exit with status 0 (since there are almost
    always echo calls as the last executed statement). The real exit
    status is therefor never written to the "res.$jobid" files by run_job().
    
    The simple solution is to use 'return' instead of 'exit'. A number
    of minor adjustments are also made to propagate this properly so that
    it is reported as the correct exit status.
    
    While at it, I've incorporated a patch for find-debuginfo/30505.
    Using this patch and another patch to the RPM package (submitted as
    github issue #3215), failures of gdb-add-index.sh will now properly fail
    the build instead of being swallowed. It should be much easier for
    developers to figure out why their builds have failed should gdb crash.
    
    Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30505
    Signed-off-by: Keith Seitz <keiths@redhat.com>
Comment 4 Mark Wielaard 2024-08-20 15:01:07 UTC
Although the above patch now propagates the errors from gdb-add-index, there are still some other places that "hide" errors. The main debugedit invocation, strip_to_debug, add_minidebug.

So keeping this bug open for now to track we handle those too.