On 22.02.2019 17:55, John Ferlan wrote:
>
>
> On 2/15/19 9:39 AM, Nikolay Shirokovskiy wrote:
>> 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@xxxxxxxxxxxxx>
>> ---
>> tools/virsh-console.c | 161 +++++++++++++++++++++++++++++++++++++-------------
>> 1 file changed, 119 insertions(+), 42 deletions(-)
>>
>
> There's three things going on in this patch, let's split into convert
> virConsole to virObjectLockable, the usage of locks in the event
> callback methods, and finally handling the late event.
>
> FWIW: I could see the order being
>
> 1. Add the virMutex{Lock|Unlock} into the functions and use cleanup path
> logic since right now @con is being altered without the lock.
>
> 2. Convert to using virObjectLockable
>
> 3. Add logic to handle late event
>
>
>> 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 */
>
> s/shutdowned/was shutdown/
>
> (repeats)
>
>> + 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;
>> + }
>
> Is there a specific reason to go with:
>
> ObjectRef
> if condition
> error path
> ObjectUnref>
> instead of
>
> if condition
> ObjectRef
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.
>
> I guess it doesn't matter too much, but looks odd to me... If the
> virEventAddHandle fails, the virObjectFreeCallback isn't called so @con
> wouldn't be Unref'd until at least you Unlock'd @con.
That's why we unref console explicitly on virEventAddHandle failure -
because callback does not get called.
>
>>
>> - 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);
>
> Should virConsoleShutdown be adjusted to avoid virCondSignal if
> con->quit is tree or this call be adjusted to not call if con->quit is
> true... IOW: Don't call it twice...
>
I'd better use first variant - this way we make virConsoleShutdown idempotent.
Nikolay
>
>> + virObjectUnlock(con);
>> + virObjectUnref(con);
>>
>> /* Restore original signal handlers */
>> sigaction(SIGQUIT, &old_sigquit, NULL);
>>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
