From e5bdc6759195dbcfc4e7dcb02bf59190a3debe06 Mon Sep 17 00:00:00 2001

From: Laszlo Ersek <lersek@redhat.com>

Date: Wed, 9 Feb 2022 13:14:54 +0100

Subject: [PATCH] loader: work around lstat()/EACCES regression in

 _g_local_file_info_get()

In glib commit 71e7b5800a31 ("Handle MLS selinux policy better",

2010-07-08), which was made for

<https://bugzilla.gnome.org/show_bug.cgi?id=623692>, an lstat() failure

with error code EACCES was *masked* in function _g_local_file_info_get().

Consequently, if osinfo_loader_find_files() calls g_file_query_info() on a

file that is inaccessible due to (e.g.) a missing "x" (search) permission

on a leading directory, then g_file_query_info() succeeds, our

"skipMissing" branch is dead, g_file_info_get_attribute_uint32() is

reached, and it returns G_FILE_TYPE_UNKNOWN.

As a consequence, the outer osinfo_loader_process_default_path() function

can fail, even though it passes skipMissing=TRUE to

osinfo_loader_process_list(). Example:

> $ HOME=/root \

>   OSINFO_SYSTEM_DIR=/usr/share/osinfo \

>   build/tools/osinfo-query os

> Error loading OS data: Can't read path /root/.config/osinfo

Arguably, this situation should be handled by simply skipping the

inaccessible path, as if all leading directories could be searched, and

only the last pathname compontent (the filename entry) didn't exist in its

direct parent directory.

The glib regression was reported in 2017:

  https://bugzilla.gnome.org/show_bug.cgi?id=777187

and then migrated to gitlab:

  https://gitlab.gnome.org/GNOME/glib/-/issues/1237

but it's still not solved today.

Work around the issue by honoring "skipMissing" on the G_FILE_TYPE_UNKNOWN

branch. Demonstration:

> $ HOME=/root \

>   OSINFO_SYSTEM_DIR=/usr/share/osinfo \

>   build/tools/osinfo-query os

>

> ** (osinfo-query:9924): WARNING **: 13:23:12.776: Can't read path /root/.config/osinfo

>  Short ID       | Name             | Version | ID

> ----------------+------------------+---------+----------------------------------------

>  alpinelinux3.5 | Alpine Linux 3.5 | 3.5     | http://alpinelinux.org/alpinelinux/3.5

> ...

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2051559

Signed-off-by: Laszlo Ersek <lersek@redhat.com>

---

 osinfo/osinfo_loader.c |  15 ++++++

 tests/test-loader.c    | 105 +++++++++++++++++++++++++++++++++++++++++

 2 files changed, 120 insertions(+)

diff --git a/osinfo/osinfo_loader.c b/osinfo/osinfo_loader.c

index 96ca6ee..e244b3f 100644

--- a/osinfo/osinfo_loader.c

+++ b/osinfo/osinfo_loader.c

@@ -2377,6 +2377,21 @@ static void osinfo_loader_find_files(OsinfoLoader *loader,

     } else if (type == G_FILE_TYPE_UNKNOWN) {

         g_autofree gchar *path = g_file_get_path(file);

         g_autofree gchar *msg = g_strdup_printf("Can't read path %s", path);

+        if (skipMissing) {

+            /* This is a work-around for

+             * <https://gitlab.gnome.org/GNOME/glib/-/issues/1237>. If the

+             * lstat() call underlying our g_file_query_info() call at the top

+             * of this function fails for "path" with EACCES, then

+             * g_file_query_info() should fail, and the "skipMissing" branch up

+             * there should suppress the error and return cleanly.

+             * Unfortunately, _g_local_file_info_get() masks the lstat()

+             * failure, g_file_info_get_attribute_uint32() is reached above,

+             * and returns G_FILE_TYPE_UNKNOWN for the file that could never be

+             * accessed. So we need to consider "skipMissing" here too.

+             */

+            g_warning("%s", msg);

+            return;

+        }

         OSINFO_LOADER_SET_ERROR(&error, msg);

         g_propagate_error(err, error);

     } else {

diff --git a/tests/test-loader.c b/tests/test-loader.c

index 6644943..bb86585 100644

--- a/tests/test-loader.c

+++ b/tests/test-loader.c

@@ -16,6 +16,8 @@

  */

 #include <osinfo/osinfo.h>

+#include <glib/gstdio.h>

+#include <unistd.h>

 static void

 test_basic(void)

@@ -31,6 +33,101 @@ test_basic(void)

     g_object_unref(loader);

 }

