Direct leak of 104 byte(s) in 1 object(s) allocated from: #0 0x7f904bfbe12b (/lib64/liblsan.so.0+0xe12b) #1 0x7f904ba0ad67 in virAlloc ../../src/util/viralloc.c:144 #2 0x7f904bbc11a4 in virNetMessageNew ../../src/rpc/virnetmessage.c:42 #3 0x7f904bbb8e77 in virNetServerClientNewInternal ../../src/rpc/virnetserverclient.c:392 #4 0x7f904bbb9921 in virNetServerClientNew ../../src/rpc/virnetserverclient.c:440 #5 0x402ce5 in testIdentity ../../tests/virnetserverclienttest.c:55 #6 0x403bed in virTestRun ../../tests/testutils.c:180 #7 0x402c1e in mymain ../../tests/virnetserverclienttest.c:146 #8 0x404c80 in virTestMain ../../tests/testutils.c:1119 #9 0x4030d5 in main ../../tests/virnetserverclienttest.c:152 #10 0x7f9047f7f889 in __libc_start_main (/lib64/libc.so.6+0x20889) Indirect leak of 4 byte(s) in 1 object(s) allocated from: #0 0x7f904bfbe12b (/lib64/liblsan.so.0+0xe12b) #1 0x7f904ba0adc7 in virAllocN ../../src/util/viralloc.c:191 #2 0x7f904bbb8ec7 in virNetServerClientNewInternal ../../src/rpc/virnetserverclient.c:395 #3 0x7f904bbb9921 in virNetServerClientNew ../../src/rpc/virnetserverclient.c:440 #4 0x402ce5 in testIdentity ../../tests/virnetserverclienttest.c:55 #5 0x403bed in virTestRun ../../tests/testutils.c:180 #6 0x402c1e in mymain ../../tests/virnetserverclienttest.c:146 #7 0x404c80 in virTestMain ../../tests/testutils.c:1119 #8 0x4030d5 in main ../../tests/virnetserverclienttest.c:152 #9 0x7f9047f7f889 in __libc_start_main (/lib64/libc.so.6+0x20889) SUMMARY: LeakSanitizer: 108 byte(s) leaked in 2 allocation(s). Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com&gt; Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; --- tests/virnetserverclienttest.c | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/virnetserverclienttest.c b/tests/virnetserverclienttest.c index c80870141c98..96b69b3e45ea 100644 --- a/tests/virnetserverclienttest.c +++ b/tests/virnetserverclienttest.c @@ -129,6 +129,7 @@ static int testIdentity(const void *opaque ATTRIBUTE_UNUSED) ret = 0; cleanup: virObjectUnref(sock); + virNetServerClientClose(client); virObjectUnref(client); virObjectUnref(ident); VIR_FORCE_CLOSE(sv[0]); -- 2.13.4

