On Wed, Jan 16, 2019 at 8:26 AM Tom de Vries <tdevr...@suse.de> wrote:
>
> On 16-01-19 01:56, Ian Lance Taylor wrote:
> > On Tue, Dec 11, 2018 at 2:14 AM Tom de Vries <tdevr...@suse.de> wrote:
> >>
> >> Read the elf file pointed at by the .gnu_debugaltlink section, and verify
> >> that
> >> the build id matches.
> >>
> >> 2018-11-11 Tom de Vries <tdevr...@suse.de>
> >>
> >> * elf.c (elf_add): Add and handle with_buildid_data and
> >> with_buildid_size parameters. Handle .gnu_debugaltlink section.
> >> (phdr_callback, backtrace_initialize): Add arguments to elf_add
> >> calls.
> >> ---
> >
> >
> >
> > @@ -2899,6 +2918,27 @@ elf_add (struct backtrace_state *state, const
> > char *filename, int descriptor,
> >> }
> >> }
> >>
> >> + if (!debugaltlink_view_valid
> >> + && strcmp (name, ".gnu_debugaltlink") == 0)
> >> + {
> >> + const char *debugaltlink_data;
> >> + size_t debugaltlink_name_len;
> >> +
> >> + if (!backtrace_get_view (state, descriptor, shdr->sh_offset,
> >> + shdr->sh_size, error_callback, data,
> >> + &debugaltlink_view))
> >> + goto fail;
> >> +
> >> + debugaltlink_view_valid = 1;
> >> + debugaltlink_data = (const char *) debugaltlink_view.data;
> >> + debugaltlink_name = debugaltlink_data;
> >> + debugaltlink_name_len = strnlen (debugaltlink_data,
> >> shdr->sh_size);
> >> + debugaltlink_buildid_data = (debugaltlink_data
> >> + + debugaltlink_name_len
> >> + + 1);
> >> + debugaltlink_buildid_size = shdr->sh_size -
> >> debugaltlink_name_len - 1;
> >> + }
> >> +
> >
> > This doesn't look quite right. debugaltlink_buildid_size is unsigned.
> > If there is some misunderstanding of the format it's possible for
> > strnlen to return shdr->sh_size. If it does,
> > debugaltlink_buildid_size will be set to a very large value.
> >
>
> I see, thanks for finding that.
>
> Fixed like this:
> ...
> debugaltlink_name_len = strnlen (debugaltlink_data, shdr->sh_size);
> if (debugaltlink_name_len < shdr->sh_size)
> {
> /* Include terminating zero. */
> debugaltlink_name_len =+ 1;
>
> debugaltlink_buildid_data
> = debugaltlink_data + debugaltlink_name_len;
> debugaltlink_buildid_size
> = shdr->sh_size - debugaltlink_name_len;
> }
> ...
>
> >> + if (debugaltlink_name != NULL)
> >> + {
> >> + int d;
> >> +
> >> + d = elf_open_debugfile_by_debuglink (state, filename,
> >> debugaltlink_name,
> >> + 0, error_callback, data);
> >> + if (d >= 0)
> >> + {
> >> + int ret;
> >> +
> >> + ret = elf_add (state, filename, d, base_address, error_callback,
> >> data,
> >> + fileline_fn, found_sym, found_dwarf, 0, 1,
> >> + debugaltlink_buildid_data,
> >> debugaltlink_buildid_size);
> >> + backtrace_release_view (state, &debugaltlink_view,
> >> error_callback,
> >> + data);
> >> + debugaltlink_view_valid = 0;
> >> + if (ret < 0)
> >> + {
> >> + backtrace_close (d, error_callback, data);
> >> + return ret;
> >> + }
> >> + }
> >> + else
> >> + {
> >> + error_callback (data,
> >> + "Could not open .gnu_debugaltlink", 0);
> >> + /* Don't goto fail, but try continue without the info in the
> >> + .gnu_debugaltlink. */
> >> + }
> >> + }
> >
> > The strings passed to error_callback always start with a lowercase
> > letter (unless they start with something like ELF) because the
> > callback will most likely print them with some prefix.
> >
>
> Fixed.
>
> > More seriously, we don't call error_callback in any cases that
> > correspond to this. We just carry on.
>
> Indeed, I noticed that, and filed PR88244 - "[libbacktrace] Failure to
> open .gnu_debuglink is silent".
>
> [ The scenario there is: an executable has a .gnu_debuglink, but the
> file the .gnu_debuglink is pointing to is absent, because f.i. it has
> been removed, or moved to a different location. If a user runs this
> executable and a backtrace is triggered, the information relating to the
> functions in the executable will be missing in the backtrace, but
> there's no error explaining to the user why that information is missing.
> Note: there is a default error "no debug info in ELF executable" in
> elf_nodebug, but AFAIU this is not triggered if debug info for one of
> the shared libraries is present. ]
>
> BTW, though in the code above an error_callback is called, we don't
> error out, but do carry on afterwards (as the comment explicitly states).
>
> > Is there any reason to call
> > error_callback here?
>
> A similar scenario: an executable has a .gnu_altdebuglink, but the file
> the .gnu_debugaltlink is pointing to is absent, because f.i. it has been
> removed, or moved to a different location. If a user runs this
> executable and a backtrace is triggered, the information stored in the
> .gnu_debugaltlink file will be missing in the backtrace, but there's no
> error explaining to the user why that information is missing.
The problem is that libbacktrace is often run in cases where it needs
to present best effort information. But the error_callback is
currently only called for cases where it can't present any information
at all. You are suggesting that we call it and then carry on. But
currently we don't do that (as far as I know); we call it and then
fail. For example, in libgo, if the error_callback function is
called, the program will print the error and then crash. So while I
understand your desire to present a warning message to the user about
a missing file, I don't think we should use the existing
error_callback mechanism to do so. I think we'll need to introduce
some way to let error_callback know that this is a warning rather than
an error. Unfortunately changing the actual API at this point would
be somewhat painful. Still, we should either do that, or introduce
some convention like "if MSG starts with a '-' then this is just a
warning." Any thoughts?
Ian
