Re: [PATCH] debuginfod: PR27917 - protect against federation loops
Hey Frank,
1) moved the XFF check to handle_buildid.
2) replace "livelock" with "deadlock" in the commit message.
- dichen
On Thu, Aug 19, 2021 at 6:55 AM Frank Ch. Eigler wrote:
> Hi -
>
> > This patch aims to reduce the risk by adding an option to debuginfod
> > that functions kind of like an IP packet's TTL: a limit on the
> > length of XFF: header that debuginfod is willing to process. If
> > X-Forwarded-For: exceeds N hops, it will not delegate a local lookup
> > miss to upstream debuginfods. [...]
>
> Thank you very much!
>
>
> > Commit ab38d167c40c99 causes federation loops for non-existent
> > resources to result in multiple temporary livelocks, each lasting
> > for $DEBUGINFOD_TIMEOUT seconds. [...]
>
> (FWIW, the term "livelock" is not quite right here, try just
> "deadlock".)
>
> The patch looks functional, and thank you also for including the
> docs and test case. Thorough enough!
>
>
> > @@ -1862,6 +1869,12 @@ handle_buildid (MHD_Connection* conn,
> >// We couldn't find it in the database. Last ditch effort
> >// is to defer to other debuginfo servers.
> >
> > + // if X-Forwarded-For: exceeds N hops,
> > + // do not delegate a local lookup miss to upstream debuginfods.
> > + if (disable_query_server)
> > +throw reportable_exception(MHD_HTTP_NOT_FOUND, "not found,
> > --forwared-ttl-limit reached \
> > +and will not query the upstream servers");
>
> One part I don't understand is why you added the code to check for XFF
> length into handler_cb(), and then passed the disable_query_server
> result flag to this function. Was there some reason not to perform
> the XFF comma-counting right here?
>
>
> - FChE
>
>
From a0b3d4ba3d15b83d23d3594b614c8e72b87e626c Mon Sep 17 00:00:00 2001
From: Di Chen
Date: Fri, 20 Aug 2021 13:03:21 +0800
Subject: [PATCH] debuginfod: PR27917 - protect against federation loops
If someone misconfigures a debuginfod federation to have loops, and
a nonexistent buildid lookup is attempted, bad things will happen,
as is documented.
This patch aims to reduce the risk by adding an option to debuginfod
that functions kind of like an IP packet's TTL: a limit on the length of
XFF: header that debuginfod is willing to process. If X-Forwarded-For:
exceeds N hops, it will not delegate a local lookup miss to upstream
debuginfods.
Commit ab38d167c40c99 causes federation loops for non-existent resources
to result in multiple temporary deadlocks, each lasting for $DEBUGINFOD_TIMEOUT
seconds. Since concurrent requests for each unique resource are now
serialized, federation loops can result in one server thread waiting to
acquire a lock while the server thread holding the lock waits for the
first thread to respond to an http request.
This PR can help protect against the above multiple temporary deadlocks
behaviour. Ex. if --forwarded-ttl-limit=0 then the timeout behaviour of
local loops should be avoided.
https://sourceware.org/bugzilla/show_bug.cgi?id=27917
Signed-off-by: Di Chen
---
debuginfod/debuginfod.cxx| 18
doc/debuginfod.8 | 6 ++
tests/run-debuginfod-find.sh | 42 +++-
3 files changed, 65 insertions(+), 1 deletion(-)
diff --git a/debuginfod/debuginfod.cxx b/debuginfod/debuginfod.cxx
index 4ddd9255..261049f2 100644
--- a/debuginfod/debuginfod.cxx
+++ b/debuginfod/debuginfod.cxx
@@ -375,6 +375,8 @@ static const struct argp_option options[] =
#define ARGP_KEY_FDCACHE_PREFETCH_FDS 0x1006
{ "fdcache-prefetch-fds", ARGP_KEY_FDCACHE_PREFETCH_FDS, "NUM", 0,"Number of files allocated to the \
prefetch cache.", 0},
+#define ARGP_KEY_FORWARDED_TTL_LIMIT 0x1007
+ {"forwarded-ttl-limit", ARGP_KEY_FORWARDED_TTL_LIMIT, "NUM", 0, "Limit of X-Forwarded-For hops, default 8.", 0},
{ NULL, 0, NULL, 0, NULL, 0 },
};
@@ -422,6 +424,7 @@ static long fdcache_prefetch;
static long fdcache_mintmp;
static long fdcache_prefetch_mbs;
static long fdcache_prefetch_fds;
+static unsigned forwarded_ttl_limit = 8;
static string tmpdir;
static void set_metric(const string& key, double value);
@@ -554,6 +557,9 @@ parse_opt (int key, char *arg,
if( fdcache_mintmp > 100 || fdcache_mintmp < 0 )
argp_failure(state, 1, EINVAL, "fdcache mintmp percent");
break;
+case ARGP_KEY_FORWARDED_TTL_LIMIT:
+ forwarded_ttl_limit = (unsigned) atoi(arg);
+ break;
case ARGP_KEY_ARG:
source_paths.insert(string(arg));
break;
@@ -1881,6 +1887,17 @@ handle_buildid (MHD_Connection* conn,
if (xff != "")
xff += string(", "); // comma separated list
+ unsigned int xff_count = 0;
+ for (auto&& i : xff){
+if (i == ',') xff_count++;
+ }
+
+ // if X-Forwarded-For: exceeds N hops,
+ // do not delegate a local lookup miss to upstream debuginfods.
+ if (xff_count >= forwarded_ttl_limit)
+throw reportable_exception(MHD_HTTP_NOT_FOUND, "not found, --forwared-ttl-lim
obv patch: debuginfod-client-config.7 man page packaging
Hi -
Pushing momentarily:
commit dc6ddd8ba061c0486a2c4b5a17ddd086d5e3a32d (HEAD -> master)
Author: Frank Ch. Eigler
Date: Fri Aug 20 13:54:55 2021 -0400
PR27950 - package new debuginfod-client-config.7 man page in rpm
The template rpm spec file needs to include the new .7 page in all the
subrpms whose man pages may .so it.
Signed-off-by: Frank Ch. Eigler
diff --git a/config/elfutils.spec.in b/config/elfutils.spec.in
index 5552352b9875..043762653c90 100644
--- a/config/elfutils.spec.in
+++ b/config/elfutils.spec.in
@@ -299,12 +299,14 @@ fi
%{_libdir}/libdebuginfod.so.*
%{_bindir}/debuginfod-find
%{_mandir}/man1/debuginfod-find.1*
+%{_mandir}/man7/debuginfod*.7*
%config(noreplace) %{_sysconfdir}/profile.d/*
%files debuginfod-client-devel
%defattr(-,root,root)
%{_libdir}/pkgconfig/libdebuginfod.pc
%{_mandir}/man3/debuginfod_*.3*
+%{_mandir}/man7/debuginfod*.7*
%{_includedir}/elfutils/debuginfod.h
%{_libdir}/libdebuginfod.so
@@ -315,6 +317,7 @@ fi
%{_unitdir}/debuginfod.service
%{_sysconfdir}/sysconfig/debuginfod
%{_mandir}/man8/debuginfod.8*
+%{_mandir}/man7/debuginfod*.7*
%dir %attr(0700,debuginfod,debuginfod) %{_localstatedir}/cache/debuginfod
%ghost %attr(0600,debuginfod,debuginfod)
%{_localstatedir}/cache/debuginfod/debuginfod.sqlite
[PATCH] lib: remove usage of `sys/cdefs.h`
This header is a BSD header that is also available in glibc. However, this is a not a standard C header and was used for `__CONCAT`. Because this is not a standard header, not all libc implementations provide the header. Remove the usage of the header and always use the previously fallback path. This is needed in order to build with musl. Signed-off-by: Saleem Abdulrasool --- lib/fixedsizehash.h | 5 - 1 file changed, 5 deletions(-) diff --git a/lib/fixedsizehash.h b/lib/fixedsizehash.h index dac2a5f5..f333ad99 100644 --- a/lib/fixedsizehash.h +++ b/lib/fixedsizehash.h @@ -30,17 +30,12 @@ #include #include #include -#include #include -#ifdef __CONCAT -#define CONCAT(t1,t2) __CONCAT (t1,t2) -#else #define STROF(t2) t2 #define CONCAT_EXPANDED(t1,t2) t1 ## t2 #define CONCAT(t1,t2) CONCAT_EXPANDED(t1,t2) -#endif /* Before including this file the following macros must be defined: -- 2.33.0.rc2.250.ged5fa647cd-goog
[PATCH] debuginfod, elfclassify: remove unnecessary header inclusion
`error.h`'s inclusion was centralised into the `system.h` header. As
the implementation currently includes `system.h` already, the inclusion
of `error.h` is unnecessary. This prepares for a future portability
change to allow elfutil to build with alternate libc implementations.
Signed-off-by: Saleem Abdulrasool
---
debuginfod/debuginfod.cxx | 1 -
src/elfclassify.c | 1 -
2 files changed, 2 deletions(-)
diff --git a/debuginfod/debuginfod.cxx b/debuginfod/debuginfod.cxx
index fca07f61..b560fdcb 100644
--- a/debuginfod/debuginfod.cxx
+++ b/debuginfod/debuginfod.cxx
@@ -46,7 +46,6 @@ extern "C" {
#include
#include
-#include
#include
#include
#include
diff --git a/src/elfclassify.c b/src/elfclassify.c
index fe7d..2f70b29a 100644
--- a/src/elfclassify.c
+++ b/src/elfclassify.c
@@ -19,7 +19,6 @@
#include
#include
-#include
#include
#include
#include
--
2.33.0.rc2.250.ged5fa647cd-goog
[PATCH] handle libc implemntations which do not provide `error.h`
Introduce a configure time check for the presence of `error.h`. In the
case that `error.h` is not available, we can fall back to `err.h`.
Although `err.h` is not a C standard header (it is a BSD extension),
many libc implementations provide. If there are targets which do not
provide an implementation of `err.h`, it would be possible to further
extend the implementation to be more portable.
This resolves PR21008.
Signed-off-by: Saleem Abdulrasool
---
configure.ac | 3 +++
lib/system.h | 23 ++-
2 files changed, 25 insertions(+), 1 deletion(-)
diff --git a/configure.ac b/configure.ac
index 7caff2c5..177bb1a2 100644
--- a/configure.ac
+++ b/configure.ac
@@ -431,6 +431,9 @@ AC_CHECK_DECLS([reallocarray],[],[],
AC_CHECK_FUNCS([process_vm_readv])
+AC_CHECK_HEADERS([error.h])
+AC_CHECK_HEADERS([err.h])
+
old_CFLAGS="$CFLAGS"
CFLAGS="$CFLAGS -D_GNU_SOURCE"
AC_FUNC_STRERROR_R()
diff --git a/lib/system.h b/lib/system.h
index 58d9deee..8adb5848 100644
--- a/lib/system.h
+++ b/lib/system.h
@@ -29,8 +29,9 @@
#ifndef LIB_SYSTEM_H
#define LIB_SYSTEM_H 1
+#include
+
#include
-#include
#include
#include
#include
@@ -40,6 +41,26 @@
#include
#include
+#if defined(HAVE_ERROR_H)
+#include
+#elif defined(HAVE_ERR_H)
+static int error_message_count = 0;
+
+static inline void error(int status, int errnum, const char *format, ...) {
+ va_list argp;
+
+ va_start(argp, format);
+ verr(status, format, argp);
+ va_end(argp);
+
+ if (status)
+exit(status);
+ ++error_message_count;
+}
+#else
+#error "err.h or error.h must be available"
+#endif
+
#if __BYTE_ORDER == __LITTLE_ENDIAN
# define LE32(n) (n)
# define LE64(n) (n)
--
2.33.0.rc2.250.ged5fa647cd-goog
Re: [PATCH] handle libc implemntations which do not provide `error.h`
Hi,
Sorry, this change is missing a modification that I seemed to have
left out accidentally. I missed the inclusion of `err.h` in the
`HAVE_ERR_H` when I was staging the changes locally. I'll send out a
new version of this patch.
On Fri, Aug 20, 2021 at 11:23 AM Saleem Abdulrasool wrote:
>
> Introduce a configure time check for the presence of `error.h`. In the
> case that `error.h` is not available, we can fall back to `err.h`.
> Although `err.h` is not a C standard header (it is a BSD extension),
> many libc implementations provide. If there are targets which do not
> provide an implementation of `err.h`, it would be possible to further
> extend the implementation to be more portable.
>
> This resolves PR21008.
>
> Signed-off-by: Saleem Abdulrasool
> ---
> configure.ac | 3 +++
> lib/system.h | 23 ++-
> 2 files changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/configure.ac b/configure.ac
> index 7caff2c5..177bb1a2 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -431,6 +431,9 @@ AC_CHECK_DECLS([reallocarray],[],[],
>
> AC_CHECK_FUNCS([process_vm_readv])
>
> +AC_CHECK_HEADERS([error.h])
> +AC_CHECK_HEADERS([err.h])
> +
> old_CFLAGS="$CFLAGS"
> CFLAGS="$CFLAGS -D_GNU_SOURCE"
> AC_FUNC_STRERROR_R()
> diff --git a/lib/system.h b/lib/system.h
> index 58d9deee..8adb5848 100644
> --- a/lib/system.h
> +++ b/lib/system.h
> @@ -29,8 +29,9 @@
> #ifndef LIB_SYSTEM_H
> #define LIB_SYSTEM_H 1
>
> +#include
> +
> #include
> -#include
> #include
> #include
> #include
> @@ -40,6 +41,26 @@
> #include
> #include
>
> +#if defined(HAVE_ERROR_H)
> +#include
> +#elif defined(HAVE_ERR_H)
> +static int error_message_count = 0;
> +
> +static inline void error(int status, int errnum, const char *format, ...) {
> + va_list argp;
> +
> + va_start(argp, format);
> + verr(status, format, argp);
> + va_end(argp);
> +
> + if (status)
> +exit(status);
> + ++error_message_count;
> +}
> +#else
> +#error "err.h or error.h must be available"
> +#endif
> +
> #if __BYTE_ORDER == __LITTLE_ENDIAN
> # define LE32(n) (n)
> # define LE64(n) (n)
> --
> 2.33.0.rc2.250.ged5fa647cd-goog
>
[PATCH v2] handle libc implemntations which do not provide `error.h`
Introduce a configure time check for the presence of `error.h`. In the
case that `error.h` is not available, we can fall back to `err.h`.
Although `err.h` is not a C standard header (it is a BSD extension),
many libc implementations provide. If there are targets which do not
provide an implementation of `err.h`, it would be possible to further
extend the implementation to be more portable.
This resolves bug #21008.
Signed-off-by: Saleem Abdulrasool
---
configure.ac | 3 +++
lib/system.h | 26 +-
2 files changed, 28 insertions(+), 1 deletion(-)
diff --git a/configure.ac b/configure.ac
index 7caff2c5..177bb1a2 100644
--- a/configure.ac
+++ b/configure.ac
@@ -431,6 +431,9 @@ AC_CHECK_DECLS([reallocarray],[],[],
AC_CHECK_FUNCS([process_vm_readv])
+AC_CHECK_HEADERS([error.h])
+AC_CHECK_HEADERS([err.h])
+
old_CFLAGS="$CFLAGS"
CFLAGS="$CFLAGS -D_GNU_SOURCE"
AC_FUNC_STRERROR_R()
diff --git a/lib/system.h b/lib/system.h
index 58d9deee..b963fd15 100644
--- a/lib/system.h
+++ b/lib/system.h
@@ -29,8 +29,9 @@
#ifndef LIB_SYSTEM_H
#define LIB_SYSTEM_H 1
+#include
+
#include
-#include
#include
#include
#include
@@ -38,8 +39,31 @@
#include
#include
#include
+#include
#include
+#if defined(HAVE_ERROR_H)
+#include
+#elif defined(HAVE_ERR_H)
+#include
+
+static int error_message_count = 0;
+
+static inline void error(int status, int errnum, const char *format, ...) {
+ va_list argp;
+
+ va_start(argp, format);
+ verr(status, format, argp);
+ va_end(argp);
+
+ if (status)
+exit(status);
+ ++error_message_count;
+}
+#else
+#error "err.h or error.h must be available"
+#endif
+
#if __BYTE_ORDER == __LITTLE_ENDIAN
# define LE32(n) (n)
# define LE64(n) (n)
--
2.33.0.rc2.250.ged5fa647cd-goog
