[oe] [meta-oe][PATCH 2/3] dbus-broker: backport patches from master
Luca Boccassi
luca.boccassi at gmail.com
Mon Oct 28 10:20:13 UTC 2019
On Sat, 2019-10-26 at 08:18 +0100, Khem Raj wrote:
> On Fri, Oct 25, 2019 at 8:13 PM <
> luca.boccassi at gmail.com
> > wrote:
> > From: Luca Boccassi <
> > luca.boccassi at microsoft.com
> > >
> >
> > These patches fix issues found in Fedora 30, which switched from
> > dbus-daemon to dbus-broker.
> > These backports align meta-oe to Fedora 30.
> >
> > Signed-off-by: Luca Boccassi <
> > luca.boccassi at microsoft.com
> > >
> > ---
> > ...h-improve-error-handling-for-opendir.patch | 48 +++++++++++
> > ...he-constant-used-for-invalid-timesta.patch | 86
> > +++++++++++++++++++
> > ...s-socket-treat-MSG_CTRUNC-gracefully.patch | 83
> > ++++++++++++++++++
> > meta-oe/recipes-core/dbus/dbus-broker_21.bb | 3 +
> > 4 files changed, 220 insertions(+)
> > create mode 100644 meta-oe/recipes-core/dbus/dbus-broker/0001-
> > launch-improve-error-handling-for-opendir.patch
> > create mode 100644 meta-oe/recipes-core/dbus/dbus-broker/0002-
> > metrics-change-the-constant-used-for-invalid-timesta.patch
> > create mode 100644 meta-oe/recipes-core/dbus/dbus-broker/0003-
> > dbus-socket-treat-MSG_CTRUNC-gracefully.patch
> >
> > diff --git a/meta-oe/recipes-core/dbus/dbus-broker/0001-launch-
> > improve-error-handling-for-opendir.patch b/meta-oe/recipes-
> > core/dbus/dbus-broker/0001-launch-improve-error-handling-for-
> > opendir.patch
> > new file mode 100644
> > index 000000000..ccc175bb8
> > --- /dev/null
> > +++ b/meta-oe/recipes-core/dbus/dbus-broker/0001-launch-improve-
> > error-handling-for-opendir.patch
> > @@ -0,0 +1,48 @@
> > +From f42d5e38859c65a186acd0da94bbeeca12faf7a2 Mon Sep 17 00:00:00
> > 2001
> > +From: David Rheinsberg <
> > david.rheinsberg at gmail.com
> > >
> > +Date: Thu, 2 May 2019 17:33:34 +0200
> > +Subject: [PATCH] launch: improve error handling for opendir()
> > +
> > +This improves the error-handling of opendir() by always printing
> > +diagnostics. Furthermore, it aligns the behavior with dbus-deamon
> > and
> > +ignores EACCES.
> > +
> > +Signed-off-by: David Rheinsberg <
> > david.rheinsberg at gmail.com
> > >
> > +Upstream-Status:
> > dbus-broker at f42d5e38859c65a186acd0da94bbeeca12faf7a2
> > +---
> > + src/launch/launcher.c | 17 +++++++++++++++--
> > + 1 file changed, 15 insertions(+), 2 deletions(-)
> > +
> > +diff --git a/src/launch/launcher.c b/src/launch/launcher.c
> > +index 31a5364..2ec4bda 100644
> > +--- a/src/launch/launcher.c
> > ++++ b/src/launch/launcher.c
> > +@@ -749,10 +749,23 @@ static int
> > launcher_load_service_dir(Launcher *launcher, const char *dirpath,
> > NS
> > +
> > + dir = opendir(dirpath);
> > + if (!dir) {
> > +- if (errno == ENOENT || errno == ENOTDIR)
> > ++ if (errno == ENOENT || errno == ENOTDIR) {
> > + return 0;
> > +- else
> > ++ } else if (errno == EACCES) {
> > ++ log_append_here(&launcher->log, LOG_ERR,
> > 0, NULL);
> > ++ r = log_commitf(&launcher->log, "Access
> > denied to service directory '%s'\n", dirpath);
> > ++ if (r)
> > ++ return error_fold(r);
> > ++
> > ++ return 0;
> > ++ } else {
> > ++ log_append_here(&launcher->log, LOG_ERR,
> > errno, NULL);
> > ++ r = log_commitf(&launcher->log, "Unable
> > to open service directory '%s': %m\n", dirpath);
> > ++ if (r)
> > ++ return error_fold(r);
> > ++
> > + return error_origin(-errno);
> > ++ }
> > + }
> > +
> > + r = dirwatch_add(launcher->dirwatch, dirpath);
> > +--
> > +2.20.1
> > +
> > diff --git a/meta-oe/recipes-core/dbus/dbus-broker/0002-metrics-
> > change-the-constant-used-for-invalid-timesta.patch b/meta-
> > oe/recipes-core/dbus/dbus-broker/0002-metrics-change-the-constant-
> > used-for-invalid-timesta.patch
> > new file mode 100644
> > index 000000000..67a2dc46f
> > --- /dev/null
> > +++ b/meta-oe/recipes-core/dbus/dbus-broker/0002-metrics-change-
> > the-constant-used-for-invalid-timesta.patch
> > @@ -0,0 +1,86 @@
> > +From 3570b3e9ba367f10718b56336ce32d5254f66575 Mon Sep 17 00:00:00
> > 2001
> > +From: Tom Gundersen <
> > teg at jklm.no
> > >
> > +Date: Thu, 9 May 2019 13:00:37 +0200
> > +Subject: [PATCH] metrics: change the constant used for invalid
> > timestamps
> > +
> > +Use (uint64_t)-1 rather than 0 to indicate an invalid timestamp.
> > It
> > +should not be possible for the kernel to return 0 from
> > +clock_gettime(), but we have received some reports of our asserts
> > +triggering, so avoid the issue entirely by using -1 instead
> > (which
> > +really can never be returned).
> > +
> > +See
> > https://retrace.fedoraproject.org/faf/reports/2539484/
> >
> > +
> > +Signed-off-by: Tom Gundersen <
> > teg at jklm.no
> > >
> > +Upstream-Status:
> > dbus-broker at 3570b3e9ba367f10718b56336ce32d5254f66575
> > +---
> > + src/util/metrics.c | 8 ++++----
> > + src/util/metrics.h | 9 ++++++---
> > + 2 files changed, 10 insertions(+), 7 deletions(-)
> > +
> > +diff --git a/src/util/metrics.c b/src/util/metrics.c
> > +index b5a7182..eef94eb 100644
> > +--- a/src/util/metrics.c
> > ++++ b/src/util/metrics.c
> > +@@ -26,7 +26,7 @@ void metrics_init(Metrics *metrics, clockid_t
> > id) {
> > + }
> > +
> > + void metrics_deinit(Metrics *metrics) {
> > +- c_assert(!metrics->timestamp);
> > ++ c_assert(metrics->timestamp ==
> > METRICS_TIMESTAMP_INVALID);
> > + metrics_init(metrics, metrics->id);
> > + }
> > +
> > +@@ -82,7 +82,7 @@ void metrics_sample_add(Metrics *metrics,
> > uint64_t timestamp) {
> > + * a sample is not currently running.
> > + */
> > + void metrics_sample_start(Metrics *metrics) {
> > +- c_assert(!metrics->timestamp);
> > ++ c_assert(metrics->timestamp ==
> > METRICS_TIMESTAMP_INVALID);
> > + metrics->timestamp = metrics_get_time(metrics);
> > + }
> > +
> > +@@ -93,11 +93,11 @@ void metrics_sample_start(Metrics *metrics) {
> > + * End a currently running sample, and update the internal state.
> > + */
> > + void metrics_sample_end(Metrics *metrics) {
> > +- c_assert(metrics->timestamp);
> > ++ c_assert(metrics->timestamp !=
> > METRICS_TIMESTAMP_INVALID);
> > +
> > + metrics_sample_add(metrics, metrics->timestamp);
> > +
> > +- metrics->timestamp = 0;
> > ++ metrics->timestamp = METRICS_TIMESTAMP_INVALID;
> > + }
> > +
> > + /**
> > +diff --git a/src/util/metrics.h b/src/util/metrics.h
> > +index a8ee915..b00dee6 100644
> > +--- a/src/util/metrics.h
> > ++++ b/src/util/metrics.h
> > +@@ -8,6 +8,8 @@
> > + #include <stdlib.h>
> > + #include <time.h>
> > +
> > ++#define METRICS_TIMESTAMP_INVALID ((uint64_t) -1)
> > ++
> > + typedef struct Metrics Metrics;
> > +
> > + struct Metrics {
> > +@@ -23,9 +25,10 @@ struct Metrics {
> > + uint64_t sum_of_squares;
> > + };
> > +
> > +-#define METRICS_INIT(_id) { \
> > +- .minimum = (uint64_t) -1, \
> > +- .id = (_id), \
> > ++#define METRICS_INIT(_id) { \
> > ++ .minimum = (uint64_t) -1, \
> > ++ .id = (_id), \
> > ++ .timestamp = METRICS_TIMESTAMP_INVALID, \
> > + }
> > +
> > + void metrics_init(Metrics *metrics, clockid_t id);
> > +--
> > +2.21.0
> > +
> > diff --git a/meta-oe/recipes-core/dbus/dbus-broker/0003-dbus-
> > socket-treat-MSG_CTRUNC-gracefully.patch b/meta-oe/recipes-
> > core/dbus/dbus-broker/0003-dbus-socket-treat-MSG_CTRUNC-
> > gracefully.patch
> > new file mode 100644
> > index 000000000..53f9e71aa
> > --- /dev/null
> > +++ b/meta-oe/recipes-core/dbus/dbus-broker/0003-dbus-socket-treat-
> > MSG_CTRUNC-gracefully.patch
> > @@ -0,0 +1,83 @@
> > +From 520c47c53deeb893e03194fefaf3c5b9223ede27 Mon Sep 17 00:00:00
> > 2001
> > +From: David Rheinsberg <
> > david.rheinsberg at gmail.com
> > >
> > +Date: Fri, 10 May 2019 10:58:06 +0200
> > +Subject: [PATCH] dbus/socket: treat MSG_CTRUNC gracefully
> > +
> > +As it turns out, LSMs allow clients to trigger a MSG_CTRUNC on the
> > +remote side of a unix socket. Whenever LSMs reject the
> > transmission of
> > +an FD, they will simply drop the FD and set MSG_CTRUNC, without
> > any
> > +other error notification.
> > +
> > +Therefore, we must assume any occurance of MSG_CTRUNC is trigger
> > by a
> > +client. This makes it impossible to consider MSG_CTRUNC for any
> > other
> > +error handling, and as such we are left to disconnecting the
> > client and
> > +ignoring the flag.
> > +
> > +Luckily, MSG_CTRUNC is expected for any other event, so we only
> > used it
> > +for diagnostics so far.
> > +
> > +Signed-off-by: David Rheinsberg <
> > david.rheinsberg at gmail.com
> > >
> > +Upstream-Status:
> > dbus-broker at 520c47c53deeb893e03194fefaf3c5b9223ede27
> > +---
> > + src/dbus/socket.c | 44 +++++++++++++++++++++++++++++++++---------
> > --
> > + 1 file changed, 33 insertions(+), 11 deletions(-)
> > +
> > +diff --git a/src/dbus/socket.c b/src/dbus/socket.c
> > +index cacdff2..6e6ba10 100644
> > +--- a/src/dbus/socket.c
> > ++++ b/src/dbus/socket.c
> > +@@ -593,18 +593,40 @@ static int socket_recvmsg(Socket *socket,
> > +
> > + if (msg.msg_flags & MSG_CTRUNC) {
> > + /*
> > +- * This flag means the control-buffer was too
> > small to retrieve
> > +- * all data. If this can be triggered remotely,
> > it means a peer
> > +- * can cause us to miss FDs. Hence, we really
> > must protect
> > +- * against this.
> > +- * We do provide suitably sized buffers to be
> > prepared for any
> > +- * possible scenario. So if this happens,
> > something is fishy
> > +- * and we better report it.
> > +- * Note that this is also reported by the kernel
> > if we exceeded
> > +- * our NOFILE limit. Since this implies resource
> > +- * misconfiguration as well, we treat it the same
> > way.
> > ++ * Our control-buffer-size is carefully
> > calculated to be big
> > ++ * enough for any possible ancillary data we
> > expect. Therefore,
> > ++ * the kernel should never be required to
> > truncate it, and thus
> > ++ * MSG_CTRUNC will never be set. This is also
> > foward compatible
> > ++ * to future extensions to the ancillary data,
> > since these must
> > ++ * be enabled explicitly before the kernel
> > considers forwarding
> > ++ * them.
> > ++ *
> > ++ * Unfortunately, the SCM_RIGHTS implementation
> > might set this
> > ++ * flag as well. In particular, if not all FDs
> > can be returned
> > ++ * to user-space, MSG_CTRUNC will be set
> > (signalling that the
> > ++ * FD-set is non-complete). No other error is
> > returned or
> > ++ * signalled, though. There are several reasons
> > why the FD
> > ++ * transmission can fail. Most importantly, if we
> > exhaust our
> > ++ * FD limit, further FDs will simply be
> > discarded. We are
> > ++ * protected against this by our accounting-
> > quotas, but we
> > ++ * would still like to catch this condition and
> > warn loudly.
> > ++ * However, FDs are also dropped if the security
> > layer refused
> > ++ * the transmission of the FD in question. This
> > means, if an
> > ++ * LSM refuses the D-Bus client to send us an FD,
> > the FD is
> > ++ * just dropped and MSG_CTRUNC will be set. This
> > can be
> > ++ * triggered by clients.
> > ++ *
> > ++ * To summarize: In an ideal world, we would
> > expect this flag
> > ++ * to never be set, and we would just use
> > ++ * `error_origin(-ENOTRECOVERABLE)` to provide
> > diagnostics.
> > ++ * Unfortunately, the gross misuse of this flag
> > for LSM
> > ++ * security enforcements means we have to assume
> > any occurence
> > ++ * of MSG_CTRUNC means the client was refused to
> > send a
> > ++ * specific message. Our only possible way to
> > deal with this is
> > ++ * to disconnect the client.
> > + */
> > +- r = error_origin(-ENOTRECOVERABLE);
> > ++ socket_close(socket);
> > ++ r = SOCKET_E_LOST_INTEREST;
> > + goto error;
> > + }
> > +
> > +--
> > +2.21.0
> > +
> > diff --git a/meta-oe/recipes-core/dbus/dbus-broker_21.bb b/meta-
> > oe/recipes-core/dbus/dbus-broker_21.bb
> > index 0b0301fe0..fd77afc60 100644
> > --- a/meta-oe/recipes-core/dbus/dbus-broker_21.bb
> > +++ b/meta-oe/recipes-core/dbus/dbus-broker_21.bb
> > @@ -8,6 +8,9 @@ LIC_FILES_CHKSUM = "
> > file://LICENSE;md5=7b486c2338d225a1405d979ed2c15ce8"
> >
> >
> > SRC_URI = "
> > https://github.com/bus1/dbus-broker/releases/download/v${PV}/dbus-broker-${PV}.tar.xz
> > "
> > SRC_URI[sha256sum] =
> > "6fff9a831a514659e2c7d704e76867ce31ebcf43e8d7a62e080c6656f64cd39e"
>
> It seems to be missing md5sum also move is below SRC_URI for
> formatting sake
>
> > +SRC_URI_append += "
> > file://0001-launch-improve-error-handling-for-opendir.patch"
> >
> > +SRC_URI_append += "
> > file://0002-metrics-change-the-constant-used-for-invalid-
> > timesta.patch"
> >
> > +SRC_URI_append += "
> > file://0003-dbus-socket-treat-MSG_CTRUNC-gracefully.patch"
> >
>
> Please add it to main SRC_URI there is no need for appends then.
Hi,
Will do both in v2, thanks for the review.
--
Kind regards,
Luca Boccassi
More information about the Openembedded-devel
mailing list