patch 1/2 debuginfod client

[Frank Ch. Eigler]

Hi -

> Hurrah! Documentation! Thanks.
> 
> But given that all other documentation is under doc/ could you move it
> there (guarded by DEBUGINFOD). It is just more consistent. If you leave
> it in this subdir I think you should also move the existing
> documentation files (and I think that is not worth it at the moment).

... ok, tried moving.  ... but but but that subdirectory is not ready
for rpm installations because it uses automake constructs that don't
exist ("notrans_dist_*_man*").  The notrans bit is needed because some
(and only SOME) elfutils artifacts will be renamed as a consequence of
the program-prefix manipulations.  automake has only limited support
for mix & match, and none for man pages.

So for debuginfod, the prefix stuff is mechanically overridden for the
whole directory (binaries + man pages).  That works fine.  For doc/,
if everything were in there - man pages for elfutils.3 functions (not
prefixed) and man pages for elfutils.1 binaries (prefixed), there will
be sadness and tears.  TEARS.  We'd need two doc/ directories.


> > +# automake std-options override: arrange to pass LD_LIBRARY_PATH
> > +installcheck-binPROGRAMS: $(bin_PROGRAMS)
> > +	bad=0; pid=$$$$; list="$(bin_PROGRAMS)"; for p in $$list; do \
> > +	  case ' $(AM_INSTALLCHECK_STD_OPTIONS_EXEMPT) ' in \
> > +	   *" $$p "* | *" $(srcdir)/$$p "*) continue;; \
> > +	  esac; \
> > +	  f=`echo "$$p" | \
> > +	     sed 's,^.*/,,;s/$(EXEEXT)$$//;$(transform);s/$$/$(EXEEXT)/'`; \
> > +	  for opt in --help --version; do \
> > +	    if LD_LIBRARY_PATH=$(DESTDIR)$(libdir) \
> > +	       $(DESTDIR)$(bindir)/$$f $$opt > c$${pid}_.out 2> c$${pid}_.err \
> > +		 && test -n "`cat c$${pid}_.out`" \
> > +		 && test -z "`cat c$${pid}_.err`"; then :; \
> > +	    else echo "$$f does not support $$opt" 1>&2; bad=1; fi; \
> > +	  done; \
> > +	done; rm -f c$${pid}_.???; exit $$bad
> 
> I see we also do this in src/Makefile.am but, ehe, why?

The --help / --version bit is apparently there to confirm that every
elfutils binary supports those two options at least.  Can remove if you
like.


> > +#include <fts.h>
> 
> O, o... older (glibc) fts is problematic on 32bit systems when using
> large file offsets. See libdwfl/linux-kernel-modules.c. See the BAD_FTS
> check in configure.ac
> 
>  Older glibc had a broken fts that didn't work with Large File Systems.
>  We want the version that can handler LFS, but include workaround if we
>  get a bad one. Add define to CFLAGS (not AC_DEFINE it) since we need to
>  check it before including config.h (which might define _FILE_OFFSET_BITS).

OK, copied over the debuginfod.cxx BAD_FTS stuff.


> Have you tried to build and run on i686 on Debian stable or CentOS7?

No.


> > +static const int max_build_id_bytes = 256; /* typical: 40 for gnu C toolchain */

OK, replacing this with <MAX_BUILD_ID_BYTES> throughout.


> > +  /* URL queried by this handle.  */
> > +  char url[PATH_MAX];
> 
> Are you sure that is the correct limit?
> Could it be made dynamic?

Could switch to self-allocating printfs throughout.


> > +/* Query each of the server URLs found in $DEBUGINFOD_URLS for the file
> > +   with the specified build-id, type (debuginfo, executable or source)
> > +   and filename. filename may be NULL. If found, return a file
> > +   descriptor for the target, otherwise return an error code.  */
> 
> You don't describe PATH. I assume the caller is responsible for freeing
> it? Is it set on error?

The man page covers this issue.


