From a486a0c52da9fba927c66dba6ee4ad31a7cc559f Mon Sep 17 00:00:00 2001 From: Benjamin Marzinski Date: Thu, 17 Aug 2023 14:39:12 -0500 Subject: [PATCH 1/9] resource: ubridge: initialize lists early in sid_resource_create In sid_resource_create() we can fail after the resource is allocated, but before we initialize the lists. In this case we were calling list_iterate_items_safe_back() on res->children and res->event_sources before they were initialized. --- src/resource/resource.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/resource/resource.c b/src/resource/resource.c index 89ade7cd..64bd48a0 100644 --- a/src/resource/resource.c +++ b/src/resource/resource.c @@ -264,6 +264,8 @@ sid_resource_t *sid_resource_create(sid_resource_t *parent_res, goto fail; res->id = id; + list_init(&res->children); + list_init(&res->event_sources); /* * Take temporary reference! @@ -280,8 +282,7 @@ sid_resource_t *sid_resource_create(sid_resource_t *parent_res, if (_create_service_link_group(res, service_link_defs) < 0) goto fail; - res->flags = flags; - list_init(&res->children); + res->flags = flags; res->type = type; res->prio = prio; res->event_loop.sd_event_loop = NULL; @@ -291,8 +292,6 @@ sid_resource_t *sid_resource_create(sid_resource_t *parent_res, if (type->with_event_loop && sd_event_new(&res->event_loop.sd_event_loop) < 0) goto fail; - list_init(&res->event_sources); - _add_res_to_parent_res(res, parent_res); if (type->with_event_loop && type->with_watchdog && sd_event_set_watchdog(res->event_loop.sd_event_loop, 1) < 0) From 4a23c36987d862e612b5c2ba1e96093834fb9112 Mon Sep 17 00:00:00 2001 From: Benjamin Marzinski Date: Thu, 17 Aug 2023 15:41:39 -0500 Subject: [PATCH 2/9] iface: service-link: remove unnecessary return codes service_link_add_notification(), service_link_remove_notification(), and service_link_group_add_member() cannot fail, but they still return an integer. _create_service_link_group() even checks this integer, although its failure path doesn't correctly cleanup the allocated service link. Instead of adding code to cleanup the service link on an error that cannot happen, just change the functions to not return anything. --- src/iface/service-link.c | 12 ++++-------- src/include/iface/service-link.h | 8 ++++---- src/resource/resource.c | 7 ++----- tests/test_notify.c | 14 +++++++------- 4 files changed, 17 insertions(+), 24 deletions(-) diff --git a/src/iface/service-link.c b/src/iface/service-link.c index 2e00b371..c1627a0f 100644 --- a/src/iface/service-link.c +++ b/src/iface/service-link.c @@ -70,16 +70,14 @@ void service_link_destroy(struct service_link *sl) free(sl); } -int service_link_add_notification(struct service_link *sl, service_link_notification_t notification) +void service_link_add_notification(struct service_link *sl, service_link_notification_t notification) { sl->notification |= notification; - return 0; } -int service_link_remove_notification(struct service_link *sl, service_link_notification_t notification) +void service_link_remove_notification(struct service_link *sl, service_link_notification_t notification) { sl->notification &= ~notification; - return 0; } struct service_link_group *service_link_group_create(const char *name) @@ -115,12 +113,10 @@ void service_link_group_destroy_with_members(struct service_link_group *slg) free(slg); } -int service_link_group_add_member(struct service_link_group *slg, struct service_link *sl) +void service_link_group_add_member(struct service_link_group *slg, struct service_link *sl) { list_add(&slg->members, &sl->list); sl->group = slg; - - return 0; } int service_link_group_remove_member(struct service_link_group *slg, struct service_link *sl) @@ -150,7 +146,7 @@ static const char *_get_arg_value(const char *str, const char *key_eq, size_t *s if (!strncmp(key_eq, str, strlen(key_eq))) { /* get the value and its size */ str += strlen(key_eq); - *size = line_end - str; + *size = line_end - str; return str; } } diff --git a/src/include/iface/service-link.h b/src/include/iface/service-link.h index dc89555b..323cacfa 100644 --- a/src/include/iface/service-link.h +++ b/src/include/iface/service-link.h @@ -82,15 +82,15 @@ struct service_link_group; struct service_link *service_link_create(service_link_type_t type, const char *name); void service_link_destroy(struct service_link *sl); -int service_link_add_notification(struct service_link *sl, service_link_notification_t notification); -int service_link_remove_notification(struct service_link *sl, service_link_notification_t notification); +void service_link_add_notification(struct service_link *sl, service_link_notification_t notification); +void service_link_remove_notification(struct service_link *sl, service_link_notification_t notification); struct service_link_group *service_link_group_create(const char *name); void service_link_group_destroy(struct service_link_group *slg); void service_link_group_destroy_with_members(struct service_link_group *slg); -int service_link_group_add_member(struct service_link_group *slg, struct service_link *sl); -int service_link_group_remove_member(struct service_link_group *slg, struct service_link *sl); +void service_link_group_add_member(struct service_link_group *slg, struct service_link *sl); +int service_link_group_remove_member(struct service_link_group *slg, struct service_link *sl); /* * Send service notification. diff --git a/src/resource/resource.c b/src/resource/resource.c index 64bd48a0..851eb810 100644 --- a/src/resource/resource.c +++ b/src/resource/resource.c @@ -194,11 +194,8 @@ static int _create_service_link_group(sid_resource_t *res, sid_resource_service_ goto out; } - if ((r = service_link_add_notification(sl, def->notification)) < 0) - goto out; - - if ((r = service_link_group_add_member(slg, sl)) < 0) - goto out; + service_link_add_notification(sl, def->notification); + service_link_group_add_member(slg, sl); } res->slg = slg; diff --git a/tests/test_notify.c b/tests/test_notify.c index 4f392356..e9a02be0 100644 --- a/tests/test_notify.c +++ b/tests/test_notify.c @@ -28,7 +28,7 @@ static void test_notify_ready(void **state) struct service_link *sl = service_link_create(SERVICE_TYPE_SYSTEMD, "systemd"); assert_non_null(sl); - assert_int_equal(service_link_add_notification(sl, SERVICE_NOTIFICATION_READY), 0); + service_link_add_notification(sl, SERVICE_NOTIFICATION_READY); will_return(__wrap_sd_notify, "READY=1\n"); assert_int_equal(service_link_notify(sl, SERVICE_NOTIFICATION_READY, NULL), 0); service_link_destroy(sl); @@ -39,8 +39,8 @@ static void test_notify_ready_reloading(void **state) struct service_link *sl = service_link_create(SERVICE_TYPE_SYSTEMD, "systemd"); assert_non_null(sl); - assert_int_equal(service_link_add_notification(sl, SERVICE_NOTIFICATION_READY), 0); - assert_int_equal(service_link_add_notification(sl, SERVICE_NOTIFICATION_RELOADING), 0); + service_link_add_notification(sl, SERVICE_NOTIFICATION_READY); + service_link_add_notification(sl, SERVICE_NOTIFICATION_RELOADING); will_return(__wrap_sd_notify, "READY=1\nRELOADING=1\n"); assert_int_equal(service_link_notify(sl, SERVICE_NOTIFICATION_READY | SERVICE_NOTIFICATION_RELOADING, NULL), 0); service_link_destroy(sl); @@ -51,7 +51,7 @@ static void test_notify_blank(void **state) struct service_link *sl = service_link_create(SERVICE_TYPE_SYSTEMD, "systemd"); assert_non_null(sl); - assert_int_equal(service_link_add_notification(sl, SERVICE_NOTIFICATION_STATUS), 0); + service_link_add_notification(sl, SERVICE_NOTIFICATION_STATUS); will_return(__wrap_sd_notify, ""); assert_int_equal(service_link_notify(sl, SERVICE_NOTIFICATION_STATUS, NULL), 0); service_link_destroy(sl); @@ -62,7 +62,7 @@ static void test_notify_errno(void **state) struct service_link *sl = service_link_create(SERVICE_TYPE_SYSTEMD, "systemd"); assert_non_null(sl); - assert_int_equal(service_link_add_notification(sl, SERVICE_NOTIFICATION_ERRNO), 0); + service_link_add_notification(sl, SERVICE_NOTIFICATION_ERRNO); will_return(__wrap_sd_notify, "ERRNO=2\n"); assert_int_equal(service_link_notify(sl, SERVICE_NOTIFICATION_ERRNO, "ERRNO=%d\n", 2), 0); service_link_destroy(sl); @@ -73,8 +73,8 @@ static void test_notify_errno_status(void **state) struct service_link *sl = service_link_create(SERVICE_TYPE_SYSTEMD, "systemd"); assert_non_null(sl); - assert_int_equal(service_link_add_notification(sl, SERVICE_NOTIFICATION_ERRNO), 0); - assert_int_equal(service_link_add_notification(sl, SERVICE_NOTIFICATION_STATUS), 0); + service_link_add_notification(sl, SERVICE_NOTIFICATION_ERRNO); + service_link_add_notification(sl, SERVICE_NOTIFICATION_STATUS); will_return(__wrap_sd_notify, "STATUS=testing\nERRNO=2\n"); assert_int_equal( service_link_notify(sl, SERVICE_NOTIFICATION_ERRNO | SERVICE_NOTIFICATION_STATUS, "ERRNO=%d\nSTATUS=testing", 2), From 6439caa35f5970fd8a96378f5d9fc7e9e730de0a Mon Sep 17 00:00:00 2001 From: Benjamin Marzinski Date: Mon, 21 Aug 2023 12:56:37 -0500 Subject: [PATCH 3/9] resource: ubridge: close fd_pass on _init_connection failure --- src/resource/ubridge.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/resource/ubridge.c b/src/resource/ubridge.c index ded76efc..246e41ff 100644 --- a/src/resource/ubridge.c +++ b/src/resource/ubridge.c @@ -4737,6 +4737,7 @@ static int _init_connection(sid_resource_t *res, const void *kickstart_data, voi sid_buffer_destroy(conn->buf); free(conn); } + close(data_spec->ext.socket.fd_pass); return -1; } From 15b6c96e244737a85deec5a65858ad8b9ba1eb2e Mon Sep 17 00:00:00 2001 From: Benjamin Marzinski Date: Mon, 21 Aug 2023 13:37:26 -0500 Subject: [PATCH 4/9] resource: ubridge: verify that message size before checking header Make sure that the message size is larger than the header before memcpying the header, so we don't access possibly unallocated memory. --- src/resource/ubridge.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/resource/ubridge.c b/src/resource/ubridge.c index 246e41ff..9ea9a335 100644 --- a/src/resource/ubridge.c +++ b/src/resource/ubridge.c @@ -4594,13 +4594,13 @@ static int _check_msg(sid_resource_t *res, struct sid_msg *msg) { struct sid_msg_header header; - memcpy(&header, msg->header, sizeof(header)); - if (msg->size < sizeof(struct sid_msg_header)) { log_error(ID(res), "Incorrect message header size."); return -1; } + memcpy(&header, msg->header, sizeof(header)); + /* Sanitize command number - map all out of range command numbers to CMD_UNKNOWN. */ switch (msg->cat) { case MSG_CATEGORY_SYSTEM: @@ -4638,11 +4638,11 @@ static int _create_command_resource(sid_resource_t *parent_res, struct sid_msg * { struct sid_msg_header header; - memcpy(&header, msg->header, sizeof(header)); - if (_check_msg(parent_res, msg) < 0) return -1; + memcpy(&header, msg->header, sizeof(header)); + if (!sid_resource_create(parent_res, &sid_resource_type_ubridge_command, SID_RESOURCE_NO_FLAGS, From bc5ea5c47be66048023f8e3f0322bdee2a4f0cc1 Mon Sep 17 00:00:00 2001 From: Benjamin Marzinski Date: Tue, 29 Aug 2023 14:03:56 -0500 Subject: [PATCH 5/9] resource: ubridge: Always allocate enough space for the vvalue header. If we are doing a remove, count will be zero here. --- src/resource/ubridge.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/resource/ubridge.c b/src/resource/ubridge.c index 9ea9a335..ee530615 100644 --- a/src/resource/ubridge.c +++ b/src/resource/ubridge.c @@ -3667,12 +3667,13 @@ static int _refresh_device_disk_hierarchy_from_sysfs(sid_resource_t *cmd_res) * +VVALUE_HEADER_CNT to include record header * -2 to subtract "." and ".." directory which we're not interested in */ - if (!(vec_buf = sid_buffer_create( - &((struct sid_buffer_spec) {.backend = SID_BUFFER_BACKEND_MALLOC, - .type = SID_BUFFER_TYPE_VECTOR, - .mode = SID_BUFFER_MODE_PLAIN}), - &((struct sid_buffer_init) {.size = count + VVALUE_HEADER_CNT - 2, .alloc_step = 1, .limit = 0}), - &r))) { + if (!(vec_buf = sid_buffer_create(&((struct sid_buffer_spec) {.backend = SID_BUFFER_BACKEND_MALLOC, + .type = SID_BUFFER_TYPE_VECTOR, + .mode = SID_BUFFER_MODE_PLAIN}), + &((struct sid_buffer_init) {.size = VVALUE_HEADER_CNT + (count >= 2 ? count - 2 : 0), + .alloc_step = 1, + .limit = 0}), + &r))) { log_error_errno(ID(cmd_res), r, "Failed to create buffer to record hierarchy for device " CMD_DEV_NAME_NUM_FMT, From bc6275dee442e75bd5fb36352006a81010998caf Mon Sep 17 00:00:00 2001 From: Benjamin Marzinski Date: Tue, 29 Aug 2023 16:41:30 -0500 Subject: [PATCH 6/9] resource: ubridge: free dirents on failure when refreshing disks --- src/resource/ubridge.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/src/resource/ubridge.c b/src/resource/ubridge.c index ee530615..0a70d01c 100644 --- a/src/resource/ubridge.c +++ b/src/resource/ubridge.c @@ -3692,10 +3692,8 @@ static int _refresh_device_disk_hierarchy_from_sysfs(sid_resource_t *cmd_res) /* Read relatives from sysfs into vec_buf. */ if (ucmd_ctx->req_env.dev.udev.action != UDEV_ACTION_REMOVE) { for (i = 0; i < count; i++) { - if (dirent[i]->d_name[0] == '.') { - free(dirent[i]); - continue; - } + if (dirent[i]->d_name[0] == '.') + goto next; if (sid_buffer_fmt_add(ucmd_ctx->common->gen_buf, (const void **) &s, @@ -3718,7 +3716,7 @@ static int _refresh_device_disk_hierarchy_from_sysfs(sid_resource_t *cmd_res) "Failed to read related disk device number from sysfs file %s.", s); sid_buffer_rewind_mem(ucmd_ctx->common->gen_buf, s); - continue; + goto next; } sid_buffer_rewind_mem(ucmd_ctx->common->gen_buf, s); @@ -3730,7 +3728,7 @@ static int _refresh_device_disk_hierarchy_from_sysfs(sid_resource_t *cmd_res) log_error(ID(cmd_res), "Failed to generate UUID for device " CMD_DEV_NAME_NUM_FMT ".", CMD_DEV_NAME_NUM(ucmd_ctx)); - continue; + goto next; } rel_spec.rel_key_spec->ns_part = mem.base; @@ -3745,7 +3743,7 @@ static int _refresh_device_disk_hierarchy_from_sysfs(sid_resource_t *cmd_res) log_error(ID(cmd_res), "Failed to add devno alias for device " CMD_DEV_NAME_NUM_FMT, CMD_DEV_NAME_NUM(ucmd_ctx)); - continue; + goto next; } } @@ -3753,10 +3751,11 @@ static int _refresh_device_disk_hierarchy_from_sysfs(sid_resource_t *cmd_res) if (!s || ((r = sid_buffer_add(vec_buf, (void *) s, strlen(s) + 1, NULL, NULL)) < 0)) goto out; } - +next: free(dirent[i]); } free(dirent); + dirent = NULL; rel_spec.rel_key_spec->ns_part = ID_NULL; } @@ -3778,6 +3777,11 @@ static int _refresh_device_disk_hierarchy_from_sysfs(sid_resource_t *cmd_res) _destroy_key(NULL, s); r = 0; out: + if (dirent) { + for (; i < count; i++) + free(dirent[i]); + free(dirent); + } if (vec_buf) { if (!vsize) sid_buffer_get_data(vec_buf, (const void **) (&vvalue), &vsize); From 6b166ffbb853b261bded3d1e430a6e57d15d3fbe Mon Sep 17 00:00:00 2001 From: Benjamin Marzinski Date: Tue, 29 Aug 2023 18:00:32 -0500 Subject: [PATCH 7/9] resource: ubridge: free unused key when refreshing disks Calling _destroy_key() with a NULL key is fine, so we can always call it on failure here. --- src/resource/ubridge.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/resource/ubridge.c b/src/resource/ubridge.c index 0a70d01c..3e0927cf 100644 --- a/src/resource/ubridge.c +++ b/src/resource/ubridge.c @@ -3748,8 +3748,10 @@ static int _refresh_device_disk_hierarchy_from_sysfs(sid_resource_t *cmd_res) } s = _compose_key_prefix(NULL, rel_spec.rel_key_spec); - if (!s || ((r = sid_buffer_add(vec_buf, (void *) s, strlen(s) + 1, NULL, NULL)) < 0)) + if (!s || ((r = sid_buffer_add(vec_buf, (void *) s, strlen(s) + 1, NULL, NULL)) < 0)) { + _destroy_key(NULL, s); goto out; + } } next: free(dirent[i]); From 25d03dee306bee8af7af5d4b81eae70f3c2d8141 Mon Sep 17 00:00:00 2001 From: Benjamin Marzinski Date: Tue, 29 Aug 2023 18:29:07 -0500 Subject: [PATCH 8/9] resource: ubridge: free key on partition refresh failure --- src/resource/ubridge.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/resource/ubridge.c b/src/resource/ubridge.c index 3e0927cf..fe4bf112 100644 --- a/src/resource/ubridge.c +++ b/src/resource/ubridge.c @@ -3802,8 +3802,8 @@ static int _refresh_device_partition_hierarchy_from_sysfs(sid_resource_t *cmd_re kv_vector_t vvalue[VVALUE_SINGLE_CNT]; char devno_buf[16]; char devid_buf[UTIL_UUID_STR_SIZE]; - const char *s; - char *key; + const char *s = NULL; + char *key = NULL; util_mem_t mem; int r = -1; @@ -3876,10 +3876,10 @@ static int _refresh_device_partition_hierarchy_from_sysfs(sid_resource_t *cmd_re */ _kv_delta_set(key, vvalue, VVALUE_SINGLE_CNT, &update_arg, true); - _destroy_key(NULL, key); - _destroy_key(NULL, s); r = 0; out: + _destroy_key(NULL, key); + _destroy_key(NULL, s); return r; } From f1cdc3a64988e758147600e2e2872843087fc0eb Mon Sep 17 00:00:00 2001 From: Benjamin Marzinski Date: Mon, 25 Sep 2023 18:05:28 -0500 Subject: [PATCH 9/9] ucmd: module: dm: don't write garbage keys If SID fails to get the sysfs dm sysfs values, likely because the device has been removed from sysfs, fail instead of using uninitialized memory for the key. --- src/modules/ucmd/type/dm/dm.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/modules/ucmd/type/dm/dm.c b/src/modules/ucmd/type/dm/dm.c index 16dace43..5112634d 100644 --- a/src/modules/ucmd/type/dm/dm.c +++ b/src/modules/ucmd/type/dm/dm.c @@ -170,12 +170,18 @@ static int _dm_ident(struct module *module, struct sid_ucmd_ctx *ucmd_ctx) log_debug(DM_ID, "ident"); snprintf(path, sizeof(path), "%s%s/dm/uuid", SYSTEM_SYSFS_PATH, sid_ucmd_event_get_dev_path(ucmd_ctx)); - sid_util_sysfs_get_value(path, uuid, sizeof(uuid)); + if (sid_util_sysfs_get_value(path, uuid, sizeof(uuid)) < 0) { + log_error(DM_ID, "Failed to get DM uuid."); + return -1; + } sid_ucmd_dev_add_alias(module, ucmd_ctx, "uuid", uuid); sid_ucmd_set_kv(module, ucmd_ctx, KV_NS_DEVMOD, "uuid", uuid, strlen(uuid) + 1, KV_SYNC | KV_SUB_RD); snprintf(path, sizeof(path), "%s%s/dm/name", SYSTEM_SYSFS_PATH, sid_ucmd_event_get_dev_path(ucmd_ctx)); - sid_util_sysfs_get_value(path, name, sizeof(name)); + if (sid_util_sysfs_get_value(path, name, sizeof(name)) < 0) { + log_error(DM_ID, "Failed to get DM name."); + return -1; + } sid_ucmd_dev_add_alias(module, ucmd_ctx, "name", name); sid_ucmd_set_kv(module, ucmd_ctx, KV_NS_DEVMOD, "name", name, strlen(name) + 1, KV_SYNC | KV_SUB_RD);