[PATCH 0/5] Various cleanups to decrease amount of stack allocated buffers
The common theme is the attempt to decrease the use of stack allocated buffers. In few cases it's coupled with outher cleanups too. Most patches are standalone and can be individually applied. The last patch is also optional but can be applied only if the fixes before that are applied too. Peter Krempa (5): lxc: virLXCProcessReadLogOutput: Automatically close FD lxc: process: Rework reading errors from the log file util: netdev: Dynamically allocate 'struct nlattr' in virNetDevSwitchdevFeature remote: dispatch: Allocate 'virDomainDef' in ACL helpers dynamically build: Decrease maximum stack frame size to 2048 meson.build | 2 +- src/lxc/lxc_process.c | 100 ++++++++++++++++------------ src/remote/remote_daemon_dispatch.c | 17 +++-- src/util/virnetdev.c | 2 +- 4 files changed, 66 insertions(+), 55 deletions(-) -- 2.37.1
Switch to 'VIR_AUTOCLOSE' to simplify cleanup. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/lxc/lxc_process.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index fe5cab3df6..39dcf53c67 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -1076,8 +1076,7 @@ virLXCProcessReadLogOutput(virDomainObj *vm, char *buf, size_t buflen) { - int fd = -1; - int ret; + VIR_AUTOCLOSE fd = -1; if ((fd = open(logfile, O_RDONLY)) < 0) { virReportSystemError(errno, @@ -1090,17 +1089,10 @@ virLXCProcessReadLogOutput(virDomainObj *vm, virReportSystemError(errno, _("Unable to seek log file %s to %llu"), logfile, (unsigned long long)pos); - VIR_FORCE_CLOSE(fd); return -1; } - ret = virLXCProcessReadLogOutputData(vm, - fd, - buf, - buflen); - - VIR_FORCE_CLOSE(fd); - return ret; + return virLXCProcessReadLogOutputData(vm, fd, buf, buflen); } -- 2.37.1
Introduce 'virLXCProcessReportStartupLogError' which simplifies the error handling on startup of the LXC process when reading of the error log is needed. This function has unusual return value semantics but it helps to make the callers simpler. This patch also removes 2 1k stack'd buffers from virLXCProcessStart. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/lxc/lxc_process.c | 90 ++++++++++++++++++++++++++----------------- 1 file changed, 55 insertions(+), 35 deletions(-) diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 39dcf53c67..2a753ae1da 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -1096,6 +1096,43 @@ virLXCProcessReadLogOutput(virDomainObj *vm, } +/** + * virLXCProcessReportStartupLogError: + * @vm: domain object + * @logfile: path to the VM logfile + * @pos: position in @logfile to look for errors + * + * Looks for the error message from the LXC container process. + * Returns: + * -1: - When reading of error message failed. Reports appropriate error + * - When successfully read a non-empty error message. Reports an error with + * the following message + * 'guest failed to start: ' with the error from the log appended + * + * 0: - When reading the error was successful but the error log was empty. + */ +static int +virLXCProcessReportStartupLogError(virDomainObj *vm, + char *logfile, + off_t pos) +{ + size_t buflen = 1024; + g_autofree char *errbuf = g_new0(char, buflen); + int rc; + + if ((rc = virLXCProcessReadLogOutput(vm, logfile, pos, errbuf, buflen)) < 0) + return -1; + + if (rc == 0) + return 0; + + virReportError(VIR_ERR_INTERNAL_ERROR, + _("guest failed to start: %s"), errbuf); + + return -1; +} + + static int virLXCProcessEnsureRootFS(virDomainObj *vm) { @@ -1151,7 +1188,6 @@ int virLXCProcessStart(virLXCDriver * driver, g_auto(GStrv) veths = NULL; int handshakefds[4] = { -1, -1, -1, -1 }; /* two pipes */ off_t pos = -1; - char ebuf[1024]; g_autofree char *timestamp = NULL; int nsInheritFDs[VIR_LXC_DOMAIN_NAMESPACE_LAST]; g_autoptr(virCommand) cmd = NULL; @@ -1368,28 +1404,28 @@ int virLXCProcessStart(virLXCDriver * driver, goto cleanup; if (status != 0) { - if (virLXCProcessReadLogOutput(vm, logfile, pos, ebuf, - sizeof(ebuf)) <= 0) { - if (WIFEXITED(status)) - g_snprintf(ebuf, sizeof(ebuf), _("unexpected exit status %d"), + if (virLXCProcessReportStartupLogError(vm, logfile, pos) < 0) + goto cleanup; + + /* In case there isn't an error in the logs report one based on the exit status */ + if (WIFEXITED(status)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("guest failed to start: unexpected exit status %d"), WEXITSTATUS(status)); - else - g_snprintf(ebuf, sizeof(ebuf), "%s", _("terminated abnormally")); + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest failed to start: terminated abnormally")); } - virReportError(VIR_ERR_INTERNAL_ERROR, - _("guest failed to start: %s"), ebuf); goto cleanup; } /* It has started running, so get its pid */ if ((r = virPidFileReadPath(pidfile, &vm->pid)) < 0) { - if (virLXCProcessReadLogOutput(vm, logfile, pos, ebuf, sizeof(ebuf)) > 0) - virReportError(VIR_ERR_INTERNAL_ERROR, - _("guest failed to start: %s"), ebuf); - else - virReportSystemError(-r, - _("Failed to read pid file %s"), - pidfile); + if (virLXCProcessReportStartupLogError(vm, logfile, pos) < 0) + goto cleanup; + + /* In case there isn't an error in the logs report that we failed to read the pidfile */ + virReportSystemError(-r, _("Failed to read pid file %s"), pidfile); goto cleanup; } @@ -1422,13 +1458,7 @@ int virLXCProcessStart(virLXCDriver * driver, /* The first synchronization point is when the controller creates CGroups. */ if (lxcContainerWaitForContinue(handshakefds[0]) < 0) { - char out[1024]; - - if (!(virLXCProcessReadLogOutput(vm, logfile, pos, out, 1024) < 0)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("guest failed to start: %s"), out); - } - + virLXCProcessReportStartupLogError(vm, logfile, pos); goto cleanup; } @@ -1461,13 +1491,7 @@ int virLXCProcessStart(virLXCDriver * driver, /* The second synchronization point is when the controller finished * creating the container. */ if (lxcContainerWaitForContinue(handshakefds[0]) < 0) { - char out[1024]; - - if (!(virLXCProcessReadLogOutput(vm, logfile, pos, out, 1024) < 0)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("guest failed to start: %s"), out); - } - + virLXCProcessReportStartupLogError(vm, logfile, pos); goto cleanup; } @@ -1476,11 +1500,7 @@ int virLXCProcessStart(virLXCDriver * driver, /* Intentionally overwrite the real monitor error message, * since a better one is almost always found in the logs */ - if (virLXCProcessReadLogOutput(vm, logfile, pos, ebuf, sizeof(ebuf)) > 0) { - virResetLastError(); - virReportError(VIR_ERR_INTERNAL_ERROR, - _("guest failed to start: %s"), ebuf); - } + virLXCProcessReportStartupLogError(vm, logfile, pos); goto cleanup; } -- 2.37.1
At time of writing DEVLINK_ATTR_MAX equals to 176, thus the stack'd size of the pointer array is almost 1.4kiB. Allocate it dynamically. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virnetdev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 6c6f6dee42..66cfc5d781 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -3210,7 +3210,7 @@ virNetDevSwitchdevFeature(const char *ifname, struct nl_msg *nl_msg = NULL; g_autofree struct nlmsghdr *resp = NULL; unsigned int recvbuflen; - struct nlattr *tb[DEVLINK_ATTR_MAX + 1] = {NULL, }; + g_autofree struct nlattr **tb = g_new0(struct nlattr *, DEVLINK_ATTR_MAX + 1); g_autoptr(virPCIDevice) pci_device_ptr = NULL; struct genlmsghdr gmsgh = { .cmd = DEVLINK_CMD_ESWITCH_GET, -- 2.37.1
At time of this patch struct 'virDomainDef' has 1736 bytes. Allocate it dynamically to keep the stack frame size in reasonable values. This patch also fixes remoteRelayDomainQemuMonitorEventCheckACL, where we didn't clear the stack'd variable prior to use. Fortunately for now the code didn't look at anything else than what the code overwrote. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/remote/remote_daemon_dispatch.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c index dc5790f077..4f42cdc610 100644 --- a/src/remote/remote_daemon_dispatch.c +++ b/src/remote/remote_daemon_dispatch.c @@ -154,22 +154,21 @@ static bool remoteRelayDomainEventCheckACL(virNetServerClient *client, virConnectPtr conn, virDomainPtr dom) { - virDomainDef def; + g_autofree virDomainDef *def = g_new0(virDomainDef, 1); g_autoptr(virIdentity) identity = NULL; bool ret = false; /* For now, we just create a virDomainDef with enough contents to * satisfy what viraccessdriverpolkit.c references. This is a bit * fragile, but I don't know of anything better. */ - memset(&def, 0, sizeof(def)); - def.name = dom->name; - memcpy(def.uuid, dom->uuid, VIR_UUID_BUFLEN); + def->name = dom->name; + memcpy(def->uuid, dom->uuid, VIR_UUID_BUFLEN); if (!(identity = virNetServerClientGetIdentity(client))) goto cleanup; if (virIdentitySetCurrent(identity) < 0) goto cleanup; - ret = virConnectDomainEventRegisterAnyCheckACL(conn, &def); + ret = virConnectDomainEventRegisterAnyCheckACL(conn, def); cleanup: ignore_value(virIdentitySetCurrent(NULL)); @@ -284,21 +283,21 @@ static bool remoteRelayDomainQemuMonitorEventCheckACL(virNetServerClient *client, virConnectPtr conn, virDomainPtr dom) { - virDomainDef def; + g_autofree virDomainDef *def = g_new0(virDomainDef, 1); g_autoptr(virIdentity) identity = NULL; bool ret = false; /* For now, we just create a virDomainDef with enough contents to * satisfy what viraccessdriverpolkit.c references. This is a bit * fragile, but I don't know of anything better. */ - def.name = dom->name; - memcpy(def.uuid, dom->uuid, VIR_UUID_BUFLEN); + def->name = dom->name; + memcpy(def->uuid, dom->uuid, VIR_UUID_BUFLEN); if (!(identity = virNetServerClientGetIdentity(client))) goto cleanup; if (virIdentitySetCurrent(identity) < 0) goto cleanup; - ret = virConnectDomainQemuMonitorEventRegisterCheckACL(conn, &def); + ret = virConnectDomainQemuMonitorEventRegisterCheckACL(conn, def); cleanup: ignore_value(virIdentitySetCurrent(NULL)); -- 2.37.1
After recent cleanups we can now restrict the maximum stack frame size to 2k. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- meson.build | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/meson.build b/meson.build index ed9f4b3f70..9d3703a321 100644 --- a/meson.build +++ b/meson.build @@ -226,7 +226,7 @@ alloc_max = run_command( ) # sanitizer instrumentation may enlarge stack frames -stack_frame_size = get_option('b_sanitize') == 'none' ? 4096 : 32768 +stack_frame_size = get_option('b_sanitize') == 'none' ? 2048 : 32768 # array_bounds=2 check triggers false positive on some GCC # versions when using sanitizers. Seen on Fedora 34 with -- 2.37.1
On a Thursday in 2022, Peter Krempa wrote:
The common theme is the attempt to decrease the use of stack allocated buffers. In few cases it's coupled with outher cleanups too.
Most patches are standalone and can be individually applied. The last patch is also optional but can be applied only if the fixes before that are applied too.
Peter Krempa (5): lxc: virLXCProcessReadLogOutput: Automatically close FD lxc: process: Rework reading errors from the log file util: netdev: Dynamically allocate 'struct nlattr' in virNetDevSwitchdevFeature remote: dispatch: Allocate 'virDomainDef' in ACL helpers dynamically build: Decrease maximum stack frame size to 2048
meson.build | 2 +- src/lxc/lxc_process.c | 100 ++++++++++++++++------------ src/remote/remote_daemon_dispatch.c | 17 +++-- src/util/virnetdev.c | 2 +- 4 files changed, 66 insertions(+), 55 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Peter Krempa