Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

2023.6 coverity minor fixes #3013

Merged
merged 4 commits into from
Aug 26, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Makefile-tests.am
Original file line number Diff line number Diff line change
Expand Up @@ -366,7 +366,7 @@ tests_test_bsdiff_CFLAGS = $(TESTS_CFLAGS)
tests_test_bsdiff_LDADD = libbsdiff.la $(TESTS_LDADD)

tests_test_otcore_CFLAGS = $(AM_CFLAGS) $(OT_INTERNAL_GIO_UNIX_CFLAGS) -I$(srcdir)/src/libotutil -I$(srcdir)/src/libotcore -I$(srcdir)/libglnx
tests_test_otcore_LDADD = $(OT_INTERNAL_GIO_UNIX_LIBS) libotcore.la libglnx.la
tests_test_otcore_LDADD = $(OT_INTERNAL_GIO_UNIX_LIBS) libotcore.la libglnx.la libotutil.la

tests_test_checksum_SOURCES = \
src/libostree/ostree-core.c \
Expand Down
10 changes: 1 addition & 9 deletions src/libostree/ostree-repo-commit.c
Original file line number Diff line number Diff line change
Expand Up @@ -2850,14 +2850,6 @@ ostree_repo_write_content_finish (OstreeRepo *self, GAsyncResult *result, guchar
return TRUE;
}

static GVariant *
create_empty_gvariant_dict (void)
{
GVariantBuilder builder;
g_variant_builder_init (&builder, G_VARIANT_TYPE ("a{sv}"));
return g_variant_builder_end (&builder);
}

/**
* ostree_repo_write_commit:
* @self: Repo
Expand Down Expand Up @@ -2951,7 +2943,7 @@ ostree_repo_write_commit_with_time (OstreeRepo *self, const char *parent, const
return FALSE;

g_autoptr (GVariant) commit = g_variant_new (
"(@a{sv}@ay@a(say)sst@ay@ay)", new_metadata ? new_metadata : create_empty_gvariant_dict (),
"(@a{sv}@ay@a(say)sst@ay@ay)", new_metadata,
parent ? ostree_checksum_to_bytes_v (parent) : ot_gvariant_new_bytearray (NULL, 0),
g_variant_new_array (G_VARIANT_TYPE ("(say)"), NULL, 0), subject ? subject : "",
body ? body : "", GUINT64_TO_BE (time),
Expand Down
9 changes: 5 additions & 4 deletions src/libostree/ostree-sign-ed25519.c
Original file line number Diff line number Diff line change
Expand Up @@ -415,17 +415,18 @@ ostree_sign_ed25519_add_pk (OstreeSign *self, GVariant *public_key, GError **err
if (!_ostree_sign_ed25519_is_initialized (sign, error))
return FALSE;

gpointer key = NULL;
g_autofree guint8 *key_owned = NULL;
const guint8 *key = NULL;
Copy link
Collaborator

@ericcurtin ericcurtin Aug 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not?

g_autofree const guint8 *key = NULL;

key_owned is unused, just set.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because g_variant_get_fixed_array() returns a const pointer into its data, whereas g_base64_decode returns a new value. Notice that we do set key_owned only in the latter case.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see, so you want it free'd at the end of scope in one case but not the other... Makes sense

gsize n_elements = 0;

if (g_variant_is_of_type (public_key, G_VARIANT_TYPE_STRING))
{
const gchar *pk_ascii = g_variant_get_string (public_key, NULL);
key = g_base64_decode (pk_ascii, &n_elements);
key = key_owned = g_base64_decode (pk_ascii, &n_elements);
}
else if (g_variant_is_of_type (public_key, G_VARIANT_TYPE_BYTESTRING))
{
key = (gpointer)g_variant_get_fixed_array (public_key, &n_elements, sizeof (guchar));
key = g_variant_get_fixed_array (public_key, &n_elements, sizeof (guchar));
}
else
{
Expand Down Expand Up @@ -461,7 +462,7 @@ _ed25519_add_revoked (OstreeSign *self, GVariant *revoked_key, GError **error)

const gchar *rk_ascii = g_variant_get_string (revoked_key, NULL);
gsize n_elements = 0;
gpointer key = g_base64_decode (rk_ascii, &n_elements);
g_autofree guint8 *key = g_base64_decode (rk_ascii, &n_elements);

if (!validate_length (n_elements, OSTREE_SIGN_ED25519_PUBKEY_SIZE, error))
return glnx_prefix_error (error, "Incorrect ed25519 revoked key");
Expand Down
37 changes: 34 additions & 3 deletions src/libotcore/otcore-prepare-root.c
Original file line number Diff line number Diff line change
Expand Up @@ -82,12 +82,12 @@ otcore_get_ostree_target (const char *cmdline, char **out_target, GError **error
{
if (strcmp (slot_suffix, "_a") == 0)
{
*out_target = strdup (slot_a);
*out_target = g_strdup (slot_a);
return TRUE;
}
else if (strcmp (slot_suffix, "_b") == 0)
{
*out_target = strdup (slot_b);
*out_target = g_strdup (slot_b);
return TRUE;
}
return glnx_throw (error, "androidboot.slot_suffix invalid: %s", slot_suffix);
Expand All @@ -98,11 +98,42 @@ otcore_get_ostree_target (const char *cmdline, char **out_target, GError **error
*/
if (proc_cmdline_has_key_starting_with (cmdline, "androidboot."))
{
*out_target = strdup (slot_a);
*out_target = g_strdup (slot_a);
return TRUE;
}

