| Message ID | 20181211101411.7067-2-tdevries@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | Handle .gnu_debugaltlink | expand |
On Tue, Dec 11, 2018 at 2:14 AM Tom de Vries <tdevries@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 <tdevries@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. > + 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. More seriously, we don't call error_callback in any cases that correspond to this. We just carry on. Is there any reason to call error_callback here? Ian
On 16-01-19 01:56, Ian Lance Taylor wrote: > On Tue, Dec 11, 2018 at 2:14 AM Tom de Vries <tdevries@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 <tdevries@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. 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 <tdevries@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/elf.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 95 insertions(+), 5 deletions(-) diff --git a/libbacktrace/elf.c b/libbacktrace/elf.c index 85a323c5876..e363e470525 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,43 @@ 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; + } + } + else + { + error_callback (data, + "could not open .gnu_debugaltlink", 0); + /* Don't goto fail, but try continue without the info in the + .gnu_debugaltlink. */ + } + } + + 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 +3287,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 +3359,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 +3387,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;
On Wed, Jan 16, 2019 at 8:26 AM Tom de Vries <tdevries@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 <tdevries@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 <tdevries@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
On 16-01-19 18:14, Ian Lance Taylor wrote: > On Wed, Jan 16, 2019 at 8:26 AM Tom de Vries <tdevries@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 <tdevries@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 <tdevries@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 <tdevries@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;
On Wed, Jan 16, 2019 at 2:48 PM Tom de Vries <tdevries@suse.de> wrote: > > For now, I've dropped the error callback for .gnu_debugaltlink. This version is OK. Thanks. Ian
diff --git a/libbacktrace/elf.c b/libbacktrace/elf.c index f4863f0bea5..f3bc2119c07 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,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; + } + /* Read the .opd section on PowerPC64 ELFv1. */ if (ehdr.e_machine == EM_PPC64 && (ehdr.e_flags & EF_PPC64_ABI) < 2 @@ -2993,8 +3033,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 +3071,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 +3090,43 @@ 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; + } + } + else + { + error_callback (data, + "Could not open .gnu_debugaltlink", 0); + /* Don't goto fail, but try continue without the info in the + .gnu_debugaltlink. */ + } + } + + 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 +3282,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 +3354,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 +3382,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;