Add typedef for the anonymous enum used for the authentication methods and remove the default case. This allows the usage of the type in a switch statement and taking advantage of the compilers feature to detect uncovered cases. Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com&gt; Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com&gt; Reviewed-by: Stefan Zimmermann <stzi(a)linux.vnet.ibm.com&gt; Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; --- daemon/remote.c | 4 +--- src/rpc/virnetserverservice.h | 4 ++-- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index 1df02c1f5042..45cb99a56146 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -3279,7 +3279,7 @@ remoteDispatchAuthList(virNetServerPtr server, if (VIR_ALLOC_N(ret->types.types_val, ret->types.types_len) < 0) goto cleanup; - switch (auth) { + switch ((virNetServerServiceAuthMethods) auth) { case VIR_NET_SERVER_SERVICE_AUTH_NONE: ret->types.types_val[0] = REMOTE_AUTH_NONE; break; @@ -3289,8 +3289,6 @@ remoteDispatchAuthList(virNetServerPtr server, case VIR_NET_SERVER_SERVICE_AUTH_SASL: ret->types.types_val[0] = REMOTE_AUTH_SASL; break; - default: - ret->types.types_val[0] = REMOTE_AUTH_NONE; } rv = 0; diff --git a/src/rpc/virnetserverservice.h b/src/rpc/virnetserverservice.h index 9afa0b13dbc6..5d8c583db2e2 100644 --- a/src/rpc/virnetserverservice.h +++ b/src/rpc/virnetserverservice.h @@ -27,11 +27,11 @@ # include "virnetserverprogram.h" # include "virobject.h" -enum { +typedef enum { VIR_NET_SERVER_SERVICE_AUTH_NONE = 0, VIR_NET_SERVER_SERVICE_AUTH_SASL, VIR_NET_SERVER_SERVICE_AUTH_POLKIT, -}; +} virNetServerServiceAuthMethods; typedef int (*virNetServerServiceDispatchFunc)(virNetServerServicePtr svc, virNetSocketPtr sock, -- 2.13.4

This makes the code more efficient. Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com&gt; Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com&gt; Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com&gt; Reviewed-by: Stefan Zimmermann <stzi(a)linux.vnet.ibm.com&gt; Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/rpc/virnetserverprogram.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/rpc/virnetserverprogram.c b/src/rpc/virnetserverprogram.c index 556c91605f90..1e8dfc2f1128 100644 --- a/src/rpc/virnetserverprogram.c +++ b/src/rpc/virnetserverprogram.c @@ -400,8 +400,8 @@ virNetServerProgramDispatchCall(virNetServerProgramPtr prog, /* If client is marked as needing auth, don't allow any RPC ops * which are except for authentication ones */ - if (virNetServerClientNeedAuth(client) && - dispatcher->needAuth) { + if (dispatcher->needAuth && + virNetServerClientNeedAuth(client)) { /* Explicitly *NOT* calling remoteDispatchAuthError() because we want back-compatibility with libvirt clients which don't support the VIR_ERR_AUTH_FAILED error code */ -- 2.13.4

The lock for @client must not only be held for the duration of checking whether the client wants to close, but also for as long as we're closing the client. The same applies to the tracking of authentications. Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com&gt; --- src/libvirt_remote.syms | 5 +++-- src/rpc/virnetserver.c | 20 ++++++++++++++------ src/rpc/virnetserverclient.c | 35 ++++++++++++++++++----------------- src/rpc/virnetserverclient.h | 5 +++-- 4 files changed, 38 insertions(+), 27 deletions(-) diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms index cecd71c49e7f..4e684ef69514 100644 --- a/src/libvirt_remote.syms +++ b/src/libvirt_remote.syms @@ -125,6 +125,7 @@ virNetServerUpdateServices; # rpc/virnetserverclient.h virNetServerClientAddFilter; virNetServerClientClose; +virNetServerClientCloseLocked; virNetServerClientDelayedClose; virNetServerClientGetAuth; virNetServerClientGetFD; @@ -138,7 +139,7 @@ virNetServerClientGetUNIXIdentity; virNetServerClientImmediateClose; virNetServerClientInit; virNetServerClientInitKeepAlive; -virNetServerClientIsClosed; +virNetServerClientIsClosedLocked; virNetServerClientIsLocal; virNetServerClientIsSecure; virNetServerClientLocalAddrStringSASL; @@ -156,7 +157,7 @@ virNetServerClientSetCloseHook; virNetServerClientSetDispatcher; virNetServerClientSetReadonly; virNetServerClientStartKeepAlive; -virNetServerClientWantClose; +virNetServerClientWantCloseLocked; # rpc/virnetservermdns.h diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c index 43f889e2a037..57cbfb59ab53 100644 --- a/src/rpc/virnetserver.c +++ b/src/rpc/virnetserver.c @@ -285,8 +285,10 @@ int virNetServerAddClient(virNetServerPtr srv, goto error; srv->clients[srv->nclients-1] = virObjectRef(client); - if (virNetServerClientNeedAuth(client)) + virObjectLock(client); + if (virNetServerClientNeedAuthLocked(client)) virNetServerTrackPendingAuthLocked(srv); + virObjectUnlock(client); virNetServerCheckLimits(srv); @@ -847,6 +849,7 @@ void virNetServerProcessClients(virNetServerPtr srv) { size_t i; + virNetServerClientPtr client; virObjectLock(srv); @@ -855,15 +858,18 @@ virNetServerProcessClients(virNetServerPtr srv) /* Coverity 5.3.0 couldn't see that srv->clients is non-NULL * if srv->nclients is non-zero. */ sa_assert(srv->clients); - if (virNetServerClientWantClose(srv->clients[i])) - virNetServerClientClose(srv->clients[i]); - if (virNetServerClientIsClosed(srv->clients[i])) { - virNetServerClientPtr client = srv->clients[i]; + client = srv->clients[i]; + virObjectLock(client); + if (virNetServerClientWantCloseLocked(client)) + virNetServerClientCloseLocked(client); + + if (virNetServerClientIsClosedLocked(client)) { VIR_DELETE_ELEMENT(srv->clients, i, srv->nclients); - if (virNetServerClientNeedAuth(client)) + if (virNetServerClientNeedAuthLocked(client)) virNetServerTrackCompletedAuthLocked(srv); + virObjectUnlock(client); virNetServerCheckLimits(srv); @@ -872,6 +878,8 @@ virNetServerProcessClients(virNetServerPtr srv) virObjectLock(srv); goto reprocess; + } else { + virObjectUnlock(client); } } diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c index afe4fb47a256..dee94450dfa3 100644 --- a/src/rpc/virnetserverclient.c +++ b/src/rpc/virnetserverclient.c @@ -987,17 +987,15 @@ void virNetServerClientDispose(void *obj) * Full free of the client is done later in a safe point * where it can be guaranteed it is no longer in use */ -void virNetServerClientClose(virNetServerClientPtr client) +void +virNetServerClientCloseLocked(virNetServerClientPtr client) { virNetServerClientCloseFunc cf; virKeepAlivePtr ka; - virObjectLock(client); VIR_DEBUG("client=%p", client); - if (!client->sock) { - virObjectUnlock(client); + if (!client->sock) return; - } if (client->keepalive) { virKeepAliveStop(client->keepalive); @@ -1048,20 +1046,25 @@ void virNetServerClientClose(virNetServerClientPtr client) virObjectUnref(client->sock); client->sock = NULL; } - - virObjectUnlock(client); } -bool virNetServerClientIsClosed(virNetServerClientPtr client) +void +virNetServerClientClose(virNetServerClientPtr client) { - bool closed; virObjectLock(client); - closed = client->sock == NULL ? true : false; + virNetServerClientCloseLocked(client); virObjectUnlock(client); - return closed; } + +bool +virNetServerClientIsClosedLocked(virNetServerClientPtr client) +{ + return client->sock == NULL ? true : false; +} + + void virNetServerClientDelayedClose(virNetServerClientPtr client) { virObjectLock(client); @@ -1076,13 +1079,11 @@ void virNetServerClientImmediateClose(virNetServerClientPtr client) virObjectUnlock(client); } -bool virNetServerClientWantClose(virNetServerClientPtr client) + +bool +virNetServerClientWantCloseLocked(virNetServerClientPtr client) { - bool wantClose; - virObjectLock(client); - wantClose = client->wantClose; - virObjectUnlock(client); - return wantClose; + return client->wantClose; } diff --git a/src/rpc/virnetserverclient.h b/src/rpc/virnetserverclient.h index 1182d53c7059..b7752a61fa8e 100644 --- a/src/rpc/virnetserverclient.h +++ b/src/rpc/virnetserverclient.h @@ -124,11 +124,12 @@ void virNetServerClientSetDispatcher(virNetServerClientPtr client, virNetServerClientDispatchFunc func, void *opaque); void virNetServerClientClose(virNetServerClientPtr client); -bool virNetServerClientIsClosed(virNetServerClientPtr client); +void virNetServerClientCloseLocked(virNetServerClientPtr client); +bool virNetServerClientIsClosedLocked(virNetServerClientPtr client); void virNetServerClientDelayedClose(virNetServerClientPtr client); void virNetServerClientImmediateClose(virNetServerClientPtr client); -bool virNetServerClientWantClose(virNetServerClientPtr client); +bool virNetServerClientWantCloseLocked(virNetServerClientPtr client); int virNetServerClientInit(virNetServerClientPtr client); -- 2.13.4

There is a race between virNetServerProcessClients (main thread) and remoteDispatchAuthList/remoteDispatchAuthPolkit/remoteSASLFinish (worker thread) that can lead to decrementing srv->nclients_unauth when it's zero. Since virNetServerCheckLimits relies on the value srv->nclients_unauth the underrun causes libvirtd to stop accepting new connections forever. Example race scenario (assuming libvirtd is using policykit and the client is privileged): 1. The client calls the RPC remoteDispatchAuthList => remoteDispatchAuthList is executed on a worker thread (Thread T1). We're assuming now the execution stops for some time before the line 'virNetServerClientSetAuth(client, 0)' 2. The client closes the connection irregularly. This causes the event loop to wake up and virNetServerProcessClient to be called (on the main thread T0). During the virNetServerProcessClients the srv lock is hold. The condition virNetServerClientNeedAuth(client) will be checked and as the authentication is not finished right now virNetServerTrackCompletedAuthLocked(srv) will be called => --srv->nclients_unauth => 0 3. The Thread T1 continues, marks the client as authenticated, and calls virNetServerTrackCompletedAuthLocked(srv) => --srv->nclients_unauth => --0 => wrap around as nclient_unauth is unsigned 4. virNetServerCheckLimits(srv) will disable the services forever To fix it, add an auth_pending field to the client struct so that it is now possible to determine if the authentication process has already been handled for this client. Setting the authentication method to none for the client in virNetServerProcessClients is not a proper way to indicate that the counter has been decremented, as this would imply that the client is authenticated. Additionally, adjust the existing test cases for this new field. Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com&gt; Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com&gt; --- src/libvirt_remote.syms | 2 + src/rpc/virnetserver.c | 27 ++++++++++-- src/rpc/virnetserverclient.c | 48 +++++++++++++++++++++- src/rpc/virnetserverclient.h | 2 + .../virnetdaemondata/output-data-admin-nomdns.json | 4 ++ .../output-data-admin-server-names.json | 4 ++ .../virnetdaemondata/output-data-anon-clients.json | 2 + tests/virnetdaemondata/output-data-client-ids.json | 2 + .../output-data-client-timestamp.json | 2 + .../output-data-initial-nomdns.json | 2 + tests/virnetdaemondata/output-data-initial.json | 2 + .../output-data-no-keepalive-required.json | 4 ++ 12 files changed, 95 insertions(+), 6 deletions(-) diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms index 62eac5ae9fdd..5436e3ec18a4 100644 --- a/src/libvirt_remote.syms +++ b/src/libvirt_remote.syms @@ -138,6 +138,7 @@ virNetServerClientGetUNIXIdentity; virNetServerClientImmediateClose; virNetServerClientInit; virNetServerClientInitKeepAlive; +virNetServerClientIsAuthPendingLocked; virNetServerClientIsClosedLocked; virNetServerClientIsLocal; virNetServerClientIsSecure; @@ -152,6 +153,7 @@ virNetServerClientRemoteAddrStringURI; virNetServerClientRemoveFilter; virNetServerClientSendMessage; virNetServerClientSetAuthLocked; +virNetServerClientSetAuthPendingLocked; virNetServerClientSetCloseHook; virNetServerClientSetDispatcher; virNetServerClientSetReadonly; diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c index 946fc29283d8..77a4c0b8dce3 100644 --- a/src/rpc/virnetserver.c +++ b/src/rpc/virnetserver.c @@ -286,7 +286,7 @@ int virNetServerAddClient(virNetServerPtr srv, srv->clients[srv->nclients-1] = virObjectRef(client); virObjectLock(client); - if (virNetServerClientNeedAuthLocked(client)) + if (virNetServerClientIsAuthPendingLocked(client)) virNetServerTrackPendingAuthLocked(srv); virObjectUnlock(client); @@ -738,6 +738,25 @@ int virNetServerSetTLSContext(virNetServerPtr srv, /** + * virNetServerSetClientAuthCompletedLocked: + * @srv: server must be locked by the caller + * @client: client must be locked by the caller + * + * If the client authentication was pending, clear that pending and + * update the server tracking. + */ +static void +virNetServerSetClientAuthCompletedLocked(virNetServerPtr srv, + virNetServerClientPtr client) +{ + if (virNetServerClientIsAuthPendingLocked(client)) { + virNetServerClientSetAuthPendingLocked(client, false); + virNetServerTrackCompletedAuthLocked(srv); + } +} + + +/** * virNetServerSetClientAuthenticated: * @srv: server must be unlocked * @client: client must be unlocked @@ -753,7 +772,7 @@ virNetServerSetClientAuthenticated(virNetServerPtr srv, virObjectLock(srv); virObjectLock(client); virNetServerClientSetAuthLocked(client, VIR_NET_SERVER_SERVICE_AUTH_NONE); - virNetServerTrackCompletedAuthLocked(srv); + virNetServerSetClientAuthCompletedLocked(srv, client); virNetServerCheckLimits(srv); virObjectUnlock(client); virObjectUnlock(srv); @@ -872,8 +891,8 @@ virNetServerProcessClients(virNetServerPtr srv) if (virNetServerClientIsClosedLocked(client)) { VIR_DELETE_ELEMENT(srv->clients, i, srv->nclients); - if (virNetServerClientNeedAuthLocked(client)) - virNetServerTrackCompletedAuthLocked(srv); + /* Update server authentication tracking */ + virNetServerSetClientAuthCompletedLocked(srv, client); virObjectUnlock(client); virNetServerCheckLimits(srv); diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c index 5ebc970e340d..7786e3e2df8e 100644 --- a/src/rpc/virnetserverclient.c +++ b/src/rpc/virnetserverclient.c @@ -71,6 +71,7 @@ struct _virNetServerClient bool delayedClose; virNetSocketPtr sock; int auth; + bool auth_pending; bool readonly; #if WITH_GNUTLS virNetTLSContextPtr tlsCtxt; @@ -375,6 +376,7 @@ static virNetServerClientPtr virNetServerClientNewInternal(unsigned long long id, virNetSocketPtr sock, int auth, + bool auth_pending, #ifdef WITH_GNUTLS virNetTLSContextPtr tls, #endif @@ -393,6 +395,7 @@ virNetServerClientNewInternal(unsigned long long id, client->id = id; client->sock = virObjectRef(sock); client->auth = auth; + client->auth_pending = auth_pending; client->readonly = readonly; #ifdef WITH_GNUTLS client->tlsCtxt = virObjectRef(tls); @@ -440,6 +443,7 @@ virNetServerClientPtr virNetServerClientNew(unsigned long long id, { virNetServerClientPtr client; time_t now; + bool auth_pending = !virNetServerClientAuthMethodImpliesAuthenticated(auth); VIR_DEBUG("sock=%p auth=%d tls=%p", sock, auth, #ifdef WITH_GNUTLS @@ -454,7 +458,7 @@ virNetServerClientPtr virNetServerClientNew(unsigned long long id, return NULL; } - if (!(client = virNetServerClientNewInternal(id, sock, auth, + if (!(client = virNetServerClientNewInternal(id, sock, auth, auth_pending, #ifdef WITH_GNUTLS tls, #endif @@ -486,7 +490,7 @@ virNetServerClientPtr virNetServerClientNewPostExecRestart(virJSONValuePtr objec virNetServerClientPtr client = NULL; virNetSocketPtr sock; int auth; - bool readonly; + bool readonly, auth_pending; unsigned int nrequests_max; unsigned long long id; long long timestamp; @@ -496,6 +500,26 @@ virNetServerClientPtr virNetServerClientNewPostExecRestart(virJSONValuePtr objec _("Missing auth field in JSON state document")); return NULL; } + + if (!virJSONValueObjectHasKey(object, "auth_pending")) { + auth_pending = !virNetServerClientAuthMethodImpliesAuthenticated(auth); + } else { + if (virJSONValueObjectGetBoolean(object, "auth_pending", &auth_pending) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Malformed auth_pending field in JSON state document")); + return NULL; + } + + /* If the used authentication method implies that the new + * client is automatically authenticated, the authentication + * cannot be pending */ + if (auth_pending && virNetServerClientAuthMethodImpliesAuthenticated(auth)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Invalid auth_pending and auth combination in JSON state document")); + return NULL; + } + } + if (virJSONValueObjectGetBoolean(object, "readonly", &readonly) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Missing readonly field in JSON state document")); @@ -544,6 +568,7 @@ virNetServerClientPtr virNetServerClientNewPostExecRestart(virJSONValuePtr objec if (!(client = virNetServerClientNewInternal(id, sock, auth, + auth_pending, #ifdef WITH_GNUTLS NULL, #endif @@ -592,6 +617,8 @@ virJSONValuePtr virNetServerClientPreExecRestart(virNetServerClientPtr client) if (virJSONValueObjectAppendNumberInt(object, "auth", client->auth) < 0) goto error; + if (virJSONValueObjectAppendBoolean(object, "auth_pending", client->auth_pending) < 0) + goto error; if (virJSONValueObjectAppendBoolean(object, "readonly", client->readonly) < 0) goto error; if (virJSONValueObjectAppendNumberUint(object, "nrequests_max", client->nrequests_max) < 0) @@ -1556,6 +1583,23 @@ virNetServerClientNeedAuth(virNetServerClientPtr client) } +/* The caller must hold the lock for @client */ +void +virNetServerClientSetAuthPendingLocked(virNetServerClientPtr client, + bool auth_pending) +{ + client->auth_pending = auth_pending; +} + + +/* The caller must hold the lock for @client */ +bool +virNetServerClientIsAuthPendingLocked(virNetServerClientPtr client) +{ + return client->auth_pending; +} + + static void virNetServerClientKeepAliveDeadCB(void *opaque) { diff --git a/src/rpc/virnetserverclient.h b/src/rpc/virnetserverclient.h index 054bea4f2f10..c174e8285f0c 100644 --- a/src/rpc/virnetserverclient.h +++ b/src/rpc/virnetserverclient.h @@ -149,6 +149,8 @@ int virNetServerClientSendMessage(virNetServerClientPtr client, bool virNetServerClientNeedAuth(virNetServerClientPtr client); bool virNetServerClientNeedAuthLocked(virNetServerClientPtr client); +bool virNetServerClientIsAuthPendingLocked(virNetServerClientPtr client); +void virNetServerClientSetAuthPendingLocked(virNetServerClientPtr client, bool auth_pending); int virNetServerClientGetTransport(virNetServerClientPtr client); int virNetServerClientGetInfo(virNetServerClientPtr client, diff --git a/tests/virnetdaemondata/output-data-admin-nomdns.json b/tests/virnetdaemondata/output-data-admin-nomdns.json index fc960f02cee7..d6d02163e282 100644 --- a/tests/virnetdaemondata/output-data-admin-nomdns.json +++ b/tests/virnetdaemondata/output-data-admin-nomdns.json @@ -41,6 +41,7 @@ { "id": 1, "auth": 1, + "auth_pending": true, "readonly": true, "nrequests_max": 15, "sock": { @@ -53,6 +54,7 @@ { "id": 2, "auth": 2, + "auth_pending": true, "readonly": true, "nrequests_max": 66, "sock": { @@ -105,6 +107,7 @@ { "id": 1, "auth": 1, + "auth_pending": true, "readonly": true, "nrequests_max": 15, "sock": { @@ -117,6 +120,7 @@ { "id": 2, "auth": 2, + "auth_pending": true, "readonly": true, "nrequests_max": 66, "sock": { diff --git a/tests/virnetdaemondata/output-data-admin-server-names.json b/tests/virnetdaemondata/output-data-admin-server-names.json index fc960f02cee7..d6d02163e282 100644 --- a/tests/virnetdaemondata/output-data-admin-server-names.json +++ b/tests/virnetdaemondata/output-data-admin-server-names.json @@ -41,6 +41,7 @@ { "id": 1, "auth": 1, + "auth_pending": true, "readonly": true, "nrequests_max": 15, "sock": { @@ -53,6 +54,7 @@ { "id": 2, "auth": 2, + "auth_pending": true, "readonly": true, "nrequests_max": 66, "sock": { @@ -105,6 +107,7 @@ { "id": 1, "auth": 1, + "auth_pending": true, "readonly": true, "nrequests_max": 15, "sock": { @@ -117,6 +120,7 @@ { "id": 2, "auth": 2, + "auth_pending": true, "readonly": true, "nrequests_max": 66, "sock": { diff --git a/tests/virnetdaemondata/output-data-anon-clients.json b/tests/virnetdaemondata/output-data-anon-clients.json index 9f1c63504a1b..cb6005bfe68c 100644 --- a/tests/virnetdaemondata/output-data-anon-clients.json +++ b/tests/virnetdaemondata/output-data-anon-clients.json @@ -41,6 +41,7 @@ { "id": 1, "auth": 1, + "auth_pending": true, "readonly": true, "nrequests_max": 15, "sock": { @@ -53,6 +54,7 @@ { "id": 2, "auth": 2, + "auth_pending": true, "readonly": true, "nrequests_max": 66, "sock": { diff --git a/tests/virnetdaemondata/output-data-client-ids.json b/tests/virnetdaemondata/output-data-client-ids.json index 43c61cc46ec7..2b1663d4f8c6 100644 --- a/tests/virnetdaemondata/output-data-client-ids.json +++ b/tests/virnetdaemondata/output-data-client-ids.json @@ -41,6 +41,7 @@ { "id": 2, "auth": 1, + "auth_pending": true, "readonly": true, "nrequests_max": 15, "sock": { @@ -53,6 +54,7 @@ { "id": 3, "auth": 2, + "auth_pending": true, "readonly": true, "nrequests_max": 66, "sock": { diff --git a/tests/virnetdaemondata/output-data-client-timestamp.json b/tests/virnetdaemondata/output-data-client-timestamp.json index b706c147ed0e..e8c8e9a991d9 100644 --- a/tests/virnetdaemondata/output-data-client-timestamp.json +++ b/tests/virnetdaemondata/output-data-client-timestamp.json @@ -41,6 +41,7 @@ { "id": 1, "auth": 1, + "auth_pending": true, "readonly": true, "nrequests_max": 15, "conn_time": 1234567890, @@ -54,6 +55,7 @@ { "id": 2, "auth": 2, + "auth_pending": true, "readonly": true, "nrequests_max": 66, "conn_time": 1234567890, diff --git a/tests/virnetdaemondata/output-data-initial-nomdns.json b/tests/virnetdaemondata/output-data-initial-nomdns.json index ca6b99553754..167aef8d295e 100644 --- a/tests/virnetdaemondata/output-data-initial-nomdns.json +++ b/tests/virnetdaemondata/output-data-initial-nomdns.json @@ -41,6 +41,7 @@ { "id": 1, "auth": 1, + "auth_pending": true, "readonly": true, "nrequests_max": 15, "sock": { @@ -53,6 +54,7 @@ { "id": 2, "auth": 2, + "auth_pending": true, "readonly": true, "nrequests_max": 66, "sock": { diff --git a/tests/virnetdaemondata/output-data-initial.json b/tests/virnetdaemondata/output-data-initial.json index a8df6336c316..3281e868d7aa 100644 --- a/tests/virnetdaemondata/output-data-initial.json +++ b/tests/virnetdaemondata/output-data-initial.json @@ -42,6 +42,7 @@ { "id": 1, "auth": 1, + "auth_pending": true, "readonly": true, "nrequests_max": 15, "sock": { @@ -54,6 +55,7 @@ { "id": 2, "auth": 2, + "auth_pending": true, "readonly": true, "nrequests_max": 66, "sock": { diff --git a/tests/virnetdaemondata/output-data-no-keepalive-required.json b/tests/virnetdaemondata/output-data-no-keepalive-required.json index fc960f02cee7..d6d02163e282 100644 --- a/tests/virnetdaemondata/output-data-no-keepalive-required.json +++ b/tests/virnetdaemondata/output-data-no-keepalive-required.json @@ -41,6 +41,7 @@ { "id": 1, "auth": 1, + "auth_pending": true, "readonly": true, "nrequests_max": 15, "sock": { @@ -53,6 +54,7 @@ { "id": 2, "auth": 2, + "auth_pending": true, "readonly": true, "nrequests_max": 66, "sock": { @@ -105,6 +107,7 @@ { "id": 1, "auth": 1, + "auth_pending": true, "readonly": true, "nrequests_max": 15, "sock": { @@ -117,6 +120,7 @@ { "id": 2, "auth": 2, + "auth_pending": true, "readonly": true, "nrequests_max": 66, "sock": { -- 2.13.4

'Squash' virNetServerClientNeedAuthLocked into virNetServerClientNeedAuth and remove virNetServerClientNeedAuthLocked as it's not longer needed. Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com&gt; Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com&gt; Reviewed-by: Stefan Zimmermann <stzi(a)linux.vnet.ibm.com&gt; --- src/libvirt_remote.syms | 1 - src/rpc/virnetserverclient.c | 9 +-------- src/rpc/virnetserverclient.h | 1 - 3 files changed, 1 insertion(+), 10 deletions(-) diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms index 5436e3ec18a4..a950bed14efc 100644 --- a/src/libvirt_remote.syms +++ b/src/libvirt_remote.syms @@ -144,7 +144,6 @@ virNetServerClientIsLocal; virNetServerClientIsSecure; virNetServerClientLocalAddrStringSASL; virNetServerClientNeedAuth; -virNetServerClientNeedAuthLocked; virNetServerClientNew; virNetServerClientNewPostExecRestart; virNetServerClientPreExecRestart; diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c index 7786e3e2df8e..70e881b83a65 100644 --- a/src/rpc/virnetserverclient.c +++ b/src/rpc/virnetserverclient.c @@ -1566,18 +1566,11 @@ int virNetServerClientSendMessage(virNetServerClientPtr client, bool -virNetServerClientNeedAuthLocked(virNetServerClientPtr client) -{ - return !virNetServerClientAuthMethodImpliesAuthenticated(client->auth); -} - - -bool virNetServerClientNeedAuth(virNetServerClientPtr client) { bool need; virObjectLock(client); - need = virNetServerClientNeedAuthLocked(client); + need = !virNetServerClientAuthMethodImpliesAuthenticated(client->auth); virObjectUnlock(client); return need; } diff --git a/src/rpc/virnetserverclient.h b/src/rpc/virnetserverclient.h index c174e8285f0c..81eac6dcec88 100644 --- a/src/rpc/virnetserverclient.h +++ b/src/rpc/virnetserverclient.h @@ -148,7 +148,6 @@ int virNetServerClientSendMessage(virNetServerClientPtr client, virNetMessagePtr msg); bool virNetServerClientNeedAuth(virNetServerClientPtr client); -bool virNetServerClientNeedAuthLocked(virNetServerClientPtr client); bool virNetServerClientIsAuthPendingLocked(virNetServerClientPtr client); void virNetServerClientSetAuthPendingLocked(virNetServerClientPtr client, bool auth_pending); -- 2.13.4

...as there is an access to priv->sasl the priv->lock is needed. Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com&gt; Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com&gt; Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com&gt; --- daemon/remote.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index b6fe6d8539ff..81d570b6e269 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -3389,6 +3389,9 @@ remoteSASLFinish(virNetServerPtr server, const char *identity; struct daemonClientPrivate *priv = virNetServerClientGetPrivateData(client); int ssf; + int rv = 0; + + virMutexLock(&priv->lock); /* TLS or UNIX domain sockets trivially OK */ if (!virNetServerClientIsSecure(client)) { @@ -3398,15 +3401,15 @@ remoteSASLFinish(virNetServerPtr server, VIR_DEBUG("negotiated an SSF of %d", ssf); if (ssf < 56) { /* 56 is good for Kerberos */ VIR_ERROR(_("negotiated SSF %d was not strong enough"), ssf); - return -2; + goto rejected; } } if (!(identity = virNetSASLSessionGetIdentity(priv->sasl))) - return -2; + goto rejected; if (!virNetSASLContextCheckIdentity(saslCtxt, identity)) - return -2; + goto rejected; if (!(clnt_identity = virNetServerClientGetIdentity(client))) goto error; @@ -3425,10 +3428,17 @@ remoteSASLFinish(virNetServerPtr server, virObjectUnref(priv->sasl); priv->sasl = NULL; - return 0; + cleanup: + virMutexUnlock(&priv->lock); + return rv; error: - return -1; + rv = -1; + goto cleanup; + + rejected: + rv = -2; + goto cleanup; } /* -- 2.13.4

On Thu, Dec 21, 2017 at 07:20 PM +0100, John Ferlan <jferlan(a)redhat.com&gt; wrote: Ohhh, you’re right! Sry for that and thanks for checking!! -- Beste Grüße / Kind regards Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294