Re: [Bug debuginfod/27277] Describe retrieved files when verbose
Hi Noah,
On Thu, 2022-07-14 at 11:32 -0400, Noah Sanci via Elfutils-devel wrote:
> Please find the patch for pr28284 attached
>
> Debuginfod and debuginfod clients are now equipped to send
> and receive http headers prefixed with X-DEBUGINFOD and
> print them in verbose mode for more context
OK, so this patch does 3 things:
- Introduce a debuginfod_get_headers public api for libdebuginfod
that provides any X-DEBUGINFOD prefixed headers for the last
retrieved file if the http(s) protocol was used (but not for the file
protocol nor when the file was found in the cache).
- It changes debuginfod-find to use this new api to show those headers
in verbose mode.
- Changes the debuginfod server to use the api to add any
X-DEBUGINFOD headers to the response if the request was
delegated.
So the last two are obvious once we have the first. I am slightly
worried about the "inconsistency" in when these headers are available.
Should we maybe cache them? Or do they only really make sense during
download/in the progress callback?
There currently are 3 special headers:
- X-DEBUGINFOD-FILE the (base) file name of the original file
- X-DEBUGINFOD-SIZE the on-disk size of the file
- X-DEBUGINFOD-ARCHIVE the archive name the file came from
All three are helpful for debug/verbose output. The size is independent
of the Content-Encoding and so is useful for progress
callback/filtering. The archive could be helpful if we had a mechanism
for getting all content such an archive.
The description of the new debuginfod_get_headers api is:
+.BR \%debuginfod_get_headers ()
+may be called with a debuginfod client. This function will return the
+http response headers prefixed with
+.BR X-DEBUGINFOD
+received from the first handle to get a response from a debuginfod server.
+Note that all other http headers aren't stored in the libcurl header
+callback function since they aren't of as much interest. The caller should
+copy the returned string if it is needed beyond the release of the client
object.
+The returned string may be NULL if no headers are prefixed with
+.BR X-DEBUGINFOD
+\.
Looking at the headers the headers are cleaned up when a new request is
made using the same client object (not just when the client object is
released).
Also I am not getting any headers because they seem to be turned into
lower-case by curl. e.g. from debuginfod.fedoraproject.org:
< date: Thu, 04 Aug 2022 12:56:20 GMT
< content-type: application/octet-stream
< x-debuginfod-size: 4896240
< x-debuginfod-archive:
/mnt/fedora_koji_prod/koji/packages/bash/5.1.16/2.fc36/x86_64/bash-debuginfo-5.1.16-2.fc36.x86_64.rpm
< x-debuginfod-file: /usr/lib/debug/usr/bin/bash-5.1.16-2.fc36.x86_64.debug
< last-modified: Wed, 19 Jan 2022 22:15:52 GMT
And according to the RFC "Each header field consists of a name followed by a
colon (":") and the field value. Field names are case-insensitive." So at least
in the code we always need to compare case-insensitive.
+ // X-DEBUGINFOD is 11 characters long.
+ // Some basic checks to ensure the headers received are of the expected
format
+ if ( strncmp(buffer, "X-DEBUGINFOD", 11) || buffer[numitems-1] != '\n'
+ || (buffer == strstr(buffer, ":")) ){
+return numitems;
+ }
and in debuginfod.cxx
+ // Clean winning headers to add all X-DEBUGINFOD lines to the pac
kage we'll send
+ while( (pos = header_dup.find("X-DEBUGINFOD")) != string::npos)
+{
+ // Focus on where X-DEBUGINFOD- begins
I do wonder if we should simply say that the debuginfod_get_headers is only
valid during a progress function callback. But maybe that is too restrictive.
If not there is a slight difference between calling debuginfod_get_headers
during a progress function and afterwards if there was an error during download
(then we seem to clean them). Which probably good, but should be documented.
Also I probably won't mind providing all header, at least during the progress
function callback. But you might be right that they aren't as informative.
Cheers,
Mark
[Bug libdw/29434] Memory leak in `dwarf_getscopes`
https://sourceware.org/bugzilla/show_bug.cgi?id=29434 Mark Wielaard changed: What|Removed |Added Assignee|unassigned at sourceware dot org |mark at klomp dot org Ever confirmed|0 |1 Status|UNCONFIRMED |ASSIGNED Last reconfirmed||2022-08-04 CC||mark at klomp dot org --- Comment #1 from Mark Wielaard --- Thanks. That seems correct. If in the first visit a.scopes was allocated, but an error occurs later or in the second visit a.scopes isn't freed. Except in the case of a realloc failure. It would make sense to also not dealloc in that failure case so we can always free on failure before we return the error as you suggest. BTW. Do you know what why result was negative (I assume it was negative, it could in theory also be zero, but that would be surprising if that would cause the leak)? -- You are receiving this mail because: You are on the CC list for the bug.
[Bug libdw/29434] Memory leak in `dwarf_getscopes`
https://sourceware.org/bugzilla/show_bug.cgi?id=29434 --- Comment #2 from Pablo Galindo Salgado --- > it could in theory also be zero In this case, the result was 0 (I am working with Matt) -- You are receiving this mail because: You are on the CC list for the bug.
[Bug libdw/29434] Memory leak in `dwarf_getscopes`
https://sourceware.org/bugzilla/show_bug.cgi?id=29434
--- Comment #3 from Mark Wielaard ---
(In reply to Pablo Galindo Salgado from comment #2)
> > it could in theory also be zero
>
> In this case, the result was 0 (I am working with Matt)
Interesting, I assumed it was on a failure path.
My proposed patch would have been:
diff --git a/libdw/dwarf_getscopes.c b/libdw/dwarf_getscopes.c
index 5662eecf..676d62f3 100644
--- a/libdw/dwarf_getscopes.c
+++ b/libdw/dwarf_getscopes.c
@@ -100,7 +100,7 @@ origin_match (unsigned int depth, struct Dwarf_Die_Chain
*die, void *arg)
Dwarf_Die *scopes = realloc (a->scopes, nscopes * sizeof scopes[0]);
if (scopes == NULL)
{
- free (a->scopes);
+ /* a->scopes will be freed by dwarf_getscopes on error. */
__libdw_seterrno (DWARF_E_NOMEM);
return -1;
}
@@ -198,6 +198,8 @@ dwarf_getscopes (Dwarf_Die *cudie, Dwarf_Addr pc, Dwarf_Die
**scopes)
if (result > 0)
*scopes = a.scopes;
+ else if (result < 0)
+free (a.scopes);
return result;
}
But if the result is zero I don't believe I fully understand yet how the leak
happens.
--
You are receiving this mail because:
You are on the CC list for the bug.
[Bug libdw/29434] Memory leak in `dwarf_getscopes`
https://sourceware.org/bugzilla/show_bug.cgi?id=29434 --- Comment #4 from Pablo Galindo Salgado --- The first result is already 0 in int result = __libdw_visit_scopes (0, &cu, NULL, &pc_match, &pc_record, &a); I think this is because walk_children finds no "real children" so it returns with the last return (ret < 0 ? -1 : 0;) every single time. -- You are receiving this mail because: You are on the CC list for the bug.
[Bug libdw/29434] Memory leak in `dwarf_getscopes`
https://sourceware.org/bugzilla/show_bug.cgi?id=29434 --- Comment #5 from Mark Wielaard --- (In reply to Mark Wielaard from comment #3) > But if the result is zero I don't believe I fully understand yet how the > leak happens. So I believe this may happen if we found an inlined subroutine with pc_record, but then don't find the abstract origin DIE with origin_match. That does feels like it should be an error, but (currently) isn't. Maybe this happens if the reference to the abstract origin is in a partial unit that isn't imported into the current CU. It would be really nice to have an example of the DIE tree when this happens. -- You are receiving this mail because: You are on the CC list for the bug.
[Bug libdw/29434] Memory leak in `dwarf_getscopes`
https://sourceware.org/bugzilla/show_bug.cgi?id=29434 --- Comment #6 from Pablo Galindo Salgado --- What would be the best way to provide that? -- You are receiving this mail because: You are on the CC list for the bug.
[Bug libdw/29434] Memory leak in `dwarf_getscopes`
https://sourceware.org/bugzilla/show_bug.cgi?id=29434 --- Comment #7 from Pablo Galindo Salgado --- The dwarf trie comes from /usr/lib64/libc-2.17.so in a RHEL7 system -- You are receiving this mail because: You are on the CC list for the bug.
[Bug libdw/29434] Memory leak in `dwarf_getscopes`
https://sourceware.org/bugzilla/show_bug.cgi?id=29434 --- Comment #8 from Pablo Galindo Salgado --- If you give me a patch adding print statements to libdw/dwarf_getscopes.c and libdw/libdw_visit_scopes.c I can give you the output. -- You are receiving this mail because: You are on the CC list for the bug.
Re: debuginfod Credential Helper RFC
Hi Daniel, On Tue, 2022-08-02 at 13:36 -0700, Daniel Thornburgh via Elfutils-devel wrote: > So, I put together a design with this approach, and it passed a security > review, so the approach broadly seems to work for us. > > It came up in review that it'd be considerably more usable to have the > environment variable point to a file: DEBUGINFOD_HEADERS_FILE=. This > would avoid storing credentials in environment variables, and it would > allow you to set up the path to the header file in your shell config at the > beginning of a session. > > Would this work for libdebuginfod? We'd also want to standardize on the > format of such a file; probably a newline-separated list of headers in the > format accepted by debuginfod_add_http_header()? I wonder if we should generalize that for other DEBUGINFOD_envs. But instead of adding more environment variables have a debuginfod control file like we already have for cache_clean_interval, max_unused_age and cache_miss. So as an alternative to setting any of the DEBUGINFOD_frob environment variables you could put an urls, cache_path, progress, verbose retry_limit, timeout, maxtime, maxsize or headers file under XDG_CONFIG_HOME (~/.config) debuginfod_client that would be used if the corresponding environment variable isn't set. The downside of course is that it would cause more file stats when creating a debuginfod_client handle, but the overhead is probably minimal especially if programs just reuse the debuginfod_client objects. Or maybe it should just be one control file that can have entries for all of the variables. Cheers, Mark
Re: debuginfod Credential Helper RFC
On Thu, Aug 4, 2022 at 10:02 AM Mark Wielaard wrote: > I wonder if we should generalize that for other DEBUGINFOD_envs. But > instead of adding more environment variables have a debuginfod control > file like we already have for cache_clean_interval, max_unused_age and > cache_miss. > > So as an alternative to setting any of the DEBUGINFOD_frob environment > variables you could put an urls, cache_path, progress, verbose > retry_limit, timeout, maxtime, maxsize or headers file under > XDG_CONFIG_HOME (~/.config) debuginfod_client that would be used if the > corresponding environment variable isn't set. > > The downside of course is that it would cause more file stats when > creating a debuginfod_client handle, but the overhead is probably > minimal especially if programs just reuse the debuginfod_client > objects. > > Or maybe it should just be one control file that can have entries for > all of the variables. > Generalizing our use case, configuration files help when you're looking to put permissions on part of the debuginfod configuration. Not sure if that generalizes beyond supplying headers, but maybe in the future? I could also see file-based config being useful if some aspect of the debuginfod configuration can change from moment-to-moment. Environment variables could be used for that, but it would require either changing those variables in the calling shell or wrapping each debuginfod client utility. Or maybe just if the number of environment variables grows unwieldy. For the permissions and mutable configuration cases, it would be desirable to point debuginfod at different paths, so that these could be kept in e.g. tmpfs. These might be machine generated, which also means there may be multiple sources of such files. So, something like a DEBUGINFOD_CONFIG_FILES list might work well there; each file could contain, say, a list of environment_variable=value pairs, (or a more debuginfod-specific format), and all the files in the list could be concatenated together. Then, a service set up in bashrc could append or prepend its moment-to-moment configuration runfile to the list of config files. You could also do this more granularly: DEBUGINFOD_HEADERS_FILES would work for us, and other lists could be created for other dynamically controllable aspects of the system. This wouldn't compose as well if you needed to do hackery to say, remove a source of config from the list, though, as then you'd have to find and remove n sources of config from n lists. -- Daniel Thornburgh | dth...@google.com
[Bug debuginfod/28204] extend webapi / verification with forthcoming signed-contents archives
https://sourceware.org/bugzilla/show_bug.cgi?id=28204 Ryan Goldberg changed: What|Removed |Added Attachment #14208|0 |1 is obsolete|| --- Comment #7 from Ryan Goldberg --- Created attachment 14256 --> https://sourceware.org/bugzilla/attachment.cgi?id=14256&action=edit Submit A Patch for 28204, corrections made The following patch fixes the previously mentioned issues. It passes all the tests across the try buildbots (users/rgoldber/try-bz28204) -- You are receiving this mail because: You are on the CC list for the bug.
