From e5bdc6759195dbcfc4e7dcb02bf59190a3debe06 Mon Sep 17 00:00:00 2001 From: Laszlo Ersek 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 , 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 --- 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 + * . 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 +#include +#include 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