When vTPM is secured via virSecret libvirt passes the secret value via an FD when swtpm is started (arguments --key and --migration-key). The writing of the secret into the FDs is handled via virCommand, specifically qemu_tpm calls virCommandSetSendBuffer()) and then virCommandRunAsync() spawns a thread to handle writing into the FD via virCommandDoAsyncIOHelper. But the thread is not created unless VIR_EXEC_ASYNC_IO flag is set, which it isn't. In order to fix it, virCommandDoAsyncIO() must be called. The credit goes to Marc-André Lureau <marcandre.lureau(a)redhat.com&gt; who has done all the debugging and proposed fix in the bugzilla. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2064115 Fixes: a9c500d2b50c5c041a1bb6ae9724402cf1cec8fe Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/qemu/qemu_tpm.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index 50f9caabf3..56bccee128 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -899,6 +899,7 @@ qemuTPMEmulatorStart(virQEMUDriver *driver, if (!(pidfile = qemuTPMEmulatorPidFileBuildPath(cfg->swtpmStateDir, shortName))) return -1; + virCommandDoAsyncIO(cmd); virCommandDaemonize(cmd); virCommandSetPidFile(cmd, pidfile); virCommandSetErrorFD(cmd, &errfd); -- 2.34.1

When virCommandSetSendBuffer() is used over a virCommand that is (or will be) daemonized, then VIR_EXEC_ASYNC_IO the command must have VIR_EXEC_ASYNC_IO flag set no later than at virCommandRunAsync() phase so that the thread that's doing IO is spawned and thus buffers can be sent to the process. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/util/vircommand.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 41cf552d7b..5f22bd0ac3 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -1719,6 +1719,9 @@ virCommandFreeSendBuffers(virCommand *cmd) * @buffer is always stolen regardless of the return value. This function * doesn't raise a libvirt error, but rather propagates the error via virCommand. * Thus callers don't need to take a special action if -1 is returned. + * + * When the @cmd is daemonized via virCommandDaemonize() remember to request + * asynchronous IO via virCommandDoAsyncIO(). */ int virCommandSetSendBuffer(virCommand *cmd, -- 2.34.1

On 3/23/22 08:48, Marc-André Lureau wrote: Yes. So what we could do, is to enable VIR_EXEC_ASYNC_IO flag in virCommandRun() and require explicit call to DoAsyncIO() for virCommandRunAsync() case. This way, it'll be consistent with the rest of buffer passing functions, like virCommandSetInputBuffer(), virCommandSetOutputBuffer() or virCommandSetErrorBuffer(). Would this work for you? Michal

In test27() the virCommandSetSendBuffer() is used, which expects unsigned char. Use that type for variables which are passed to the function. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- tests/commandtest.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/commandtest.c b/tests/commandtest.c index 74e60a072b..9d4de151f0 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -1043,8 +1043,8 @@ static int test27(const void *unused G_GNUC_UNUSED) int buf2fd; size_t buflen = 1024 * 128; g_autofree char *buffer0 = NULL; - g_autofree char *buffer1 = NULL; - g_autofree char *buffer2 = NULL; + g_autofree unsigned char *buffer1 = NULL; + g_autofree unsigned char *buffer2 = NULL; g_autofree char *outactual = NULL; g_autofree char *erractual = NULL; g_autofree char *outexpect = NULL; @@ -1057,8 +1057,8 @@ static int test27(const void *unused G_GNUC_UNUSED) "END STDERR\n" buffer0 = g_new0(char, buflen); - buffer1 = g_new0(char, buflen); - buffer2 = g_new0(char, buflen); + buffer1 = g_new0(unsigned char, buflen); + buffer2 = g_new0(unsigned char, buflen); memset(buffer0, 'H', buflen - 2); buffer0[buflen - 2] = '\n'; @@ -1077,8 +1077,8 @@ static int test27(const void *unused G_GNUC_UNUSED) errexpect = g_strdup_printf(TEST27_ERREXPECT_TEMP, buffer0, buffer1, buffer2); - buf1fd = virCommandSetSendBuffer(cmd, (unsigned char *) g_steal_pointer(&buffer1), buflen - 1); - buf2fd = virCommandSetSendBuffer(cmd, (unsigned char *) g_steal_pointer(&buffer2), buflen - 1); + buf1fd = virCommandSetSendBuffer(cmd, g_steal_pointer(&buffer1), buflen - 1); + buf2fd = virCommandSetSendBuffer(cmd, g_steal_pointer(&buffer2), buflen - 1); virCommandAddArg(cmd, "--readfd"); virCommandAddArgFormat(cmd, "%d", buf1fd); -- 2.34.1

