Re: [PATCH 1/9] [libbacktrace] Read .gnu_debugaltlink
On Wed, Jan 16, 2019 at 2:48 PM Tom de Vries wrote: > > For now, I've dropped the error callback for .gnu_debugaltlink. This version is OK. Thanks. Ian
Re: [PATCH 1/9] [libbacktrace] Read .gnu_debugaltlink
On 16-01-19 18:14, Ian Lance Taylor wrote:
> On Wed, Jan 16, 2019 at 8:26 AM Tom de Vries wrote:
>>
>> On 16-01-19 01:56, Ian Lance Taylor wrote:
>>> On Tue, Dec 11, 2018 at 2:14 AM Tom de Vries 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
* 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,
+ _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, _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
>>>
Re: [PATCH 1/9] [libbacktrace] Read .gnu_debugaltlink
On Wed, Jan 16, 2019 at 8:26 AM Tom de Vries wrote:
>
> On 16-01-19 01:56, Ian Lance Taylor wrote:
> > On Tue, Dec 11, 2018 at 2:14 AM Tom de Vries 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
> >>
> >> * 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,
> >> + _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, _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
Re: [PATCH 1/9] [libbacktrace] Read .gnu_debugaltlink
On 16-01-19 01:56, Ian Lance Taylor wrote:
> On Tue, Dec 11, 2018 at 2:14 AM Tom de Vries 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
>>
>> * 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,
>> + _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, _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
Re: [PATCH 1/9] [libbacktrace] Read .gnu_debugaltlink
On Tue, Dec 11, 2018 at 2:14 AM Tom de Vries 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
>
> * 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,
> + _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, _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
