Re: [PATCH] debuginfod: add --listen-address option
Hi Michael, On Thu, Mar 27, 2025 at 04:43:49PM +0100, Michael Trapp wrote: > Use MHD_OPTION_SOCK_ADDR to bind the http listen socket to a single address. > The address should be an IPv4 or IPv6 address configured on the system: > --listen-address=127.0.0.1 > --listen-address=::1 > --listen-address='LOCAL_IPv4|IPv6_ADDRESS' > As debuginfod does not include any security features, a listen on the > localhost address is sufficient for a HTTP/HTTPS reverse-proxy setup. I think this looks good, but one last thing. We require contributors to use a Signed-off-by line to show they have the right to and grand others rights to use their contribution. See the CONTRIBUTING file: https://sourceware.org/cgit/elfutils/tree/CONTRIBUTING#n15 Thanks, Mark
[PATCH] debuginfod: add --listen-address option
Use MHD_OPTION_SOCK_ADDR to bind the http listen socket to a single address.
The address should be an IPv4 or IPv6 address configured on the system:
--listen-address=127.0.0.1
--listen-address=::1
--listen-address='LOCAL_IPv4|IPv6_ADDRESS'
As debuginfod does not include any security features, a listen on the
localhost address is sufficient for a HTTP/HTTPS reverse-proxy setup.
Signed-off-by: Michael Trapp
---
debuginfod/debuginfod.cxx | 115 ++
doc/debuginfod.8 | 5 ++
2 files changed, 84 insertions(+), 36 deletions(-)
diff --git a/debuginfod/debuginfod.cxx b/debuginfod/debuginfod.cxx
index 0edd57cb..8fc9426e 100644
--- a/debuginfod/debuginfod.cxx
+++ b/debuginfod/debuginfod.cxx
@@ -81,6 +81,7 @@ extern "C" {
#include
#include
#include
+#include
/* If fts.h is included before config.h, its indirect inclusions may not
@@ -481,6 +482,8 @@ static const struct argp_option options[] =
#define ARGP_KEY_METADATA_MAXTIME 0x100C
{ "metadata-maxtime", ARGP_KEY_METADATA_MAXTIME, "SECONDS", 0,
"Number of seconds to limit metadata query run time, 0=unlimited.", 0 },
+#define ARGP_KEY_HTTP_ADDR 0x100D
+ { "listen-address", ARGP_KEY_HTTP_ADDR, "ADDR", 0, "HTTP address to listen
on.", 0 },
{ NULL, 0, NULL, 0, NULL, 0 },
};
@@ -512,6 +515,8 @@ static volatile sig_atomic_t sigusr1 = 0;
static volatile sig_atomic_t forced_groom_count = 0;
static volatile sig_atomic_t sigusr2 = 0;
static unsigned http_port = 8002;
+static struct sockaddr_in6 http_sockaddr;
+static string addr_info = "";
static bool webapi_cors = false;
static unsigned rescan_s = 300;
static unsigned groom_s = 86400;
@@ -753,6 +758,16 @@ parse_opt (int key, char *arg,
requires_koji_sigcache_mapping = true;
break;
#endif
+case ARGP_KEY_HTTP_ADDR:
+ if (inet_pton(AF_INET, arg, &(((sockaddr_in*)&http_sockaddr)->sin_addr))
== 1)
+ http_sockaddr.sin6_family = AF_INET;
+ else
+ if (inet_pton(AF_INET6, arg, &http_sockaddr.sin6_addr) == 1)
+ http_sockaddr.sin6_family = AF_INET6;
+ else
+ argp_failure(state, 1, EINVAL, "listen-address");
+ addr_info = arg;
+ break;
// case 'h': argp_state_help (state, stderr,
ARGP_HELP_LONG|ARGP_HELP_EXIT_OK);
default: return ARGP_ERR_UNKNOWN;
}
@@ -5596,6 +5611,8 @@ main (int argc, char *argv[])
fdcache_prefetch = 64; // guesstimate storage is this much less costly than
re-decompression
/* Parse and process arguments. */
+ memset(&http_sockaddr, 0, sizeof(http_sockaddr));
+ http_sockaddr.sin6_family = AF_UNSPEC;
int remaining;
(void) argp_parse (&argp, argc, argv, ARGP_IN_ORDER, &remaining, NULL);
if (remaining != argc)
@@ -5702,50 +5719,75 @@ main (int argc, char *argv[])
#endif
| MHD_USE_DEBUG); /* report errors to stderr */
- // Start httpd server threads. Use a single dual-homed pool.
- MHD_Daemon *d46 = MHD_start_daemon (mhd_flags, http_port,
- NULL, NULL, /* default accept policy */
- handler_cb, NULL, /* handler callback */
- MHD_OPTION_EXTERNAL_LOGGER,
- error_cb, NULL,
- MHD_OPTION_THREAD_POOL_SIZE,
- (int)connection_pool,
- MHD_OPTION_END);
-
- MHD_Daemon *d4 = NULL;
- if (d46 == NULL)
+ MHD_Daemon *dsa = NULL,
+*d4 = NULL,
+*d46 = NULL;
+
+ if (http_sockaddr.sin6_family != AF_UNSPEC)
{
- // Cannot use dual_stack, use ipv4 only
- mhd_flags &= ~(MHD_USE_DUAL_STACK);
- d4 = MHD_start_daemon (mhd_flags, http_port,
-NULL, NULL, /* default accept policy */
+ if (http_sockaddr.sin6_family == AF_INET)
+ ((sockaddr_in*)&http_sockaddr)->sin_port = htons(http_port);
+ if (http_sockaddr.sin6_family == AF_INET6)
+ http_sockaddr.sin6_port = htons(http_port);
+ // Start httpd server threads on socket addr:port.
+ dsa = MHD_start_daemon (mhd_flags & ~MHD_USE_DUAL_STACK, http_port,
+ NULL, NULL, /* default accept policy */
handler_cb, NULL, /* handler callback */
MHD_OPTION_EXTERNAL_LOGGER,
error_cb, NULL,
-(connection_pool
- ? MHD_OPTION_THREAD_POOL_SIZE
- : MHD_OPTION_END),
-(connection_pool
- ? (int)connection_pool
- : MHD_OPTION_END),
+MHD_OPTION_SOCK_ADDR,
+(struct sockaddr *) &http_sockaddr,
+MHD_OPTION_THREAD_POOL_SIZE,
+(int)co
[PATCH] debuginfod: add --listen-address option
Use MHD_OPTION_SOCK_ADDR to bind the http listen socket to a single address.
The address should be an IPv4 or IPv6 address configured on the system:
--listen-address=127.0.0.1
--listen-address=::1
--listen-address='LOCAL_IPv4|IPv6_ADDRESS'
As debuginfod does not include any security features, a listen on the
localhost address is sufficient for a HTTP/HTTPS reverse-proxy setup.
---
debuginfod/debuginfod.cxx | 115 ++
doc/debuginfod.8 | 5 ++
2 files changed, 84 insertions(+), 36 deletions(-)
diff --git a/debuginfod/debuginfod.cxx b/debuginfod/debuginfod.cxx
index 0edd57cb..8fc9426e 100644
--- a/debuginfod/debuginfod.cxx
+++ b/debuginfod/debuginfod.cxx
@@ -81,6 +81,7 @@ extern "C" {
#include
#include
#include
+#include
/* If fts.h is included before config.h, its indirect inclusions may not
@@ -481,6 +482,8 @@ static const struct argp_option options[] =
#define ARGP_KEY_METADATA_MAXTIME 0x100C
{ "metadata-maxtime", ARGP_KEY_METADATA_MAXTIME, "SECONDS", 0,
"Number of seconds to limit metadata query run time, 0=unlimited.", 0 },
+#define ARGP_KEY_HTTP_ADDR 0x100D
+ { "listen-address", ARGP_KEY_HTTP_ADDR, "ADDR", 0, "HTTP address to listen
on.", 0 },
{ NULL, 0, NULL, 0, NULL, 0 },
};
@@ -512,6 +515,8 @@ static volatile sig_atomic_t sigusr1 = 0;
static volatile sig_atomic_t forced_groom_count = 0;
static volatile sig_atomic_t sigusr2 = 0;
static unsigned http_port = 8002;
+static struct sockaddr_in6 http_sockaddr;
+static string addr_info = "";
static bool webapi_cors = false;
static unsigned rescan_s = 300;
static unsigned groom_s = 86400;
@@ -753,6 +758,16 @@ parse_opt (int key, char *arg,
requires_koji_sigcache_mapping = true;
break;
#endif
+case ARGP_KEY_HTTP_ADDR:
+ if (inet_pton(AF_INET, arg, &(((sockaddr_in*)&http_sockaddr)->sin_addr))
== 1)
+ http_sockaddr.sin6_family = AF_INET;
+ else
+ if (inet_pton(AF_INET6, arg, &http_sockaddr.sin6_addr) == 1)
+ http_sockaddr.sin6_family = AF_INET6;
+ else
+ argp_failure(state, 1, EINVAL, "listen-address");
+ addr_info = arg;
+ break;
// case 'h': argp_state_help (state, stderr,
ARGP_HELP_LONG|ARGP_HELP_EXIT_OK);
default: return ARGP_ERR_UNKNOWN;
}
@@ -5596,6 +5611,8 @@ main (int argc, char *argv[])
fdcache_prefetch = 64; // guesstimate storage is this much less costly than
re-decompression
/* Parse and process arguments. */
+ memset(&http_sockaddr, 0, sizeof(http_sockaddr));
+ http_sockaddr.sin6_family = AF_UNSPEC;
int remaining;
(void) argp_parse (&argp, argc, argv, ARGP_IN_ORDER, &remaining, NULL);
if (remaining != argc)
@@ -5702,50 +5719,75 @@ main (int argc, char *argv[])
#endif
| MHD_USE_DEBUG); /* report errors to stderr */
- // Start httpd server threads. Use a single dual-homed pool.
- MHD_Daemon *d46 = MHD_start_daemon (mhd_flags, http_port,
- NULL, NULL, /* default accept policy */
- handler_cb, NULL, /* handler callback */
- MHD_OPTION_EXTERNAL_LOGGER,
- error_cb, NULL,
- MHD_OPTION_THREAD_POOL_SIZE,
- (int)connection_pool,
- MHD_OPTION_END);
-
- MHD_Daemon *d4 = NULL;
- if (d46 == NULL)
+ MHD_Daemon *dsa = NULL,
+*d4 = NULL,
+*d46 = NULL;
+
+ if (http_sockaddr.sin6_family != AF_UNSPEC)
{
- // Cannot use dual_stack, use ipv4 only
- mhd_flags &= ~(MHD_USE_DUAL_STACK);
- d4 = MHD_start_daemon (mhd_flags, http_port,
-NULL, NULL, /* default accept policy */
+ if (http_sockaddr.sin6_family == AF_INET)
+ ((sockaddr_in*)&http_sockaddr)->sin_port = htons(http_port);
+ if (http_sockaddr.sin6_family == AF_INET6)
+ http_sockaddr.sin6_port = htons(http_port);
+ // Start httpd server threads on socket addr:port.
+ dsa = MHD_start_daemon (mhd_flags & ~MHD_USE_DUAL_STACK, http_port,
+ NULL, NULL, /* default accept policy */
handler_cb, NULL, /* handler callback */
MHD_OPTION_EXTERNAL_LOGGER,
error_cb, NULL,
-(connection_pool
- ? MHD_OPTION_THREAD_POOL_SIZE
- : MHD_OPTION_END),
-(connection_pool
- ? (int)connection_pool
- : MHD_OPTION_END),
+MHD_OPTION_SOCK_ADDR,
+(struct sockaddr *) &http_sockaddr,
+MHD_OPTION_THREAD_POOL_SIZE,
+(int)connection_pool,
Re: [COMMITTED] libdw/libdw_findcu.c: Fix TOCTOU race condition in __libdw_findcu
Hi Aaron,
On Thu, Mar 27, 2025 at 12:07:18AM -0400, Aaron Merey wrote:
> Ensure that dwarf_lock is held before accessing next_tu_offset and
> next_cu_offset.
>
> This fixes a TOCTOU bug in __libdw_findcu that causes NULL to be
> incorrectly returned.
Could you explain what the issue is in a few more words?
I am not sure I fully follow what the lock protects here precisely.
The patch and the mention of TOCTOU seem to indicate it is about this in
particular:
struct Dwarf_CU **found = eu_tfind (&fake, tree, findcu_cb);
if (found != NULL)
return *found;
Does that mean that despite eu_tfind internally taking a lock on the
tree lock, the returned result can still change after the call? If
that is the case how exactly does that happen?
Or isn't this the part that needs guarding?
Asking because if it is this eu_tfind check return != NULL construct
then there are more places that might need extra guarding (or we have
to write better eu-search wrappers?)
Thanks,
Mark
> Signed-off-by: Aaron Merey
> ---
> libdw/libdw_findcu.c | 9 ++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/libdw/libdw_findcu.c b/libdw/libdw_findcu.c
> index 8805af9b..0e4dcc37 100644
> --- a/libdw/libdw_findcu.c
> +++ b/libdw/libdw_findcu.c
> @@ -240,6 +240,8 @@ struct Dwarf_CU *
> internal_function
> __libdw_findcu (Dwarf *dbg, Dwarf_Off start, bool v4_debug_types)
> {
> + mutex_lock (dbg->dwarf_lock);
> +
>search_tree *tree = v4_debug_types ? &dbg->tu_tree : &dbg->cu_tree;
>Dwarf_Off *next_offset
> = v4_debug_types ? &dbg->next_tu_offset : &dbg->next_cu_offset;
> @@ -249,9 +251,10 @@ __libdw_findcu (Dwarf *dbg, Dwarf_Off start, bool
> v4_debug_types)
>struct Dwarf_CU **found = eu_tfind (&fake, tree, findcu_cb);
>struct Dwarf_CU *result = NULL;
>if (found != NULL)
> -return *found;
> -
> - mutex_lock (dbg->dwarf_lock);
> +{
> + mutex_unlock (dbg->dwarf_lock);
> + return *found;
> +}
>
>if (start < *next_offset)
> __libdw_seterrno (DWARF_E_INVALID_DWARF);
> --
> 2.49.0
>
Re: [COMMITTED] libdw/libdw_findcu.c: Fix TOCTOU race condition in __libdw_findcu
Hi Mark,
On Thu, Mar 27, 2025 at 4:51 AM Mark Wielaard wrote:
>
> Hi Aaron,
>
> On Thu, Mar 27, 2025 at 12:07:18AM -0400, Aaron Merey wrote:
> > Ensure that dwarf_lock is held before accessing next_tu_offset and
> > next_cu_offset.
> >
> > This fixes a TOCTOU bug in __libdw_findcu that causes NULL to be
> > incorrectly returned.
>
> Could you explain what the issue is in a few more words?
>
> I am not sure I fully follow what the lock protects here precisely.
> The patch and the mention of TOCTOU seem to indicate it is about this in
> particular:
>
> struct Dwarf_CU **found = eu_tfind (&fake, tree, findcu_cb);
> if (found != NULL)
> return *found;
>
> Does that mean that despite eu_tfind internally taking a lock on the
> tree lock, the returned result can still change after the call? If
> that is the case how exactly does that happen?
Multiple threads might call eu_tfind before the desired Dwarf_CU has
been added to the search_tree. In this case `found == NULL` for both
threads. The first thread to grab the mutex will successfully find the
Dwarf_CU, add it to the search_tree and return it. For the second
thread, `if (start < *next_offset)` will evaluate to true because the
first thread will have already updated *next_offset. __libdw_seterrno
(DWARF_E_INVALID_DWARF) gets set and NULL is returned.
>
> Or isn't this the part that needs guarding?
Yes this part needs guarding. The mutex_lock could be moved from the
beginning of __libdw_findcu to immediately before the call to
eu_tfind.
> Asking because if it is this eu_tfind check return != NULL construct
> then there are more places that might need extra guarding (or we have
> to write better eu-search wrappers?)
Yes there are other eu_tfind calls that are susceptible to this bug.
If eu_tfind returns NULL, a lock should continue to be held until the
thread has a chance to add the result to the search_tree. I will work
on a patch to improve the eu-search wrappers.
Aaron
>
> Thanks,
>
> Mark
>
> > Signed-off-by: Aaron Merey
> > ---
> > libdw/libdw_findcu.c | 9 ++---
> > 1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/libdw/libdw_findcu.c b/libdw/libdw_findcu.c
> > index 8805af9b..0e4dcc37 100644
> > --- a/libdw/libdw_findcu.c
> > +++ b/libdw/libdw_findcu.c
> > @@ -240,6 +240,8 @@ struct Dwarf_CU *
> > internal_function
> > __libdw_findcu (Dwarf *dbg, Dwarf_Off start, bool v4_debug_types)
> > {
> > + mutex_lock (dbg->dwarf_lock);
> > +
> >search_tree *tree = v4_debug_types ? &dbg->tu_tree : &dbg->cu_tree;
> >Dwarf_Off *next_offset
> > = v4_debug_types ? &dbg->next_tu_offset : &dbg->next_cu_offset;
> > @@ -249,9 +251,10 @@ __libdw_findcu (Dwarf *dbg, Dwarf_Off start, bool
> > v4_debug_types)
> >struct Dwarf_CU **found = eu_tfind (&fake, tree, findcu_cb);
> >struct Dwarf_CU *result = NULL;
> >if (found != NULL)
> > -return *found;
> > -
> > - mutex_lock (dbg->dwarf_lock);
> > +{
> > + mutex_unlock (dbg->dwarf_lock);
> > + return *found;
> > +}
> >
> >if (start < *next_offset)
> > __libdw_seterrno (DWARF_E_INVALID_DWARF);
> > --
> > 2.49.0
> >
>
