[patch git] PR28284 - debuginfod x-debuginfod* header processing

[patch git] PR28284 - debuginfod x-debuginfod* header processing

[Frank Ch. Eigler]

Hi -

I had a bit of time to tweak Noah Sanci's PR28284 work, and I believe
it addresses Mark's last set of comments (from a while ago).  This
follow-up patch corrects things like case sensitivity, spacing, \r\n
processing, and tweaks documentation.

The gist of it is to add a new client api function
debuginfod_get_headers(), documented to return x-debuginfod* headers
from current or previous fetch requests.  debuginfod-find prints those
in -v verbose mode, and debuginfod relays them in federation.

This stuff is an enabler for rgoldber's subsequent
signature-passing/checking code, to which I plan to turn my attention
next.

Please see users/fche/try-pr28284d for this draft of the code.  I'd
like to keep it as two separate commits to preserve Noah's id in the
git history, even though that makes it a bit harder to give a final
review.

git diff --stat origin/master...origin/users/fche/try-pr28284d

 debuginfod/ChangeLog                     | 22 ++++++++++++++++++++++
 debuginfod/debuginfod-client.c           | 24 ++++++++++++++++++------
 debuginfod/debuginfod-find.c             |  3 +++
 debuginfod/debuginfod.cxx                | 22 ++++++++++++++++++++++
 debuginfod/debuginfod.h.in               |  4 ++++
 debuginfod/libdebuginfod.map             |  3 +++
 doc/ChangeLog                            | 10 ++++++++++
 doc/debuginfod_find_debuginfo.3          | 14 ++++++++++++++
 doc/debuginfod_get_headers.3             |  2 ++
 tests/ChangeLog                          | 12 ++++++++++++
 tests/run-debuginfod-response-headers.sh | 53 +++++++++++++++++++++++++++++++++++++++++++++--------
 11 files changed, 155 insertions(+), 14 deletions(-)


- FChE

Re: [patch git] PR28284 - debuginfod x-debuginfod* header processing

[Mark Wielaard]

Hi Frank,

On Fri, 2022-09-02 at 20:13 -0400, Frank Ch. Eigler via Elfutils-devel
wrote:
> I had a bit of time to tweak Noah Sanci's PR28284 work, and I believe
> it addresses Mark's last set of comments (from a while ago).  This
> follow-up patch corrects things like case sensitivity, spacing, \r\n
> processing, and tweaks documentation.

I hadn't thought about the \r\n at the end of the HTTP headers. Thanks.
I assume \r\n at the end of HTTP headers required, since you are
expecting in the code now, or could it also be \n on its own?

> The gist of it is to add a new client api function
> debuginfod_get_headers(), documented to return x-debuginfod* headers
> from current or previous fetch requests.

This looks good, but I think c->winning_headers needs to be
freed/cleared at the start of debuginfod_query_server. Otherwise if you
reuse the debuginfod_client and you hit the cache, the user gets the
headers from the last use of debuginfod_client that did fetch something
from a server. Which imho is confusing (the headers won't match the
cached result returned).

>   debuginfod-find prints those
> in -v verbose mode, and debuginfod relays them in federation.

This is the only thing I am not 100% happy about. It means you can only
see the headers using debuginfod-find but no longer with any other
client when DEBUGINFOD_VERBOSE is set. Is this really what we want?

> This stuff is an enabler for rgoldber's subsequent
> signature-passing/checking code, to which I plan to turn my attention
> next.
> 
> Please see users/fche/try-pr28284d for this draft of the code.  I'd
> like to keep it as two separate commits to preserve Noah's id in the
> git history, even though that makes it a bit harder to give a final
> review.

Thanks. I like this version except for those two nitpicks above. What
do you think?

Cheers,

Mark

Re: [patch git] PR28284 - debuginfod x-debuginfod* header processing

[Frank Ch. Eigler]

Hi -

Thanks for giving it a look.

> > I had a bit of time to tweak Noah Sanci's PR28284 work, and I believe
> > it addresses Mark's last set of comments (from a while ago).  This
> > follow-up patch corrects things like case sensitivity, spacing, \r\n
> > processing, and tweaks documentation.
> 
> I hadn't thought about the \r\n at the end of the HTTP headers. Thanks.
> I assume \r\n at the end of HTTP headers required, since you are
> expecting in the code now, or could it also be \n on its own?

At the wire protocol level, yes, required.  But this code strips them
from the normal C/userspace processing (and libcurl adds them back as
needed).

