On 16-01-19 18:14, Ian Lance Taylor wrote:
> 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.
Aha, I see.
> 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.
Agreed.
> 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?
I think the "if MSG starts with a '-' then this is just a warning"
convention will break existing clients.
An easy solution that would be backward compatible would be to add
versions of each function that accepts an error callback, that also
accepts a warning call back, say for:
...
extern struct backtrace_state *backtrace_create_state (
const char *filename,
int threaded,
backtrace_error_callback error_callback,
void *data);
...
we add:
...
extern struct backtrace_state *backtrace_create_state_with_warning (
const char *filename,
int threaded,
backtrace_error_callback error_callback,
backtrace_warning_callback warning_call,
void *data);
...
Each client would continue to work as before. And if a client want
warnings it could enable this by using the _with_warning variants of the
API functions.
For now, I've dropped the error callback for .gnu_debugaltlink.
Thanks,
- Tom
[libbacktrace] Read .gnu_debugaltlink
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.
---
libbacktrace/Makefile.in | 2 +-
libbacktrace/elf.c | 93 +++++++++++++++++++++++++++++++++++++++++++++---
2 files changed, 89 insertions(+), 6 deletions(-)
diff --git a/libbacktrace/Makefile.in b/libbacktrace/Makefile.in
index c595a8b4a3e..17e7ea86879 100644
--- a/libbacktrace/Makefile.in
+++ b/libbacktrace/Makefile.in
@@ -15,7 +15,7 @@
@SET_MAKE@
# Makefile.am -- Backtrace Makefile.
-# Copyright (C) 2012-2018 Free Software Foundation, Inc.
+# Copyright (C) 2012-2019 Free Software Foundation, Inc.
# Redistribution and use in source and binary forms, with or without
# modification, are permitted provided that the following conditions are
diff --git a/libbacktrace/elf.c b/libbacktrace/elf.c
index 85a323c5876..abe4cded5e9 100644
--- a/libbacktrace/elf.c
+++ b/libbacktrace/elf.c
@@ -2638,7 +2638,8 @@ static int
elf_add (struct backtrace_state *state, const char *filename, int descriptor,
uintptr_t base_address, backtrace_error_callback error_callback,
void *data, fileline *fileline_fn, int *found_sym, int *found_dwarf,
- int exe, int debuginfo)
+ int exe, int debuginfo, const char *with_buildid_data,
+ uint32_t with_buildid_size)
{
struct backtrace_view ehdr_view;
b_elf_ehdr ehdr;
@@ -2670,6 +2671,11 @@ elf_add (struct backtrace_state *state, const char *filename, int descriptor,
int debuglink_view_valid;
const char *debuglink_name;
uint32_t debuglink_crc;
+ struct backtrace_view debugaltlink_view;
+ int debugaltlink_view_valid;
+ const char *debugaltlink_name;
+ const char *debugaltlink_buildid_data;
+ uint32_t debugaltlink_buildid_size;
off_t min_offset;
off_t max_offset;
struct backtrace_view debug_view;
@@ -2694,6 +2700,10 @@ elf_add (struct backtrace_state *state, const char *filename, int descriptor,
debuglink_view_valid = 0;
debuglink_name = NULL;
debuglink_crc = 0;
+ debugaltlink_view_valid = 0;
+ debugaltlink_name = NULL;
+ debugaltlink_buildid_data = NULL;
+ debugaltlink_buildid_size = 0;
debug_view_valid = 0;
opd = NULL;
@@ -2873,6 +2883,15 @@ elf_add (struct backtrace_state *state, const char *filename, int descriptor,
buildid_data = ¬e->name[0] + ((note->namesz + 3) & ~ 3);
buildid_size = note->descsz;
}
+
+ if (with_buildid_size != 0)
+ {
+ if (buildid_size != with_buildid_size)
+ goto fail;
+
+ if (memcmp (buildid_data, with_buildid_data, buildid_size) != 0)
+ goto fail;
+ }
}
/* Read the debuglink file if present. */
@@ -2899,6 +2918,32 @@ 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);
+ 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;
+ }
+ }
+
/* Read the .opd section on PowerPC64 ELFv1. */
if (ehdr.e_machine == EM_PPC64
&& (ehdr.e_flags & EF_PPC64_ABI) < 2
@@ -2993,8 +3038,11 @@ elf_add (struct backtrace_state *state, const char *filename, int descriptor,
if (debuglink_view_valid)
backtrace_release_view (state, &debuglink_view, error_callback,
data);
+ if (debugaltlink_view_valid)
+ backtrace_release_view (state, &debugaltlink_view, error_callback,
+ data);
ret = elf_add (state, NULL, d, base_address, error_callback, data,
- fileline_fn, found_sym, found_dwarf, 0, 1);
+ fileline_fn, found_sym, found_dwarf, 0, 1, NULL, 0);
if (ret < 0)
backtrace_close (d, error_callback, data);
else
@@ -3028,8 +3076,11 @@ elf_add (struct backtrace_state *state, const char *filename, int descriptor,
backtrace_release_view (state, &debuglink_view, error_callback,
data);
+ if (debugaltlink_view_valid)
+ backtrace_release_view (state, &debugaltlink_view, error_callback,
+ data);
ret = elf_add (state, NULL, d, base_address, error_callback, data,
- fileline_fn, found_sym, found_dwarf, 0, 1);
+ fileline_fn, found_sym, found_dwarf, 0, 1, NULL, 0);
if (ret < 0)
backtrace_close (d, error_callback, data);
else
@@ -3044,6 +3095,36 @@ elf_add (struct backtrace_state *state, const char *filename, int descriptor,
debuglink_view_valid = 0;
}
+ 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;
+ }
+ }
+ }
+
+ if (debugaltlink_view_valid)
+ {
+ backtrace_release_view (state, &debugaltlink_view, error_callback, data);
+ debugaltlink_view_valid = 0;
+ }
+
/* Read all the debug sections in a single view, since they are
probably adjacent in the file. We never release this view. */
@@ -3199,6 +3280,8 @@ elf_add (struct backtrace_state *state, const char *filename, int descriptor,
backtrace_release_view (state, &strtab_view, error_callback, data);
if (debuglink_view_valid)
backtrace_release_view (state, &debuglink_view, error_callback, data);
+ if (debugaltlink_view_valid)
+ backtrace_release_view (state, &debugaltlink_view, error_callback, data);
if (buildid_view_valid)
backtrace_release_view (state, &buildid_view, error_callback, data);
if (debug_view_valid)
@@ -3269,7 +3352,7 @@ phdr_callback (struct dl_phdr_info *info, size_t size ATTRIBUTE_UNUSED,
if (elf_add (pd->state, filename, descriptor, info->dlpi_addr,
pd->error_callback, pd->data, &elf_fileline_fn, pd->found_sym,
- &found_dwarf, 0, 0))
+ &found_dwarf, 0, 0, NULL, 0))
{
if (found_dwarf)
{
@@ -3297,7 +3380,7 @@ backtrace_initialize (struct backtrace_state *state, const char *filename,
struct phdr_data pd;
ret = elf_add (state, filename, descriptor, 0, error_callback, data,
- &elf_fileline_fn, &found_sym, &found_dwarf, 1, 0);
+ &elf_fileline_fn, &found_sym, &found_dwarf, 1, 0, NULL, 0);
if (!ret)
return 0;
