Re: patch 1/2 debuginfod client

[Frank Ch. Eigler]

Hi -

> [...libdw* debuginfod_find_* calls]
> But given that they are almost similar, I would suggest to move both
> into their own file sharing most of the code to do the dlopen dance.

Where?  It can't be in the solib.  We're talking about sharing, what,
two copies of three or four lines of code (the two dlopen attempts?),
and a replacement function will not be shorter, all-in.


- FChE

Re: patch 2/2 debuginfod server etc.

[Frank Ch. Eigler]

Hi -

> > +++ b/config/debuginfod.service
> > +[Service]
> > +Group=debuginfod
> > +#CacheDirectory=debuginfod
> > +ExecStart=/usr/bin/debuginfod -d /var/cache/debuginfod/debuginfod.sqlite 
> > -p $DEBUGINFOD_PORT $DEBUGINFOD_VERBOSE $DEBUGINFOD_PATHS

> Why is CacheDirectory commented out, I cannot find it anywhere else in
> the code.

It's a placeholder for a recently added systemd capability (post-rhel7)
to avoid hardcoding /var/cache/$SUBDIR paths.


> > +++ b/config/debuginfod.sysconfig
> > +#DEBUGINFOD_VERBOSE="-v"
> > +DEBUGINFOD_PATHS="/usr/lib/debug /usr/bin /usr/sbin /usr/lib /usr/lib64 
> > /usr/local"
> 
> Should this also include /usr/libexec ?
> Isn't /usr/local too broad? Should it also include /opt and/or /srv?

Not sure how much it matters.  Added a few of them.  It's not a problem
to include a path that includes non-elf/dwarf non-rpm files; they'll be
checked only once.


> I am slightly confused about xz-devel vs liblzma.
> Could you remind me again what the relation was?
> I see we do check for liblzma with pkgconfig in configure.ac.
> So I assume the change is correct. Just confused about the naming.

Yeah, some distros put the xz libraries into one vs the other name.


> > +%if 0%{?rhel} >= 8 || 0%{?fedora} >= 20
> > +Recommends: elfutils-debuginfod-client
> > +%endif
> > +
> 
> Should we add %else Requires: elfutils-debuginfod-client?

Up to you.  Remember, we made the debuginfod client such that it was
dlopen'd into libdw(fl) because you didn't want all the
debuginfod-client (libcurl) required solibs to be loaded into the
program - or into the minimal elfutils installation footprint.  This
would undo the latter.


> Also I think it would be more correct to move the
> Recommends/Requires up with the other Requires.  (Side note, in
> fedora we actually have a separate libs subpackage, in fedora it
> should be moved there.)

OK.

> > +%package debuginfod-client
> > +Summary: Libraries and command-line frontend for HTTP ELF/DWARF file 
> > server addressed by build-id.
> > +License: GPLv2+
> 
> That should probably be:
> GPLv3+ and (GPLv2+ or LGPLv3+)
> ^ for the binary, ^ for the library

OK.

> > +%package debuginfod
> > +Summary: HTTP ELF/DWARF file server addressed by build-id.
> > +License: GPLv2+
> 
> GPLv3+

OK.

> > +BuildRequires: systemd
> > +Requires(post):   systemd
> > +Requires(preun):  systemd
> > +Requires(postun): systemd
> > +Requires: shadow-utils
> > +Requires: /usr/bin/rpm2cpio
> 
> Should that be Requires(pre): shadow-utils?
> Because you only need it it the pre-script?

Correct.

> > -%makeinstall
> > +%make_install
> 
> O. Why? What?
> Probably fine, just different.

Yeah, fedora things.


> but I think we should also split this into debuginfod-client
> with just the shared library and binary plus manpage. And debuginfod-
> client-devel with the header file, pkgconfig and other man-pages.

OK, if you don't mind the subrpm proliferation.


> Maybe there are some C++ specific warnings we could enable?

Will look into it, but wouldn't be surprised if g++ -Wall / -Wextra
includes some already.


- FChE

Re: patch 2/2 debuginfod server etc.

[Frank Ch. Eigler]

Hi -

> Like with the client manpages, I think they should go into the doc/ dir
> just because all manpages are there at the moment.

OK.

