PR25369 slice 3/3: debuginfod header relay

Mark Wielaard mark@klomp.org
Fri Mar 27 13:49:01 GMT 2020


Hi Frank,

On Thu, 2020-03-26 at 18:54 -0400, Frank Ch. Eigler wrote:
> > Normally a user client doesn't really need to know much about whether
> > or how the file information is fetched. But in this case we have a http
> > transport specific thing (and we just added support for file:// URLs).
> > It also seems very curl specific. To use it correctly you need to know
> > the structure of HTTP request header/value and how CURLOPT_HTTPHEADER
> > uses the given string to add, replace, delete (end with a colon) or
> > create a header with no value (adding a semicolon at the end).
> 
> I don't see why this would be true.  The function is named "add", and
> the documentation says "Header: value".  This does not expose CURL
> replace/delete operations.
> 
> > I think that we should be very explicit that this API is an optional
> > hint only. That the user must not rely on the header actually being
> > used or forwarded through the transport. And that it is only to be used
> > for optional logging.
> 
> I guess I don't see a problem here.  HTTP request headers are by
> definition optional things.  How it's used --- why would we want to
> dictate "only ... for" anything?  We do what we do.

But what we do is technically just pass the given string to curl as is.
It is just that I know what people will do is read the source code and
read the CURLOPT_HTTPHEADER documentation and think they can just use
it like that. I just want it to be super clear that we reserve the
right to break someones code if they rely on the current
implementation.

> > I am especially worried that people will start using the API to
> > override and/or disable internal (curl) headers, which would mean we
> > are stuck with all kinds of curl specifics. So ideally I would like to
> > programmatically prevent that. Or at document very clearly that if
> > someone does that, that is totally unsupported and we are free to break
> > such usage in any (minor) update/bug fix release.
> 
> People who manage to muck up curl internal headers would already be
> violating our documentation ("add", "Header: value").  I just don't
> see any threat here, let alone any sort of special technical
> obligation on us to stop this or offer scare stories.  The function
> simply does what it says on the tin.

Except that people will figure out what it really does. I don't think
it is a scare story to explicitly say: "Note that the current
implementation uses libcurl, but you shouldn't rely on that fact. The
only supported usage of this method is for adding an optional header
which might or might not be passed through to the server."

> > >  void
> > >  debuginfod_end (debuginfod_client *client)
> > >  {
> > > +  if (client) /* make it safe to be invoked with NULL */
> > > +    curl_slist_free_all (client->headers);
> > >    free (client->url);
> > >    free (client);
> > >  }
> > 
> > It seems a good idea to allow debuginfod_end (NULL). But if client
> > would be NULL then the free (client->url) would also crash.
> > So just put everything under the if (client) { ... }
> 
> Or if (!client) return or whatever.

Yes, as long as the behavior is consistent.

> > >  {
> > > +  add_default_headers(client);
> > >    return debuginfod_query_server(client, build_id, build_id_len,
> > >                                   "source", filename, path);
> > >  }
> > 
> > Why not have add_default_headers at the top of the
> > debuginfod_query_server () function instead of repeating it 3 times
> > here?
> 
> Sure, OK.
> 
> 
> > So if there is any way to check whether this is overriding and/or
> > deleting an existing internal curl header here it would be really good
> > to add that here and simply reject that.
> 
> I guess I don't see why we would voluntarily undertake the
> responsibility for detailed classification of already
> documentation-violating input.  Remember, this is just a client.  If
> some user really needs this sort of goofy capability, they could have
> always just written their code to libcurl directly.  We can't stop
> them.

Sure. It is simple to write your own client code using libcurl if you
want to. And it might be too hard to sanity check the input. If it is,
too bad. But if it is easy to check then I think we should simply do
that to catch user mistakes.

> > >            struct MHD_Response *r = 0;
> > >            try
> > >              {
> > > -              r = handle_buildid (buildid, "debuginfo", "", &alt_fd);
> > > +              r = handle_buildid (0, buildid, "debuginfo", "", &alt_fd);
> > >              }
> > 
> > 0 instead of NULL?
> > I believe we went over this before, 0 is more C++?
> 
> Yes.

OK. I guess I got stuck in C language mindset.

> > > +/* Add an outgoing HTTP request  "Header: Value".  Copies string.  */
> > > +int debuginfod_add_http_header (debuginfod_client *client, const char* header);
> > 
> > So here at least add "optional header" or something. A warning that
> > this is a hint only and might be ignored depending on backend/transport
> > used.
> 
> Adding a http header to a non-http transport URL is obviously
> meaningless and harmless.  I don't see why we should belabour it or
> give people any concern about it.

I just like the documentation to be clear and precise.

Cheers,

Mark


More information about the Elfutils-devel mailing list