We only check now for virObjectWait failures in virshRunConsole but we'd better check and for other failures too. Anyway if failure happened we need to shutdown console to stop delivering events from event loop thread or we are in trouble as console is freed on virshRunConsole exit. And we need to add refcounter to console object because event can be delivered after we remove fd handle/stream callback. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com&gt; --- tools/virsh-console.c | 161 +++++++++++++++++++++++++++++++++++++------------- 1 file changed, 119 insertions(+), 42 deletions(-) diff --git a/tools/virsh-console.c b/tools/virsh-console.c index 045a636..c0c3f90 100644 --- a/tools/virsh-console.c +++ b/tools/virsh-console.c @@ -60,9 +60,10 @@ struct virConsoleBuffer { typedef struct virConsole virConsole; typedef virConsole *virConsolePtr; struct virConsole { + virObjectLockable parent; + virStreamPtr st; bool quit; - virMutex lock; virCond cond; int stdinWatch; @@ -74,6 +75,19 @@ struct virConsole { char escapeChar; }; +static virClassPtr virConsoleClass; +static void virConsoleDispose(void *obj); + +static int +virConsoleOnceInit(void) +{ + if (!VIR_CLASS_NEW(virConsole, virClassForObjectLockable())) + return -1; + + return 0; +} + +VIR_ONCE_GLOBAL_INIT(virConsole); static void virConsoleHandleSignal(int sig ATTRIBUTE_UNUSED) @@ -104,16 +118,14 @@ virConsoleShutdown(virConsolePtr con) static void -virConsoleFree(virConsolePtr con) +virConsoleDispose(void *obj) { - if (!con) - return; + virConsolePtr con = obj; if (con->st) virStreamFree(con->st); - virMutexDestroy(&con->lock); + virCondDestroy(&con->cond); - VIR_FREE(con); } @@ -123,6 +135,12 @@ virConsoleEventOnStream(virStreamPtr st, { virConsolePtr con = opaque; + virObjectLock(con); + + /* we got late event after console shutdowned */ + if (!con->st) + goto cleanup; + if (events & VIR_STREAM_EVENT_READABLE) { size_t avail = con->streamToTerminal.length - con->streamToTerminal.offset; @@ -132,7 +150,7 @@ virConsoleEventOnStream(virStreamPtr st, if (VIR_REALLOC_N(con->streamToTerminal.data, con->streamToTerminal.length + 1024) < 0) { virConsoleShutdown(con); - return; + goto cleanup; } con->streamToTerminal.length += 1024; avail += 1024; @@ -143,10 +161,10 @@ virConsoleEventOnStream(virStreamPtr st, con->streamToTerminal.offset, avail); if (got == -2) - return; /* blocking */ + goto cleanup; /* blocking */ if (got <= 0) { virConsoleShutdown(con); - return; + goto cleanup; } con->streamToTerminal.offset += got; if (con->streamToTerminal.offset) @@ -162,10 +180,10 @@ virConsoleEventOnStream(virStreamPtr st, con->terminalToStream.data, con->terminalToStream.offset); if (done == -2) - return; /* blocking */ + goto cleanup; /* blocking */ if (done < 0) { virConsoleShutdown(con); - return; + goto cleanup; } memmove(con->terminalToStream.data, con->terminalToStream.data + done, @@ -187,6 +205,9 @@ virConsoleEventOnStream(virStreamPtr st, events & VIR_STREAM_EVENT_HANGUP) { virConsoleShutdown(con); } + + cleanup: + virObjectUnlock(con); } @@ -198,6 +219,12 @@ virConsoleEventOnStdin(int watch ATTRIBUTE_UNUSED, { virConsolePtr con = opaque; + virObjectLock(con); + + /* we got late event after console shutdowned */ + if (!con->st) + goto cleanup; + if (events & VIR_EVENT_HANDLE_READABLE) { size_t avail = con->terminalToStream.length - con->terminalToStream.offset; @@ -207,7 +234,7 @@ virConsoleEventOnStdin(int watch ATTRIBUTE_UNUSED, if (VIR_REALLOC_N(con->terminalToStream.data, con->terminalToStream.length + 1024) < 0) { virConsoleShutdown(con); - return; + goto cleanup; } con->terminalToStream.length += 1024; avail += 1024; @@ -220,15 +247,15 @@ virConsoleEventOnStdin(int watch ATTRIBUTE_UNUSED, if (got < 0) { if (errno != EAGAIN) virConsoleShutdown(con); - return; + goto cleanup; } if (got == 0) { virConsoleShutdown(con); - return; + goto cleanup; } if (con->terminalToStream.data[con->terminalToStream.offset] == con->escapeChar) { virConsoleShutdown(con); - return; + goto cleanup; } con->terminalToStream.offset += got; @@ -242,6 +269,9 @@ virConsoleEventOnStdin(int watch ATTRIBUTE_UNUSED, events & VIR_EVENT_HANDLE_HANGUP) { virConsoleShutdown(con); } + + cleanup: + virObjectUnlock(con); } @@ -253,6 +283,12 @@ virConsoleEventOnStdout(int watch ATTRIBUTE_UNUSED, { virConsolePtr con = opaque; + virObjectLock(con); + + /* we got late event after console shutdowned */ + if (!con->st) + goto cleanup; + if (events & VIR_EVENT_HANDLE_WRITABLE && con->streamToTerminal.offset) { ssize_t done; @@ -263,7 +299,7 @@ virConsoleEventOnStdout(int watch ATTRIBUTE_UNUSED, if (done < 0) { if (errno != EAGAIN) virConsoleShutdown(con); - return; + goto cleanup; } memmove(con->streamToTerminal.data, con->streamToTerminal.data + done, @@ -285,6 +321,38 @@ virConsoleEventOnStdout(int watch ATTRIBUTE_UNUSED, events & VIR_EVENT_HANDLE_HANGUP) { virConsoleShutdown(con); } + + cleanup: + virObjectUnlock(con); +} + + +static virConsolePtr +virConsoleNew(void) +{ + virConsolePtr con; + + if (virConsoleInitialize() < 0) + return NULL; + + if (!(con = virObjectNew(virConsoleClass))) + return NULL; + + if (virCondInit(&con->cond) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot initialize console condition")); + + goto error; + } + + con->stdinWatch = -1; + con->stdoutWatch = -1; + + return con; + + error: + virObjectUnref(con); + return NULL; } @@ -324,6 +392,11 @@ virshRunConsole(vshControl *ctl, if (vshTTYMakeRaw(ctl, true) < 0) goto resettty; + if (!(con = virConsoleNew())) + goto resettty; + + virObjectLock(con); + /* Trap all common signals so that we can safely restore the original * terminal settings on STDIN before the process exits - people don't like * being left with a messed up terminal ! */ @@ -333,9 +406,6 @@ virshRunConsole(vshControl *ctl, sigaction(SIGHUP, &sighandler, &old_sighup); sigaction(SIGPIPE, &sighandler, &old_sigpipe); - if (VIR_ALLOC(con) < 0) - goto cleanup; - con->escapeChar = virshGetEscapeChar(priv->escapeChar); con->st = virStreamNew(virDomainGetConnect(dom), VIR_STREAM_NONBLOCK); @@ -345,42 +415,49 @@ virshRunConsole(vshControl *ctl, if (virDomainOpenConsole(dom, dev_name, con->st, flags) < 0) goto cleanup; - if (virCondInit(&con->cond) < 0 || virMutexInit(&con->lock) < 0) + virObjectRef(con); + if ((con->stdinWatch = virEventAddHandle(STDIN_FILENO, + VIR_EVENT_HANDLE_READABLE, + virConsoleEventOnStdin, + con, + virObjectFreeCallback)) < 0) { + virObjectUnref(con); goto cleanup; + } - virMutexLock(&con->lock); - - con->stdinWatch = virEventAddHandle(STDIN_FILENO, - VIR_EVENT_HANDLE_READABLE, - virConsoleEventOnStdin, - con, - NULL); - con->stdoutWatch = virEventAddHandle(STDOUT_FILENO, - 0, - virConsoleEventOnStdout, - con, - NULL); + virObjectRef(con); + if ((con->stdoutWatch = virEventAddHandle(STDOUT_FILENO, + 0, + virConsoleEventOnStdout, + con, + virObjectFreeCallback)) < 0) { + virObjectUnref(con); + goto cleanup; + } - virStreamEventAddCallback(con->st, - VIR_STREAM_EVENT_READABLE, - virConsoleEventOnStream, - con, - NULL); + virObjectRef(con); + if (virStreamEventAddCallback(con->st, + VIR_STREAM_EVENT_READABLE, + virConsoleEventOnStream, + con, + virObjectFreeCallback) < 0) { + virObjectUnref(con); + goto cleanup; + } while (!con->quit) { - if (virCondWait(&con->cond, &con->lock) < 0) { - virMutexUnlock(&con->lock); + if (virCondWait(&con->cond, &con->parent.lock) < 0) { VIR_ERROR(_("unable to wait on console condition")); goto cleanup; } } - virMutexUnlock(&con->lock); - ret = 0; cleanup: - virConsoleFree(con); + virConsoleShutdown(con); + virObjectUnlock(con); + virObjectUnref(con); /* Restore original signal handlers */ sigaction(SIGQUIT, &old_sigquit, NULL); -- 1.8.3.1

On 22.02.2019 17:55, John Ferlan wrote: We can use second construction safely here because we hold console lock and virConsoleEventOnStdin grabs the lock too. If not we could have error event/console shutdown/removing handle/calling free callback before we have chance to reference console on behalf of the watch and this would lead to use after free in virshRunConsole. So first construction is a bit more robust. You just first refcount object and then pass it further without extra consideration. That's why we unref console explicitly on virEventAddHandle failure - because callback does not get called. I'd better use first variant - this way we make virConsoleShutdown idempotent. Nikolay

On 2/15/19 9:39 AM, Nikolay Shirokovskiy wrote: If the console was disconnected due to a connection problem or a problem on the server side it is convenient If the error come from the API, then the error is save in a virsh global variable. However, since success is returned from virshRunConsole and we reach the waiting stage, then the error is never reported. Let's track the error in the event loop. [I think this paragraph could be dropped in favor of below] Written this way it feels like an extra patch; however, I know it's not... How about... Since we'll be sending the error to the consumer, each failure path from the event handlers needs to be augmented to provide what error generated the failure so that can be reported properly during cleanup processing from virshRunConsole. if (got <= 0) { if (got == 0) virReportError(...) virConsoleShutdown goto cleanup } "cannot" is fine (similar below) John