Introduce a test case which ensures that a daemonized process can work with virCommandSetSendBuffer() when async IO is enabled. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- tests/commanddata/test29.log | 19 ++++++++ tests/commandtest.c | 84 ++++++++++++++++++++++++++++++++++++ 2 files changed, 103 insertions(+) create mode 100644 tests/commanddata/test29.log diff --git a/tests/commanddata/test29.log b/tests/commanddata/test29.log new file mode 100644 index 0000000000..49993a4947 --- /dev/null +++ b/tests/commanddata/test29.log @@ -0,0 +1,19 @@ +ARG:--close-stdin +ARG:--readfd +ARG:3 +ENV:DISPLAY=:0.0 +ENV:HOME=/home/test +ENV:HOSTNAME=test +ENV:LANG=C +ENV:LOGNAME=test +ENV:PATH=/usr/bin:/bin +ENV:TMPDIR=/tmp +ENV:USER=test +FD:0 +FD:1 +FD:2 +FD:3 +FD:6 +DAEMON:yes +CWD:/ +UMASK:0022 diff --git a/tests/commandtest.c b/tests/commandtest.c index 10a051124d..573a4f250d 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -25,6 +25,7 @@ #include <sys/stat.h> #ifndef WIN32 # include <sys/wait.h> +# include <poll.h> #endif #include <fcntl.h> @@ -1157,6 +1158,88 @@ test28(const void *unused G_GNUC_UNUSED) } +static int +test29(const void *unused G_GNUC_UNUSED) +{ + g_autoptr(virCommand) cmd = virCommandNew(abs_builddir "/commandhelper"); + g_autofree char *pidfile = virPidFileBuildPath(abs_builddir, "commandhelper"); + pid_t pid; + int buffd; + VIR_AUTOCLOSE outfd = -1; + size_t buflen = 1024 * 10; + g_autofree unsigned char *buffer = NULL; + g_autofree char *outactual = NULL; + g_autofree char *outexpect = NULL; + size_t i; + size_t outactuallen = 0; + int ret = -1; + + if (!pidfile) + return -1; + + buffer = g_new0(unsigned char, buflen + 1); + for (i = 0; i < buflen; i++) { + buffer[i] = 'a' + i % ('z' - 'a' + 1); + } + buffer[buflen] = '\0'; + + outexpect = g_strdup_printf("BEGIN STDOUT\n%sEND STDOUT\n", buffer); + + buffd = virCommandSetSendBuffer(cmd, &buffer, buflen); + + virCommandAddArg(cmd, "--close-stdin"); + virCommandAddArg(cmd, "--readfd"); + virCommandAddArgFormat(cmd, "%d", buffd); + + virCommandSetOutputFD(cmd, &outfd); + virCommandSetPidFile(cmd, pidfile); + virCommandDaemonize(cmd); + virCommandDoAsyncIO(cmd); + + if (virCommandRun(cmd, NULL) < 0) { + fprintf(stderr, "Cannot run child %s\n", virGetLastErrorMessage()); + goto cleanup; + } + + if (virPidFileReadPath(pidfile, &pid) < 0) { + fprintf(stderr, "cannot read pidfile: %s\n", pidfile); + goto cleanup; + } + + while (1) { + char buf[1024] = { 0 }; + int rc = 0; + + rc = read(outfd, buf, sizeof(buf)); + if (rc < 0) { + fprintf(stderr, "cannot read from output pipe: errno=%d\n", errno); + goto cleanup; + } + + if (rc == 0) + break; + + outactual = g_renew(char, outactual, outactuallen + rc + 1); + memcpy(outactual + outactuallen, buf, rc); + outactuallen += rc; + outactual[outactuallen] = '\0'; + } + + if (STRNEQ_NULLABLE(outactual, outexpect)) { + virTestDifference(stderr, outexpect, outactual); + goto cleanup; + } + + ret = checkoutput("test29"); + + cleanup: + if (pidfile) + unlink(pidfile); + + return ret; +} + + static int mymain(void) { @@ -1254,6 +1337,7 @@ mymain(void) DO_TEST(test26); DO_TEST(test27); DO_TEST(test28); + DO_TEST(test29); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.34.1