+typedef struct {

+    gchar *tmp_parent;

+    gchar *tmp_child;

+    gchar *orig_userdir;

+    gchar *expected_warning;

+} TestEaccesFixture;

+

+static void

+eacces_fixture_setup(TestEaccesFixture *fixture, gconstpointer user_data)

+{

+    gpointer rp;

+    gint ri;

+    gboolean rb;

+

+    /* create a temporary directory with permissions 0700 */

+    fixture->tmp_parent = g_strdup_printf("%s/%s", g_get_tmp_dir(),

+                                          "test_eacces.XXXXXX");

+    rp = g_mkdtemp_full(fixture->tmp_parent, 0700);

+    g_assert_nonnull(rp);

+

+    /* create a child directory called "osinfo" in it, with permissions 0700 */

+    fixture->tmp_child = g_strdup_printf("%s/osinfo", fixture->tmp_parent);

+    ri = g_mkdir(fixture->tmp_child, 0700);

+    g_assert_cmpint(ri, ==, 0);

+

+    /* revoke the search permission (0100) from the parent */

+    ri = g_chmod(fixture->tmp_parent, 0600);

+    g_assert_cmpint(ri, ==, 0);

+

+    /* stash the current value of OSINFO_USER_DIR */

+    fixture->orig_userdir = g_strdup(g_getenv("OSINFO_USER_DIR"));

+

+    /* point osinfo_loader_get_user_path() inside

+     * osinfo_loader_process_default_path() to the child directory

+     */

+    rb = g_setenv("OSINFO_USER_DIR", fixture->tmp_child, TRUE);

+    g_assert_true(rb);

+

+    /* format the pattern for the warning expected later on */

+    fixture->expected_warning = g_strdup_printf("Can't read path %s",

+                                                fixture->tmp_child);

+}

+

+static void

+eacces_fixture_teardown(TestEaccesFixture *fixture, gconstpointer user_data)

+{

+    gboolean rb;

+    gint ri;

+

+    /* free the expected warning pattern */

+    g_free(fixture->expected_warning);

+

+    /* restore the OSINFO_USER_DIR variable */

+    if (fixture->orig_userdir) {

+        rb = g_setenv("OSINFO_USER_DIR", fixture->orig_userdir, TRUE);

+        g_assert_true(rb);

+        g_free(fixture->orig_userdir);

+    } else {

+        g_unsetenv("OSINFO_USER_DIR");

+    }

+

+    /* restore search permission on the parent */

+    ri = g_chmod(fixture->tmp_parent, 0700);

+    g_assert_cmpint(ri, ==, 0);

+

+    /* remove both directories */

+    ri = g_rmdir(fixture->tmp_child);

+    g_assert_cmpint(ri, ==, 0);

+    g_free(fixture->tmp_child);

+

+    ri = g_rmdir(fixture->tmp_parent);

+    g_assert_cmpint(ri, ==, 0);

+    g_free(fixture->tmp_parent);

+}

+

+static void

+test_eacces(TestEaccesFixture *fixture, gconstpointer user_data)

+{

+    OsinfoLoader *loader = osinfo_loader_new();

+    GError *error = NULL;

+

+    g_assert_true(OSINFO_IS_LOADER(loader));

+

+    /* this should trigger an EACCES in glib's lstat(), but not break db

+     * loading; also we expect the warning here

+     */

+    g_test_expect_message(G_LOG_DOMAIN, G_LOG_LEVEL_WARNING,

+                          fixture->expected_warning);

+    osinfo_loader_process_default_path(loader, &error);

+    g_assert_no_error(error);

+    g_test_assert_expected_messages();

+

+    g_object_unref(loader);

+}

+

 int

 main(int argc, char *argv[])

 {

@@ -38,6 +135,14 @@ main(int argc, char *argv[])

     g_test_add_func("/loader/basic", test_basic);

+    /* the following test depends on a directory with file mode bits 0600 being

+     * unsearchable for the owner, so skip it if the test is running as root

+     */

+    if (geteuid() != 0) {

+        g_test_add("/loader/eacces", TestEaccesFixture, NULL,

+                   eacces_fixture_setup, test_eacces, eacces_fixture_teardown);

+    }

+

     /* Upfront so we don't confuse valgrind */

     osinfo_entity_get_type();

     osinfo_db_get_type();

-- 

2.34.1