Re: patch 1/2 debuginfod client

[Mark Wielaard <mark@klomp.org>]

Hi,

On Wed, 2019-11-13 at 18:24 -0500, Frank Ch. Eigler wrote:
> 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.

O, I missed that you have this:

> # Disable eu- prefixing for artifacts (binaries & man pages) in this
> # directory, since they do not conflict with binutils tools.
> program_prefix=
> program_transform_name = s,x,x,

That does complicate things a bit indeed.
Why not simply keep the program prefix as is?

It also seems that how you did it on the branch:

> if DEBUGINFOD
> notrans_dist_man8_MANS += debuginfod.8
> notrans_dist_man3_MANS += debuginfod_find_debuginfo.3
> debuginfod_find_source.3 debuginfod_find_executable.3
> debuginfod_set_progressfn.3
> notrans_dist_man1_MANS += debuginfod-find.1
> endif

actually works for me. After an make install I get:

/opt/local/install/elfutils/share/man/man1:
debuginfod-find.1  eu-elfclassify.1  eu-readelf.1

/opt/local/install/elfutils/share/man/man3:
debuginfod_find_debuginfo.3   debuginfod_set_progressfn.3  elf_getdata.
3
debuginfod_find_executable.3  elf_begin.3                  elf_update.3
debuginfod_find_source.3      elf_clone.3

/opt/local/install/elfutils/share/man/man8:
debuginfod.8

> > > +# 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.

Very odd. That is actually what make distcheck would do. automake
normally generates that rule, but without the LD_LIBRARY_PATH set.
I tried various ways to just use what automake (std-options) generates.
But I couldn't get it working. Maybe it requires we also use libtool,
which we currently don't.

Lets keep it as is for now.

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

I'll try setting up the buildbot workers so they have all necessary
libraries installed.

> > > +  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.

But since the user won't see the URL generated they might not notice
what is really going on. They will just see that something wasn't
found, won't they?

> > 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.

It might be unlikely the user accidentally set the environment
variables to something odd, but they might be tricked into running it
on a debug file that was doctored. Which might produce lookups for odd
source files. It might even just be a buggy compiler that created a few
../.. too many. It would be bad if that would cause havoc.

> 
> > > +/* 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.

It also says:

   This function is not thread safe.
   You must not call it when any other thread in the program (i.e. a  thread sharing the same memory) is running. This doesn't just mean no other thread that is using libcurl.  Because curl_global_init calls functions of other libraries that are similarly thread unsafe,
   it could conflict with any other thread that uses these other libraries.

That doesn't make me very happy.
It looks like using libcurl from another library is not really very
safe if the program or another library it links against are also
libcurl users.

Do you know how other libraries that use libcurl deal with this?

> > > +#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.

Aha. That is done by this code:

>       /* copy the filename to suffix, s,/,#,g */
>       for (q=0; q<sizeof(suffix)-1; q++)
>         {
>           if (filename[q] == '\0') break;
>           if (filename[q] == '/' || filename[q] == '.') suffix[q] = '#';
>           else suffix[q] = filename[q];
>         }

Why do you also replace '.' with '#'?
That seems unnecessary.

What about files that already contain '#' chars?
Wouldn't something like /foo/bar#/baz clash with /foo/bar/#baz?
Or do you just think that is just completely unlikely to ever occur?

> > > +.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.

But the client side might or might not verify the server side ssl
certificate. Does it do that? And if so, which trusted roots does it
use? And can that be disabled or changed?

> > > +  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.

Maybe if we had a debuginfo-client request object it could store the
URLs tried...? :)

> > > +.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?

I was more thinking zero == infinity (no timeout).

Cheers,

Mark