> > The gist of it is to add a new client api function
> > debuginfod_get_headers(), documented to return x-debuginfod* headers
> > from current or previous fetch requests.
> 
> This looks good, but I think c->winning_headers needs to be
> freed/cleared at the start of debuginfod_query_server. Otherwise if you
> reuse the debuginfod_client and you hit the cache, the user gets the
> headers from the last use of debuginfod_client that did fetch something
> from a server. Which imho is confusing (the headers won't match the
> cached result returned).

Good point, we don't want an aborted new transfer to retain records from
a previous run, will fix that.

> >   debuginfod-find prints those
> > in -v verbose mode, and debuginfod relays them in federation.
> 
> This is the only thing I am not 100% happy about. It means you can only
> see the headers using debuginfod-find but no longer with any other
> client when DEBUGINFOD_VERBOSE is set. Is this really what we want?

Well, dunno, can't speak for we all. :-) For debugging purposes (which
is what DEBUGINFOD_VERBOSE is), we could print all headers that come
in, from any server we're connecting to.  For API / human-friendly
use, limiting to x-debuginfod* ones from the winning server seems more
useful.  (I hope we don't have to rethink again once the signature
contents start coming down that pipe too - hope they're not too long.)

Will add a commit to that effect shortly and run it through the
trybots.

- FChE

Re: [patch git] PR28284 - debuginfod x-debuginfod* header processing

[Ryan Goldberg]

[-- Attachment #1: Type: text/plain, Size: 927 bytes --]

Hi all,

> Well, dunno, can't speak for we all. :-) For debugging purposes (which
> is what DEBUGINFOD_VERBOSE is), we could print all headers that come
> in, from any server we're connecting to.  For API / human-friendly
> use, limiting to x-debuginfod* ones from the winning server seems more
> useful.  (I hope we don't have to rethink again once the signature
> contents start coming down that pipe too - hope they're not too long.)
>

Just to add in here about the signature lengths, the maximum allowed
signature size is 1024B but most I've come across seem to be closer to
~250B. Something like
X-DEBUGINFOD-IMASIGNATURE:
030204262a64fc00806dd514e73e3008c3a357b93193c7e654aab193e3f72404132c9c103b7f9eb73a9a333f57ac38cba636fc3d00b5d7d5bc6ca755c4f5a1ec6c77620b2692807495b7519086372b0f81050b9f675ac3fd03da95bcd20bd21ba995804a082e99b39e2e37029190c56c6874fd19b5e668e52c0d8d3b2b79ceca26c52a3aab4ec6e483

Ryan

Re: [patch git] PR28284 - debuginfod x-debuginfod* header processing

[Mark Wielaard]

Hi Frank,

On Tue, 2022-09-06 at 12:05 -0400, Frank Ch. Eigler via Elfutils-devel
wrote:
> > This looks good, but I think c->winning_headers needs to be
> > freed/cleared at the start of debuginfod_query_server. Otherwise if you
> > reuse the debuginfod_client and you hit the cache, the user gets the
> > headers from the last use of debuginfod_client that did fetch something
> > from a server. Which imho is confusing (the headers won't match the
> > cached result returned).
> 
> Good point, we don't want an aborted new transfer to retain records
> from a previous run, will fix that.

Not just a new transfer, but also when we hit the cache before doing a
new transfer. Currently when we hit the cache and don't do any transfer
the winning_headers will point to the last http transfer which will
have nothing to do with the returned (cached) result. Just like we
clear client->url early.

Thanks,

Mark

Re: [patch git] PR28284 - debuginfod x-debuginfod* header processing

[Frank Ch. Eigler]

Hi -

> > Good point, we don't want an aborted new transfer to retain records
> > from a previous run, will fix that.
> 
> Not just a new transfer, but also when we hit the cache before doing a
> new transfer. Currently when we hit the cache and don't do any transfer
> the winning_headers will point to the last http transfer which will
> have nothing to do with the returned (cached) result. Just like we
> clear client->url early.

Got it, pushing this:

diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index 272a6a7a007f..5e5c140ab205 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -588,9 +588,11 @@ debuginfod_query_server (debuginfod_client *c,
       goto out;
     }
 
-  /* Clear the obsolete URL from a previous _find operation. */
+  /* Clear the obsolete data from a previous _find operation. */
   free (c->url);
   c->url = NULL;
+  free (c->winning_headers);
+  c->winning_headers = NULL;
 
   /* PR 27982: Add max size if DEBUGINFOD_MAXSIZE is set. */
   long maxsize = 0;

- FChE