> > -bin_PROGRAMS = debuginfod-find
> > +bin_PROGRAMS = debuginfod debuginfod-find
> > +debuginfod_SOURCES = debuginfod.cxx
> > +debuginfod_cxx_CXXFLAGS = -Wno-unused-functions
> 
> Should that be debuginfod_CXXFLAGS?
> Why are there unused functions are there?

I don't think there are in debuginfod itself.

> > +# g++ 4.8 on rhel7 otherwise errors gthr-default.h / 
> > __gthrw2(__gthrw_(__pthread_key_create) undefs
> 
> Could you explain this comment a bit more?

Not sure, these might have been notes from diagnosing
the argp.h bug that messed with function attributes,
or something else.


> > +dist_man8_MANS = debuginfod.8
> 
> As stated above, I think this should be moved to doc/

OK.

> [...]
> So you give it directories with executables and directories with debug
> files. They are indexed if they have a build-id. You never provide a
> sources dir, but if something is indexed as a debug file then any
> source file referenced from it can be requested.

Yes.

> In theory you only need to provide the executable dirs, from that you
> can normally get the separate .debug files.

That's if those debug files are installed in the proper system 
location.  A build tree mid-strippinng is not like that.  And
in the case of RPMs, it's not an option at all.

> Why didn't you do that (only providing executable dirs)?
> And why do you allow sources to be found indirectly?

Because there's no need to search/index source files.  We can already
tell instantly whether they exist (when we parse the dwarf file) via a
stat(2).  We index debuginfo/source rpms for their content because
that instant check is not possible.

> Would it make sense to restrict the sources (prefix) dir?

I don't see any reason, because ...

> The questions above are simply because it makes me nervous that some,
> but not all, files that can be requested can indirectly be found
> anywhere. If you have an explicit debug dir, then it might make sense
> to also have an explicit sources dir.

If the executables we find are trusted, then the source file paths
inside are trustworthy too.  (We don't have an explicit debug dir.)


> In general I think it would be good to think about a selinux security
> context for debuginfod. This is not urgent, but I think we should see
> if we can get someone to do a security audit.

Will keep it in mind.


> One concern I have is that if a process is run in a restricted security
> context in which it might not be able to access certain files, but it
> might have access to a local socket to debuginfod, then it can get
> access to files it normally wouldn't. 

Note that the systemd service runs under an unprivileged userid.

> And if it could somehow write into any directory that debuginfod
> reads then it could construct a .debug file that points to any file
> on the system by adding it to the DWARF debug_lines file list. All
> this might be unlikely because this is all locally running
> processes. But it would be good to have someone who is a bit
> paranoid take a look if we aren't accidentally "leaking" files.

I see where you're going with it, it's a reasonable concern.  For now,
we can consider it covered by the "debuginfod should be given
trustworthy binaries" general rule.


> > +Each listed PATH also creates a thread to scan for ELF/DWARF/source
> > +files contained in matching RPMs under the given directory.  Duplicate
> > +directories are ignored.  You may use a file name for a PATH, but
> > +source code indexing may be incomplete; prefer using a directory that
> > +contains normal RPMs alongside debuginfo/debugsource RPMs.  Because of
> > +complications such as DWZ-compressed debuginfo, may require \fItwo\fP
> > +scan passes to identify all source code.
> 
> Again, this is convenient. But wouldn't it make sense to just have
> explicit paths for things like debuginfo/sources RPM containers?

Not really - these are intermingled as early as rpmbuild, and are kept
nearby in distro systems such as koji.


> > +If no PATH is listed, then \fBdebuginfod\fP will simply serve content
> > +that it scanned into its index in previous runs.
> 
> Please add a sentence explaining how to reset the index.

OK.  ("remove the database file").

> Also this means that if you change PATHs the contents from the old
> PATHS is still accessible? If so, do mention this explicitily.

OK, the doc says:

If no PATH is listed, then debuginfod will simply serve content that it
scanned into its index in previous runs: the data is cumulative.

> > +File names must match extended regular expressions given by the \-I
> > +option and not the \-X option (if any) in order to be considered.
> 
> These are global and cannot be set per PATH?

Yes.  It wouldn't be hard to make the code do this, but command line
parsing would have to be more stateful, as these options would have to
be repeated before? after? PA

[PATCH] dwelf_elf_e_machine_string: Clear errno before calling strtol

[Andreas Schwab]

Avoid spurious failure if errno is modified by any other library call in
the test.

Signed-off-by: Andreas Schwab 
---
 tests/ChangeLog| 5 +
 tests/dwelf_elf_e_machine_string.c | 1 +
 2 files changed, 6 insertions(+)

diff --git a/tests/ChangeLog b/tests/ChangeLog
index 97b8dedb..5b8a6224 100644
--- a/tests/ChangeLog
+++ b/tests/ChangeLog
@@ -1,3 +1,8 @@
+2019-11-14  Andreas Schwab  
+
+   * dwelf_elf_e_machine_string.c (main): Clear errno before calling
+   strtol.
+
 2019-09-02  Mark Wielaard  
 
* run-readelf-s.sh: Add --dyn-syms case.
diff --git a/tests/dwelf_elf_e_machine_string.c 
b/tests/dwelf_elf_e_machine_string.c
index 1df2b233..afad1058 100644
--- a/tests/dwelf_elf_e_machine_string.c
+++ b/tests/dwelf_elf_e_machine_string.c
@@ -40,6 +40,7 @@ main (int argc, char **argv)
   int em;
   const char *machine;
 
+  errno = 0;
   if (strncmp ("0x", argv[i], 2) == 0)
val = strtol (&argv[i][2], NULL, 16);
   else
-- 
2.24.0


-- 
Andreas Schwab, SUSE Labs, sch...@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

[PATCH] run-large-elf-file.sh: skip if free memory information is not available

[Andreas Schwab]

---
 tests/ChangeLog | 5 +
 tests/run-large-elf-file.sh | 4 ++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/tests/ChangeLog b/tests/ChangeLog
index 97b8dedb..f37f6cd6 100644
--- a/tests/ChangeLog
+++ b/tests/ChangeLog
@@ -1,3 +1,8 @@
+2019-11-14  Andreas Schwab  
+
+   * run-large-elf-file.sh: Skip if available memory cannot be
+   determined.
+
 2019-09-02  Mark Wielaard  
 
* run-readelf-s.sh: Add --dyn-syms case.
diff --git a/tests/run-large-elf-file.sh b/tests/run-large-elf-file.sh
index 6146cfed..cbe30615 100755
--- a/tests/run-large-elf-file.sh
+++ b/tests/run-large-elf-file.sh
@@ -42,9 +42,9 @@ if [ "x$VALGRIND_CMD" != "x" ]; then
   mem_needed=$[${mem_needed} + 2]
 fi
 echo "mem_needed: $mem_needed"
-mem_available=$(free -g | grep ^Mem: | awk -F ' +' '{print $7}')
+mem_available=$(free -g 2>/dev/null | grep ^Mem: | awk -F ' +' '{print $7}')
 echo "mem_available: $mem_available"
-if test $mem_available -lt $mem_needed; then
+if test -z "$mem_available" || test $mem_available -lt $mem_needed; then
   echo "Need at least ${mem_needed}GB free available memory"
   exit 77
 fi
-- 
2.24.0


-- 
Andreas Schwab, SUSE Labs, sch...@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: patch 2/2 debuginfod server etc.

[Mark Wielaard]

Hi,

On Mon, 2019-10-28 at 15:07 -0400, Frank Ch. Eigler wrote:
> Add the server to the debuginfod/ subdirectory.  This is a highly
> multithreaded c++11 program (still buildable on rhel7's gcc 4.8,
> which is only partly c++11 compliant).  Includes an initial suite
> of tests, man pages, and a sample systemd service.

Some comments on debuginfod.cxx.

> diff --git a/debuginfod/debuginfod.cxx b/debuginfod/debuginfod.cxx
> new file mode 100644
> index ..c9d5b271b328
> --- /dev/null
> +++ b/debuginfod/debuginfod.cxx
> @@ -0,0 +1,2501 @@
> +/* Debuginfo-over-http server.
> +   Copyright (C) 2019 Red Hat, Inc.
> +   This file is part of elfutils.
> +
> +   This file is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   elfutils is distributed in the hope that it will be useful, but
> +   WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see .  */
> +
> +
> +/* cargo-cult from libdwfl linux-kernel-modules.c */
> +/* In case we have a bad fts we include this before config.h because it
> +   can't handle _FILE_OFFSET_BITS.
> +   Everything we need here is fine if its declarations just come first.
> +   Also, include sys/types.h before fts. On some systems fts.h is not self
> +   contained. */
> +#ifdef BAD_FTS
> +  #include 
> +  #include 
> +#endif

Yeah, this is unfortunate, thanks for also adopting it for the client
code.

> +#ifdef HAVE_CONFIG_H
> +  #include "config.h"
> +#endif
> +
> +extern "C" {
> +#include "printversion.h"
> +}
> +
> +#include "debuginfod.h"
> +#include 
> +
> +#include 
> +#ifdef __GNUC__
> +#undef __attribute__ /* glibc bug - rhbz 1763325 */
> +#endif

Urgh. But yeah.

> +#include 
> +#include 
> +#include 
> +// #include  // not until it supports C++ << better
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +
> +/* If fts.h is included before config.h, its indirect inclusions may not
> +   give us the right LFS aliases of these functions, so map them manually.  
> */
> +#ifdef BAD_FTS
> +  #ifdef _FILE_OFFSET_BITS
> +#define open open64
> +#define fopen fopen64
> +  #endif
> +#else
> +  #include 
> +  #include 
> +#endif

yeah :{

> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +// #include  // on rhel7 gcc 4.8, not competent
> +#include 
> +// #include 
> +using namespace std;
> +
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#ifdef __linux__
> +#include 
> +#endif
> +
> +
> +// Roll this identifier for every sqlite schema incompatiblity.
> +#define BUILDIDS "buildids9"
> +
> +#if SQLITE_VERSION_NUMBER >= 3008000
> +#define WITHOUT_ROWID "without rowid"
> +#else
> +#define WITHOUT_ROWID ""
> +#endif
> +
> +static const char DEBUGINFOD_SQLITE_DDL[] =
> +  "pragma foreign_keys = on;\n"
> +  "pragma synchronous = 0;\n" // disable fsync()s - this cache is disposable 
> across a machine crash
> +  "pragma journal_mode = wal;\n" // https://sqlite.org/wal.html
> +  "pragma wal_checkpoint = truncate;\n" // clean out any preexisting wal file
> +  "pragma journal_size_limit = 0;\n" // limit steady state file (between 
> grooming, which also =truncate's)
> +  "pragma auto_vacuum = incremental;\n" // https://sqlite.org/pragma.html
> +  "pragma busy_timeout = 1000;\n" // https://sqlite.org/pragma.html  
> +  // NB: all these are overridable with -D option
> +
> +  // Normalization table for interning file names
> +  "create table if not exists " BUILDIDS "_files (\n"
> +  "id integer primary key not null,\n"
> +  "name text unique not null\n"
> +  ");\n"
> +  // Normalization table for interning buildids
> +  "create table if not exists " BUILDIDS "_buildids (\n"
> +  "id integer primary key not null,\n"
> +  "hex text unique not null);\n"
> +  // Track the completion of scanning of a given file & sourcetype at given 
> time
> +  "create table if not exists " BUILDIDS "_file_mtime_scanned (\n"
> +  "mtime integer not null,\n"
> +  "file integer not null,\n"
> +  "size integer not null,\n" // in bytes
> +  "sourcetype text(1) not null\n"
> +  "check (sourcetype IN ('F', 'R')),\n"
> +  "foreign key (file) references " BUILDIDS "_files(id) on update 
> cascade on delete cascade,\n"
> +  "primary key (file, mtime, sourcetype)\n"
> +  ") " WITHOUT_ROWID ";\n"
> +  "create table if not exists " BUILD

Re: [PATCH] dwelf_elf_e_machine_string: Clear errno before calling strtol

[Mark Wielaard]

On Thu, Nov 14, 2019 at 02:54:58PM +0100, Andreas Schwab wrote:
> Avoid spurious failure if errno is modified by any other library call in
> the test.

Looks correct. Pushed to master.

Thanks,

Mark

Re: [PATCH] run-large-elf-file.sh: skip if free memory information is not available

[Mark Wielaard]

On Thu, Nov 14, 2019 at 04:50:26PM +0100, Andreas Schwab wrote:
> +2019-11-14  Andreas Schwab  
> +
> + * run-large-elf-file.sh: Skip if available memory cannot be
> + determined.

Yes, good idea. When /usr/bin/free isn't installed we would otherwise
always run the test. Pushed to master.

Thanks,

Mark

P.S. You forgot the Signed-off-by line.