> > +  char cache_path[PATH_MAX];
> > +  char maxage_path[PATH_MAX*3]; /* These *3 multipliers are to shut up gcc -Wformat-truncation */
> > +  char interval_path[PATH_MAX*4];
> > +  char target_cache_dir[PATH_MAX*2];
> > +  char target_cache_path[PATH_MAX*3];
> > +  char target_cache_tmppath[PATH_MAX*4];
> > +  char suffix[PATH_MAX];
> 
> Ah, lots more uses of PATH_MAX. Now my worry is kind of the opposite.
> These are all stack allocated. Won't that blow up the stack?

Well, not that much.  We expect the web urls to be relatively short: a
couple dozen chars for buildid etc., and a source-file path, which I
suppose could be as long as PATH_MAX.  But modern glibc/gcc like to
warn when building strings out of pieces as large as PATH_MAX, even
if we are okay with truncation or whatever.

But really, alloc-on-the-fly type printf would be ideal here.  I am
pretty sure I looked for one being used in elfutils before but
couldn't find one just right.


> > +  char build_id_bytes[max_build_id_bytes * 2 + 1];
> > +
> > +  /* Copy lowercase hex representation of build_id into buf.  */
> > +  if ((build_id_len >= max_build_id_bytes) ||
> > +      (build_id_len == 0 &&
> > +       sizeof(build_id_bytes) > max_build_id_bytes*2 + 1))
> > +    return -EINVAL;
> 
> I wonder if you also want to check for a build_id that is simply too
> small. Since build_id are supposed to be globally unique it doesn't
> really make sense to have a build_id length that is too small. libdwfl
> for example needs at least 3 bytes:

Doesn't much matter ... debuginfod will reject it.


> > +  if (build_id_len == 0) /* expect clean hexadecimal */
> > +    strcpy (build_id_bytes, (const char *) build_id);
> > +  else
> > +    for (int i = 0; i < build_id_len; i++)
> > +      sprintf(build_id_bytes + (i * 2), "%02x", build_id[i]);
> 
> I would sanity check the "clean hexadecimal" == 0 case.

Possible, but since these bits are just getting transcribed straight
to a debuginfod URL, an insane unclean hex will just get rejected at
that time.  There is no lost-error case there.


> > +  /* avoid using snprintf here due to compiler warning.  */
> > +  snprintf(target_cache_dir, sizeof(target_cache_dir), "%s/%s", cache_path, build_id_bytes);
> > +  snprintf(target_cache_path, sizeof(target_cache_path), "%s/%s%s", target_cache_dir, type, suffix);
> > +  snprintf(target_cache_tmppath, sizeof(target_cache_tmppath), "%s.XXXXXX", target_cache_path);
> 
> I don't understand the comment, you are actually using snprintf?

Ah, the comment predates switching back to snprintf ... and those 
goofy big destination buffers.


> > +      /* Wait 1 second, the minimum DEBUGINFOD_TIMEOUT.  */
> > +      curl_multi_wait(curlm, NULL, 0, 1000, NULL);
> 
> Why not actually use DEBUGINFOD_TIMEOUT?

Because then we couldn't give a progress callback before that time.


> Question about writing/creating and removal of target_cache.
> It seems they rely on an environment variable. Can a user trick this
> call into overwriting some existing files? Are you scrubbing all paths
> of things like ../ ?

Given that this is a client-side library, so the user is already
running all this code under their own privilege, there is no need to
"trick".

> Just a bit concerned about weird paths, file names, URLs being set
> accidentally and the wrong files being over-written/removed.

Yeah ... if a user were to set DEBUGINFOD_CACHE_DIR to $HOME, she will
get a good thorough cleaning, or a root user were to set it to /.
PEBCAK IMHO; don't see easy ways of protecting themselves against it.