// Otherwise, fall back to the default `ostree=` kernel cmdline
*out_target = otcore_find_proc_cmdline_key (cmdline, "ostree");
return TRUE;
}

// Load a config file; if it doesn't exist, we return an empty configuration.
// NULL will be returned if we caught an error.
GKeyFile *
otcore_load_config (int rootfs_fd, const char *filename, GError **error)
{
// The path to the config file for this binary
static const char *const config_roots[] = { "usr/lib", "etc" };
g_autoptr (GKeyFile) ret = g_key_file_new ();

for (guint i = 0; i < G_N_ELEMENTS (config_roots); i++)
{
glnx_autofd int fd = -1;
g_autofree char *path = g_build_filename (config_roots[i], filename, NULL);
if (!ot_openat_ignore_enoent (rootfs_fd, path, &fd, error))
return NULL;
/* If the config file doesn't exist, that's OK */
if (fd == -1)
continue;

g_print ("Loading %s\n", path);

g_autofree char *buf = glnx_fd_readall_utf8 (fd, NULL, NULL, error);
if (!buf)
return NULL;
if (!g_key_file_load_from_data (ret, buf, -1, 0, error))
return NULL;
}

return g_steal_pointer (&ret);
}
2 changes: 2 additions & 0 deletions src/libotcore/otcore.h
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ gboolean otcore_validate_ed25519_signature (GBytes *data, GBytes *pubkey, GBytes
char *otcore_find_proc_cmdline_key (const char *cmdline, const char *key);
gboolean otcore_get_ostree_target (const char *cmdline, char **out_target, GError **error);

GKeyFile *otcore_load_config (int rootfs, const char *filename, GError **error);

// Our directory with transient state (eventually /run/ostree-booted should be a link to
// /run/ostree/booted)
#define OTCORE_RUN_OSTREE "/run/ostree"
Expand Down
44 changes: 10 additions & 34 deletions src/switchroot/ostree-prepare-root.c
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,6 @@
#include "ot-keyfile-utils.h"
#include "otcore.h"

// The path to the config file for this binary
const char *config_roots[] = { "/usr/lib", "/etc" };
#define PREPARE_ROOT_CONFIG_PATH "ostree/prepare-root.conf"

// This key is used by default if present in the initramfs to verify
Expand Down Expand Up @@ -106,35 +104,6 @@ const char *config_roots[] = { "/usr/lib", "/etc" };

#include "ostree-mount-util.h"

// Load our config file; if it doesn't exist, we return an empty configuration.
// NULL will be returned if we caught an error.
static GKeyFile *
load_config (GError **error)
{
g_autoptr (GKeyFile) ret = g_key_file_new ();

for (guint i = 0; i < G_N_ELEMENTS (config_roots); i++)
{
glnx_autofd int fd = -1;
g_autofree char *path = g_build_filename (config_roots[i], PREPARE_ROOT_CONFIG_PATH, NULL);
if (!ot_openat_ignore_enoent (AT_FDCWD, path, &fd, error))
return NULL;
/* If the config file doesn't exist, that's OK */
if (fd == -1)
continue;

g_print ("Loading %s\n", path);

g_autofree char *buf = glnx_fd_readall_utf8 (fd, NULL, NULL, error);
if (!buf)
return NULL;
if (!g_key_file_load_from_data (ret, buf, -1, 0, error))
return NULL;
}

return g_steal_pointer (&ret);
}

static bool
sysroot_is_configured_ro (const char *sysroot)
{
Expand Down Expand Up @@ -278,8 +247,8 @@ static void
free_composefs_config (ComposefsConfig *config)
{
g_ptr_array_unref (config->pubkeys);
free (config->signature_pubkey);
free (config);
g_free (config->signature_pubkey);
g_free (config);
}

G_DEFINE_AUTOPTR_CLEANUP_FUNC (ComposefsConfig, free_composefs_config)
Expand Down Expand Up @@ -350,7 +319,14 @@ main (int argc, char *argv[])
err (EXIT_FAILURE, "usage: ostree-prepare-root SYSROOT");
const char *root_arg = argv[1];

g_autoptr (GKeyFile) config = load_config (&error);
// Since several APIs want to operate in terms of file descriptors, let's
// open the initramfs now. Currently this is just used for the config parser.
glnx_autofd int initramfs_rootfs_fd = -1;
if (!glnx_opendirat (AT_FDCWD, "/", FALSE, &initramfs_rootfs_fd, &error))
errx (EXIT_FAILURE, "Failed to open /: %s", error->message);

g_autoptr (GKeyFile) config
= otcore_load_config (initramfs_rootfs_fd, PREPARE_ROOT_CONFIG_PATH, &error);
if (!config)
errx (EXIT_FAILURE, "Failed to parse config: %s", error->message);

Expand Down
49 changes: 49 additions & 0 deletions tests/test-otcore.c
Original file line number Diff line number Diff line change
Expand Up @@ -81,12 +81,61 @@ test_prepare_root_cmdline (void)
free (g_steal_pointer (&target));
}

static void
test_prepare_root_config (void)
{
g_autoptr (GError) error = NULL;
g_auto (GLnxTmpDir) tmpdir = {
0,
};
g_assert (glnx_mkdtempat (AT_FDCWD, "/tmp/test-XXXXXX", 0777, &tmpdir, &error));
g_assert_no_error (error);

{
g_autoptr (GKeyFile) config = NULL;
g_auto (GStrv) keys = NULL;
config = otcore_load_config (tmpdir.fd, "ostree/someconfig.conf", &error);
g_assert (config);
keys = g_key_file_get_groups (config, NULL);
g_assert (keys && *keys == NULL);
}

g_assert (glnx_shutil_mkdir_p_at (tmpdir.fd, "usr/lib/ostree", 0755, NULL, NULL));
g_assert (glnx_file_replace_contents_at (tmpdir.fd, "usr/lib/ostree/someconfig.conf",
(guint8 *)"[foo]\nbar=baz", -1, 0, NULL, NULL));

{
g_autoptr (GKeyFile) config = NULL;
g_auto (GStrv) keys = NULL;
config = otcore_load_config (tmpdir.fd, "ostree/someconfig.conf", &error);
g_assert (config);
keys = g_key_file_get_groups (config, NULL);
g_assert (keys);
g_assert_cmpstr (*keys, ==, "foo");
}

g_assert (glnx_shutil_mkdir_p_at (tmpdir.fd, "etc/ostree", 0755, NULL, NULL));
g_assert (glnx_file_replace_contents_at (tmpdir.fd, "usr/lib/ostree/someconfig.conf",
(guint8 *)"[test]\nbar=baz", -1, 0, NULL, NULL));

{
g_autoptr (GKeyFile) config = NULL;
g_auto (GStrv) keys = NULL;
config = otcore_load_config (tmpdir.fd, "ostree/someconfig.conf", &error);
g_assert (config);
keys = g_key_file_get_groups (config, NULL);
g_assert (keys);
g_assert_cmpstr (*keys, ==, "test");
}
}

int
main (int argc, char **argv)
{
g_test_init (&argc, &argv, NULL);
otcore_ed25519_init ();
g_test_add_func ("/ed25519", test_ed25519);
g_test_add_func ("/prepare-root-cmdline", test_prepare_root_cmdline);
g_test_add_func ("/prepare-root-config", test_prepare_root_config);
return g_test_run ();
}
Loading