From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 61108 invoked by alias); 16 Nov 2019 00:46:30 -0000 Mailing-List: contact elfutils-devel-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Post: List-Help: List-Subscribe: Sender: elfutils-devel-owner@sourceware.org Received: (qmail 61091 invoked by uid 89); 16 Nov 2019 00:46:29 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Checked: by ClamAV 0.100.3 on sourceware.org X-Virus-Found: No X-Spam-SWARE-Status: No, score=-6.6 required=5.0 tests=AWL,BAYES_00,SPF_PASS autolearn=ham version=3.3.1 spammy=ssl, dunno, transcribed, installations X-Spam-Status: No, score=-6.6 required=5.0 tests=AWL,BAYES_00,SPF_PASS autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on sourceware.org X-Spam-Level: X-HELO: gnu.wildebeest.org Received: from wildebeest.demon.nl (HELO gnu.wildebeest.org) (212.238.236.112) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sat, 16 Nov 2019 00:46:27 +0000 Received: from tarox.wildebeest.org (tarox.wildebeest.org [172.31.17.39]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by gnu.wildebeest.org (Postfix) with ESMTPSA id 883FB300073F; Sat, 16 Nov 2019 01:46:24 +0100 (CET) Received: by tarox.wildebeest.org (Postfix, from userid 1000) id 36CD7405CC5A; Sat, 16 Nov 2019 01:46:24 +0100 (CET) Message-ID: <6d7430368a18c943f72bc3583efeafb2c192516f.camel@klomp.org> Subject: Re: patch 1/2 debuginfod client From: Mark Wielaard To: "Frank Ch. Eigler" Cc: elfutils-devel@sourceware.org, amerey@redhat.com Date: Sat, 16 Nov 2019 00:46:00 -0000 In-Reply-To: <20191113232456.GA31583@redhat.com> References: <20191028190438.GC14349@redhat.com> <20191028190602.GD14349@redhat.com> <9bc154bce6389be9b07f2db9dcdcc605ad4f39e3.camel@klomp.org> <20191113232456.GA31583@redhat.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Mailer: Evolution 3.28.5 (3.28.5-5.el7) Mime-Version: 1.0 X-Spam-Flag: NO X-IsSubscribed: yes X-SW-Source: 2019-q4/txt/msg00153.txt.bz2 Hi, On Wed, 2019-11-13 at 18:24 -0500, Frank Ch. Eigler wrote: > Hurrah! Documentation! Thanks. > >=20 > > 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). >=20 > ... 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. >=20 > 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=3D > program_transform_name =3D 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 +=3D debuginfod.8 > notrans_dist_man3_MANS +=3D debuginfod_find_debuginfo.3 > debuginfod_find_source.3 debuginfod_find_executable.3 > debuginfod_set_progressfn.3 > notrans_dist_man1_MANS +=3D 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=3D0; pid=3D$$$$; list=3D"$(bin_PROGRAMS)"; for p in $$list; do \ > > > + case ' $(AM_INSTALLCHECK_STD_OPTIONS_EXEMPT) ' in \ > > > + *" $$p "* | *" $(srcdir)/$$p "*) continue;; \ > > > + esac; \ > > > + f=3D`echo "$$p" | \ > > > + sed > > > 's,^.*/,,;s/$(EXEEXT)$$//;$(transform);s/$$/$(EXEEXT)/'`; \ > > > + for opt in --help --version; do \ > > > + if LD_LIBRARY_PATH=3D$(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=3D1; fi; \ > > > + done; \ > > > + done; rm -f c$${pid}_.???; exit $$bad > >=20 > > I see we also do this in src/Makefile.am but, ehe, why? >=20 > 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? >=20 > No. I'll try setting up the buildbot workers so they have all necessary libraries installed. > > > + if (build_id_len =3D=3D 0) /* expect clean hexadecimal */ > > > + strcpy (build_id_bytes, (const char *) build_id); > > > + else > > > + for (int i =3D 0; i < build_id_len; i++) > > > + sprintf(build_id_bytes + (i * 2), "%02x", build_id[i]); > >=20 > > I would sanity check the "clean hexadecimal" =3D=3D 0 case. >=20 > 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 ../ ? >=20 > 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. >=20 > 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. >=20 > > > +/* NB: these are thread-unsafe. */ > > > +__attribute__((constructor)) attribute_hidden void libdebuginfod_cto= r(void) > > > +{ > > > + curl_global_init(CURL_GLOBAL_DEFAULT); > > > +} > >=20 > > How does this interact with a program that uses libcurl itself and also > > links with libdebuginfod? >=20 > 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 threa= d sharing the same memory) is running. This doesn't just mean no other thre= ad that is using libcurl. Because curl_global_init calls functions of othe= r 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 source BUILDID /usr/include/stdio.h > > > +/path/to/foo.c source BUILDID /path/to/foo.c > > > +\../bar/foo.c AT_comp_dir=3D/zoo source BUILDID /zoo/../bar/foo.c > > > +.TE > >=20 > > 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. >=20 > 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=3D0; q { > if (filename[q] =3D=3D '\0') break; > if (filename[q] =3D=3D '/' || filename[q] =3D=3D '.') suffix[q]= =3D '#'; > else suffix[q] =3D 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. > >=20 > > I assume libcurl handles tls certificates for https? Does that need any > > mention here? >=20 > 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; > > > + } > >=20 > > 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. >=20 > 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 HT= TP > > > +connection. A server that fails to respond within this many seconds > > > +is skipped. The default is 5. > >=20 > > zero isn't allowed? >=20 > 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 =3D=3D infinity (no timeout). Cheers, Mark