> > +/* NB: these are thread-unsafe. */
> > +__attribute__((constructor)) attribute_hidden void libdebuginfod_ctor(void)
> > +{
> > +  curl_global_init(CURL_GLOBAL_DEFAULT);
> > +}
> 
> How does this interact with a program that uses libcurl itself and also
> links with libdebuginfod?

The call is harmless if repeated.  It is described merely as
multi-thread-unsafe.


> > +/* NB: this is very thread-unsafe: it breaks other threads that are still in libcurl */
> > +__attribute__((destructor)) attribute_hidden void libdebuginfod_dtor(void)
> > +{
> > +  /* ... so don't do this: */
> > +  /* curl_global_cleanup(); */
> > +}
> 
> yeah, so something leaks, o well.

Yeah.  Well, valgrind shows everything squeaky-clean so it can't be
that bad.  TBH I'm surprised this is not done in the libcurl solib's
own ctor/dtor, where it belongs.


> > +#include <stdio.h>	source BUILDID /usr/include/stdio.h
> > +/path/to/foo.c	source BUILDID /path/to/foo.c
> > +\../bar/foo.c AT_comp_dir=/zoo	source BUILDID /zoo/../bar/foo.c
> > +.TE
> 
> Good that you give an example. This somewhat ties into my question
> above. So you don't scrub /../ normally. I am still somewhat worried
> about bogus paths to go outside of what is expected.

These are the webapi URL strings.  The cache file names are not the
same - those are specifically scrubbed with the '#' characters.


> > +.SH "SECURITY"
> > +
> > +debuginfod-find \fBdoes not\fP include any particular security
> > +features.  It trusts that the binaries returned by the debuginfod(s)
> > +are accurate.  Therefore, the list of servers should include only
> > +trustworthy ones.  If accessed across HTTP rather than HTTPS, the
> > +network should be trustworthy.
> 
> I assume libcurl handles tls certificates for https? Does that need any
> mention here?

Dunno if it's worth discussing ... the client side of https usually
does not deal with configurable certificates.


> For standalone tools, like those in src, we normally just use GPLv3+.

OK, fixed.


> > +  if (rc < 0)
> > +    {
> > +      fprintf(stderr, "Server query failed: %s\n", strerror(-rc));
> > +      return 1;
> > +    }
> 
> Is there any way we can get/print the actual URL tried here?
> That would really help the user trying to figure out what happened.

Hm, one problem here is that, while a subsequent version of
debuginfod-find does have a verbosity command line option, the code
itself does not know the URL.  That's delegated to the -client solib.


> > +#ifndef _LIBBGSERVER_CLIENT_H
> > +#define _LIBBGSERVER_CLIENT_H 1
> 
> Not that it really matters, but should that be _LIBDEBUGINFOD_CLIENT_H?

Changed.


> > +#ifdef __cplusplus
> > +extern "C" {
> > +#endif
> 
> You can also use __BEGIN_DECLS

This might be one of those things where explicitness is okay.

> > +#ifdef __cplusplus
> > +}
> > +#endif
> 
> and __END_DECLS here?

ditto.



> > +.TP 21
> > +.B DEBUGINFOD_TIMEOUT
> > +This environment variable governs the timeout for each debuginfod HTTP
> > +connection.  A server that fails to respond within this many seconds
> > +is skipped.  The default is 5.
> 
> zero isn't allowed?

I suppose that wouldn't make any sense.  Anything will take a nonzero
amount of time. :-) Maybe it could be a floating point number or
something; worth it?


> > +++ b/debuginfod/libdebuginfod.map
> > @@ -0,0 +1,7 @@
> > +ELFUTILS_0 { };
> > +ELFUTILS_0.177 {
> > +  global:
> > +  debuginfod_find_debuginfo;
> > +  debuginfod_find_executable;
> > +  debuginfod_find_source;
> > +} ELFUTILS_0;
> 
> Make that ELFUTILS_0.178 since that is the first version where they are
> added.

Yup, later typo-fixed versions of the patches on branch already fixed this.

- FChE