From: Mike Yuan Date: Tue, 8 Oct 2024 12:53:14 +0000 (+0200) Subject: core/service: use array rather than list for extra fds, limit max number X-Git-Tag: v257-rc1~251^2~1 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=32af4dd80fd0ce8ae1c30b3b8ee915b7a97cfb33;p=thirdparty%2Fsystemd.git core/service: use array rather than list for extra fds, limit max number Follow-up for 3543456f84ec2e83e07b6c9bf2b3a1c5d30241d8 I don't think list is particularly useful here. The passed fds are constant for the lifetime of service, and with this commit we track the number of extra fds in a dedicated var anyway. --- diff --git a/src/core/dbus-service.c b/src/core/dbus-service.c index a0244c18607..77e9a08bdd2 100644 --- a/src/core/dbus-service.c +++ b/src/core/dbus-service.c @@ -78,7 +78,7 @@ static int property_get_extra_file_descriptors( void *userdata, sd_bus_error *error) { - ServiceExtraFD **extra_fds = ASSERT_PTR(userdata); + Service *s = ASSERT_PTR(userdata); int r; assert(bus); @@ -88,8 +88,8 @@ static int property_get_extra_file_descriptors( if (r < 0) return r; - LIST_FOREACH(extra_fd, efd, *extra_fds) { - r = sd_bus_message_append_basic(reply, 's', efd->fdname); + FOREACH_ARRAY(i, s->extra_fds, s->n_extra_fds) { + r = sd_bus_message_append_basic(reply, 's', i->fdname); if (r < 0) return r; } @@ -367,7 +367,7 @@ const sd_bus_vtable bus_service_vtable[] = { SD_BUS_PROPERTY("NRestarts", "u", bus_property_get_unsigned, offsetof(Service, n_restarts), SD_BUS_VTABLE_PROPERTY_EMITS_CHANGE), SD_BUS_PROPERTY("OOMPolicy", "s", bus_property_get_oom_policy, offsetof(Service, oom_policy), SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("OpenFile", "a(sst)", property_get_open_files, offsetof(Service, open_files), SD_BUS_VTABLE_PROPERTY_CONST), - SD_BUS_PROPERTY("ExtraFileDescriptorNames", "as", property_get_extra_file_descriptors, offsetof(Service, extra_fds), SD_BUS_VTABLE_PROPERTY_CONST), + SD_BUS_PROPERTY("ExtraFileDescriptorNames", "as", property_get_extra_file_descriptors, 0, SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("ReloadSignal", "i", bus_property_get_int, offsetof(Service, reload_signal), SD_BUS_VTABLE_PROPERTY_CONST), BUS_EXEC_STATUS_VTABLE("ExecMain", offsetof(Service, main_exec_status), SD_BUS_VTABLE_PROPERTY_EMITS_CHANGE), @@ -748,15 +748,13 @@ static int bus_service_set_transient_property( return bus_set_transient_reload_signal(u, name, &s->reload_signal, message, flags, error); if (streq(name, "ExtraFileDescriptors")) { - int fd; - const char *fdname; - r = sd_bus_message_enter_container(message, 'a', "(hs)"); if (r < 0) return r; for (;;) { - _cleanup_(service_extra_fd_freep) ServiceExtraFD *efd = NULL; + const char *fdname; + int fd; r = sd_bus_message_read(message, "(hs)", &fd, &fdname); if (r < 0) @@ -767,29 +765,30 @@ static int bus_service_set_transient_property( /* Disallow empty string for ExtraFileDescriptors. * Unlike OpenFile, StandardInput and friends, there isn't a good sane * default for an arbitrary FD. */ - if (fd < 0 || isempty(fdname) || !fdname_is_valid(fdname)) - return -EINVAL; + if (isempty(fdname) || !fdname_is_valid(fdname)) + return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Invalid extra fd name: %s", fdname); + + if (s->n_extra_fds >= NOTIFY_FD_MAX) + return sd_bus_error_set(error, SD_BUS_ERROR_LIMITS_EXCEEDED, "Too many extra fds sent"); if (UNIT_WRITE_FLAGS_NOOP(flags)) continue; - efd = new(ServiceExtraFD, 1); - if (!efd) + if (!GREEDY_REALLOC(s->extra_fds, s->n_extra_fds + 1)) return -ENOMEM; - *efd = (ServiceExtraFD) { - .fd = -EBADF, - .fdname = strdup(fdname), - }; - - if (!efd->fdname) + _cleanup_free_ char *fdname_dup = strdup(fdname); + if (!fdname_dup) return -ENOMEM; - efd->fd = fcntl(fd, F_DUPFD_CLOEXEC, 3); - if (efd->fd < 0) + _cleanup_close_ int fd_dup = fcntl(fd, F_DUPFD_CLOEXEC, 3); + if (fd_dup < 0) return -errno; - LIST_APPEND(extra_fd, s->extra_fds, TAKE_PTR(efd)); + s->extra_fds[s->n_extra_fds++] = (ServiceExtraFD) { + .fd = TAKE_FD(fd_dup), + .fdname = TAKE_PTR(fdname_dup), + }; } r = sd_bus_message_exit_container(message); diff --git a/src/core/service.c b/src/core/service.c index c008849b786..eddec763e16 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -446,7 +446,7 @@ static void service_release_fd_store(Service *s) { if (!s->fd_store) return; - log_unit_debug(UNIT(s), "Releasing all stored fds"); + log_unit_debug(UNIT(s), "Releasing all stored fds."); while (s->fd_store) service_fd_store_unlink(s->fd_store); @@ -454,19 +454,21 @@ static void service_release_fd_store(Service *s) { assert(s->n_fd_store == 0); } -ServiceExtraFD* service_extra_fd_free(ServiceExtraFD *efd) { - if (!efd) - return NULL; - - efd->fd = asynchronous_close(efd->fd); - free(efd->fdname); - return mfree(efd); -} - static void service_release_extra_fds(Service *s) { assert(s); - LIST_CLEAR(extra_fd, s->extra_fds, service_extra_fd_free); + if (!s->extra_fds) + return; + + log_unit_debug(UNIT(s), "Releasing extra file descriptors."); + + FOREACH_ARRAY(i, s->extra_fds, s->n_extra_fds) { + asynchronous_close(i->fd); + free(i->fdname); + } + + s->extra_fds = mfree(s->extra_fds); + s->n_extra_fds = 0; } static void service_release_stdio_fd(Service *s) { @@ -924,12 +926,18 @@ static int service_dump_fd(int fd, const char *fdname, const char *header, FILE struct stat st; int flags; + assert(fd >= 0); + assert(fdname); + assert(header); + assert(f); + assert(prefix); + if (fstat(fd, &st) < 0) - return log_debug_errno(errno, "Failed to stat fdstore entry: %m"); + return log_debug_errno(errno, "Failed to stat service fd: %m"); flags = fcntl(fd, F_GETFL); if (flags < 0) - return log_debug_errno(errno, "Failed to get fdstore entry flags: %m"); + return log_debug_errno(errno, "Failed to get service fd flags: %m"); (void) fd_get_path(fd, &path); @@ -948,32 +956,6 @@ static int service_dump_fd(int fd, const char *fdname, const char *header, FILE return 0; } -static void service_dump_fdstore(Service *s, FILE *f, const char *prefix) { - assert(s); - assert(f); - assert(prefix); - - LIST_FOREACH(fd_store, i, s->fd_store) - (void) service_dump_fd(i->fd, - i->fdname, - i == s->fd_store ? "File Descriptor Store Entry:" : " ", - f, - prefix); -} - -static void service_dump_extra_fds(Service *s, FILE *f, const char *prefix) { - assert(s); - assert(f); - assert(prefix); - - LIST_FOREACH(extra_fd, i, s->extra_fds) - (void) service_dump_fd(i->fd, - i->fdname, - i == s->extra_fds ? "Extra File Descriptor Entry:" : " ", - f, - prefix); -} - static void service_dump(Unit *u, FILE *f, const char *prefix) { Service *s = ASSERT_PTR(SERVICE(u)); const char *prefix2; @@ -1102,7 +1084,7 @@ static void service_dump(Unit *u, FILE *f, const char *prefix) { fprintf(f, "%sStatus Varlink Error: %s\n", prefix, s->status_varlink_error); - if (s->n_fd_store_max > 0) + if (s->n_fd_store_max > 0) { fprintf(f, "%sFile Descriptor Store Max: %u\n" "%sFile Descriptor Store Pin: %s\n" @@ -1111,7 +1093,20 @@ static void service_dump(Unit *u, FILE *f, const char *prefix) { prefix, exec_preserve_mode_to_string(s->fd_store_preserve_mode), prefix, s->n_fd_store); - service_dump_fdstore(s, f, prefix); + LIST_FOREACH(fd_store, i, s->fd_store) + (void) service_dump_fd(i->fd, + i->fdname, + i == s->fd_store ? "File Descriptor Store Entry:" : " ", + f, + prefix); + } + + FOREACH_ARRAY(i, s->extra_fds, s->n_extra_fds) + (void) service_dump_fd(i->fd, + i->fdname, + i == s->extra_fds ? "Extra File Descriptor Entry:" : " ", + f, + prefix); if (s->open_files) LIST_FOREACH(open_files, of, s->open_files) { @@ -1128,8 +1123,6 @@ static void service_dump(Unit *u, FILE *f, const char *prefix) { fprintf(f, "%sOpen File: %s\n", prefix, ofs); } - service_dump_extra_fds(s, f, prefix); - cgroup_context_dump(UNIT(s), f, prefix); } @@ -1465,7 +1458,7 @@ static int service_collect_fds( _cleanup_strv_free_ char **rfd_names = NULL; _cleanup_free_ int *rfds = NULL; - size_t rn_socket_fds = 0, rn_storage_fds = 0, rn_extra_fds = 0; + size_t rn_socket_fds = 0; int r; assert(s); @@ -1520,61 +1513,31 @@ static int service_collect_fds( } } - if (s->n_fd_store > 0) { - size_t n_fds; - char **nl; - int *t; - - t = reallocarray(rfds, rn_socket_fds + s->n_fd_store, sizeof(int)); + if (s->n_fd_store + s->n_extra_fds > 0) { + int *t = reallocarray(rfds, rn_socket_fds + s->n_fd_store + s->n_extra_fds, sizeof(int)); if (!t) return -ENOMEM; - rfds = t; - nl = reallocarray(rfd_names, rn_socket_fds + s->n_fd_store + 1, sizeof(char *)); + char **nl = reallocarray(rfd_names, rn_socket_fds + s->n_fd_store + s->n_extra_fds + 1, sizeof(char *)); if (!nl) return -ENOMEM; - rfd_names = nl; - n_fds = rn_socket_fds; + + size_t n_fds = rn_socket_fds; LIST_FOREACH(fd_store, fs, s->fd_store) { rfds[n_fds] = fs->fd; - rfd_names[n_fds] = strdup(strempty(fs->fdname)); + rfd_names[n_fds] = strdup(fs->fdname); if (!rfd_names[n_fds]) return -ENOMEM; - rn_storage_fds++; n_fds++; } - rfd_names[n_fds] = NULL; - } - - LIST_FOREACH(extra_fd, extra_fd, s->extra_fds) - rn_extra_fds++; - - if (rn_extra_fds > 0) { - size_t n_fds; - char **nl; - int *t; - - t = reallocarray(rfds, rn_socket_fds + rn_storage_fds + rn_extra_fds, sizeof(int)); - if (!t) - return -ENOMEM; - - rfds = t; - - nl = reallocarray(rfd_names, rn_socket_fds + rn_storage_fds + rn_extra_fds + 1, sizeof(char *)); - if (!nl) - return -ENOMEM; - - rfd_names = nl; - n_fds = rn_socket_fds + rn_storage_fds; - - LIST_FOREACH(extra_fd, extra_fd, s->extra_fds) { - rfds[n_fds] = extra_fd->fd; - rfd_names[n_fds] = strdup(extra_fd->fdname); + FOREACH_ARRAY(i, s->extra_fds, s->n_extra_fds) { + rfds[n_fds] = i->fd; + rfd_names[n_fds] = strdup(i->fdname); if (!rfd_names[n_fds]) return -ENOMEM; @@ -1587,8 +1550,8 @@ static int service_collect_fds( *fds = TAKE_PTR(rfds); *fd_names = TAKE_PTR(rfd_names); *n_socket_fds = rn_socket_fds; - *n_storage_fds = rn_storage_fds; - *n_extra_fds = rn_extra_fds; + *n_storage_fds = s->n_fd_store; + *n_extra_fds = s->n_extra_fds; return 0; } diff --git a/src/core/service.h b/src/core/service.h index 504595b0289..74b2b1c55ed 100644 --- a/src/core/service.h +++ b/src/core/service.h @@ -115,8 +115,6 @@ struct ServiceFDStore { struct ServiceExtraFD { int fd; char *fdname; - - LIST_FIELDS(ServiceExtraFD, extra_fd); }; struct Service { @@ -228,23 +226,24 @@ struct Service { unsigned n_fd_store_max; ExecPreserveMode fd_store_preserve_mode; - char *usb_function_descriptors; - char *usb_function_strings; - int stdin_fd; int stdout_fd; int stderr_fd; - OOMPolicy oom_policy; + /* If service spawned from transient unit, extra file descriptors can be passed via dbus API */ + ServiceExtraFD *extra_fds; + size_t n_extra_fds; LIST_HEAD(OpenFile, open_files); - /* If service spawned from transient unit, extra file descriptors can be passed via dbus API */ - LIST_HEAD(ServiceExtraFD, extra_fds); - int reload_signal; usec_t reload_begin_usec; + OOMPolicy oom_policy; + + char *usb_function_descriptors; + char *usb_function_strings; + /* The D-Bus request, we will reply once the operation is finished, so that callers can block */ sd_bus_message *mount_request; }; @@ -306,6 +305,3 @@ DEFINE_CAST(SERVICE, Service); /* Only exported for unit tests */ int service_deserialize_exec_command(Unit *u, const char *key, const char *value); - -ServiceExtraFD* service_extra_fd_free(ServiceExtraFD *efd); -DEFINE_TRIVIAL_CLEANUP_FUNC(ServiceExtraFD*, service_extra_fd_free);