[PATCH 0/5] Credential asking cleanup follow-up
Few things noticed during review and during fixing of the original patches after review. Peter Krempa (5): util: authconfig: Don't check return value of 'g_key_file_new()' virNetSSHSessionAuthAddPasswordAuth: Don't access unlocked 'sess' virnetsshsession: Don't check return value of 'virNetSSHSessionAuthMethodNew' virNetLibsshSessionAuthAddPasswordAuth: Don't access unlocked 'sess' virnetlibsshsession: Don't check return value of 'virNetLibsshSessionAuthMethodNew' src/rpc/virnetlibsshsession.c | 50 ++++++++--------------------------- src/rpc/virnetsshsession.c | 41 +++++++--------------------- src/util/virauthconfig.c | 8 ++---- 3 files changed, 23 insertions(+), 76 deletions(-) -- 2.38.1
The function can't fail so it's pointless to check its return value. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virauthconfig.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/util/virauthconfig.c b/src/util/virauthconfig.c index 0363a1bef9..818df7a252 100644 --- a/src/util/virauthconfig.c +++ b/src/util/virauthconfig.c @@ -41,9 +41,7 @@ virAuthConfigNew(const char *path) g_autoptr(virAuthConfig) auth = g_new0(virAuthConfig, 1); auth->path = g_strdup(path); - - if (!(auth->keyfile = g_key_file_new())) - return NULL; + auth->keyfile = g_key_file_new(); if (!g_key_file_load_from_file(auth->keyfile, path, 0, NULL)) return NULL; @@ -60,9 +58,7 @@ virAuthConfigNewData(const char *path, g_autoptr(virAuthConfig) auth = g_new0(virAuthConfig, 1); auth->path = g_strdup(path); - - if (!(auth->keyfile = g_key_file_new())) - return NULL; + auth->keyfile = g_key_file_new(); if (!g_key_file_load_from_data(auth->keyfile, data, len, 0, NULL)) return NULL; -- 2.38.1
'sess->authPath' is modified before locking the 'sess' object. Additionally on failure of 'virAuthGetConfigFilePathURI' 'sess' would be unlocked even when it was not yet locked. Fixes: 273745b43122a77adf8c73b2e0a852ac42387349 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/rpc/virnetsshsession.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/rpc/virnetsshsession.c b/src/rpc/virnetsshsession.c index 8584a961d6..73e65d9371 100644 --- a/src/rpc/virnetsshsession.c +++ b/src/rpc/virnetsshsession.c @@ -970,15 +970,17 @@ virNetSSHSessionAuthAddPasswordAuth(virNetSSHSession *sess, { virNetSSHAuthMethod *auth; + virObjectLock(sess); + if (uri) { VIR_FREE(sess->authPath); - if (virAuthGetConfigFilePathURI(uri, &sess->authPath) < 0) - goto error; + if (virAuthGetConfigFilePathURI(uri, &sess->authPath) < 0) { + virObjectUnlock(sess); + return -1; + } } - virObjectLock(sess); - if (!(auth = virNetSSHSessionAuthMethodNew(sess))) goto error; -- 2.38.1
The function can't return NULL to the callers so it doesn't make sense to check it. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/rpc/virnetsshsession.c | 31 ++++--------------------------- 1 file changed, 4 insertions(+), 27 deletions(-) diff --git a/src/rpc/virnetsshsession.c b/src/rpc/virnetsshsession.c index 73e65d9371..a3dd60f5a3 100644 --- a/src/rpc/virnetsshsession.c +++ b/src/rpc/virnetsshsession.c @@ -981,17 +981,11 @@ virNetSSHSessionAuthAddPasswordAuth(virNetSSHSession *sess, } } - if (!(auth = virNetSSHSessionAuthMethodNew(sess))) - goto error; - + auth = virNetSSHSessionAuthMethodNew(sess); auth->method = VIR_NET_SSH_AUTH_PASSWORD; virObjectUnlock(sess); return 0; - - error: - virObjectUnlock(sess); - return -1; } int @@ -1001,17 +995,11 @@ virNetSSHSessionAuthAddAgentAuth(virNetSSHSession *sess) virObjectLock(sess); - if (!(auth = virNetSSHSessionAuthMethodNew(sess))) - goto error; - + auth = virNetSSHSessionAuthMethodNew(sess); auth->method = VIR_NET_SSH_AUTH_AGENT; virObjectUnlock(sess); return 0; - - error: - virObjectUnlock(sess); - return -1; } int @@ -1028,11 +1016,7 @@ virNetSSHSessionAuthAddPrivKeyAuth(virNetSSHSession *sess, virObjectLock(sess); - if (!(auth = virNetSSHSessionAuthMethodNew(sess))) { - virObjectUnlock(sess); - return -1; - } - + auth = virNetSSHSessionAuthMethodNew(sess); auth->filename = g_strdup(keyfile); auth->method = VIR_NET_SSH_AUTH_PRIVKEY; @@ -1048,19 +1032,12 @@ virNetSSHSessionAuthAddKeyboardAuth(virNetSSHSession *sess, virObjectLock(sess); - if (!(auth = virNetSSHSessionAuthMethodNew(sess))) - goto error; - + auth = virNetSSHSessionAuthMethodNew(sess); auth->tries = tries; auth->method = VIR_NET_SSH_AUTH_KEYBOARD_INTERACTIVE; virObjectUnlock(sess); return 0; - - error: - virObjectUnlock(sess); - return -1; - } void -- 2.38.1
'sess->authPath' is modified before locking the 'sess' object. Additionally on failure of 'virAuthGetConfigFilePathURI' 'sess' would be unlocked even when it was not yet locked. Fixes: 6917467c2b0e8f655999f3e568708c4651811689 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/rpc/virnetlibsshsession.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/rpc/virnetlibsshsession.c b/src/rpc/virnetlibsshsession.c index 8de7c86a41..d2e9e20ff2 100644 --- a/src/rpc/virnetlibsshsession.c +++ b/src/rpc/virnetlibsshsession.c @@ -846,16 +846,17 @@ virNetLibsshSessionAuthAddPasswordAuth(virNetLibsshSession *sess, int ret; virNetLibsshAuthMethod *auth; + virObjectLock(sess); + if (uri) { VIR_FREE(sess->authPath); if (virAuthGetConfigFilePathURI(uri, &sess->authPath) < 0) { - ret = -1; - goto cleanup; + virObjectUnlock(sess); + return -1; } } - virObjectLock(sess); if (!(auth = virNetLibsshSessionAuthMethodNew(sess))) { ret = -1; -- 2.38.1
The function can't return NULL to the callers so it doesn't make sense to check it. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/rpc/virnetlibsshsession.c | 43 ++++++----------------------------- 1 file changed, 7 insertions(+), 36 deletions(-) diff --git a/src/rpc/virnetlibsshsession.c b/src/rpc/virnetlibsshsession.c index d2e9e20ff2..bba37210df 100644 --- a/src/rpc/virnetlibsshsession.c +++ b/src/rpc/virnetlibsshsession.c @@ -843,7 +843,6 @@ int virNetLibsshSessionAuthAddPasswordAuth(virNetLibsshSession *sess, virURI *uri) { - int ret; virNetLibsshAuthMethod *auth; virObjectLock(sess); @@ -857,43 +856,27 @@ virNetLibsshSessionAuthAddPasswordAuth(virNetLibsshSession *sess, } } - - if (!(auth = virNetLibsshSessionAuthMethodNew(sess))) { - ret = -1; - goto cleanup; - } - + auth = virNetLibsshSessionAuthMethodNew(sess); auth->method = VIR_NET_LIBSSH_AUTH_PASSWORD; auth->ssh_flags = SSH_AUTH_METHOD_PASSWORD; - ret = 0; - - cleanup: virObjectUnlock(sess); - return ret; + return 0; } int virNetLibsshSessionAuthAddAgentAuth(virNetLibsshSession *sess) { - int ret; virNetLibsshAuthMethod *auth; virObjectLock(sess); - if (!(auth = virNetLibsshSessionAuthMethodNew(sess))) { - ret = -1; - goto cleanup; - } - + auth = virNetLibsshSessionAuthMethodNew(sess); auth->method = VIR_NET_LIBSSH_AUTH_AGENT; auth->ssh_flags = SSH_AUTH_METHOD_PUBLICKEY; - ret = 0; - - cleanup: virObjectUnlock(sess); - return ret; + return 0; } int @@ -910,11 +893,7 @@ virNetLibsshSessionAuthAddPrivKeyAuth(virNetLibsshSession *sess, virObjectLock(sess); - if (!(auth = virNetLibsshSessionAuthMethodNew(sess))) { - virObjectUnlock(sess); - return -1; - } - + auth = virNetLibsshSessionAuthMethodNew(sess); auth->filename = g_strdup(keyfile); auth->method = VIR_NET_LIBSSH_AUTH_PRIVKEY; auth->ssh_flags = SSH_AUTH_METHOD_PUBLICKEY; @@ -927,26 +906,18 @@ int virNetLibsshSessionAuthAddKeyboardAuth(virNetLibsshSession *sess, int tries) { - int ret; virNetLibsshAuthMethod *auth; virObjectLock(sess); - if (!(auth = virNetLibsshSessionAuthMethodNew(sess))) { - ret = -1; - goto cleanup; - } + auth = virNetLibsshSessionAuthMethodNew(sess); auth->tries = tries; auth->method = VIR_NET_LIBSSH_AUTH_KEYBOARD_INTERACTIVE; auth->ssh_flags = SSH_AUTH_METHOD_INTERACTIVE; - ret = 0; - - cleanup: virObjectUnlock(sess); - return ret; - + return 0; } void -- 2.38.1
On a Monday in 2023, Peter Krempa wrote:
Few things noticed during review and during fixing of the original patches after review.
Peter Krempa (5): util: authconfig: Don't check return value of 'g_key_file_new()' virNetSSHSessionAuthAddPasswordAuth: Don't access unlocked 'sess' virnetsshsession: Don't check return value of 'virNetSSHSessionAuthMethodNew' virNetLibsshSessionAuthAddPasswordAuth: Don't access unlocked 'sess' virnetlibsshsession: Don't check return value of 'virNetLibsshSessionAuthMethodNew'
src/rpc/virnetlibsshsession.c | 50 ++++++++--------------------------- src/rpc/virnetsshsession.c | 41 +++++++--------------------- src/util/virauthconfig.c | 8 ++---- 3 files changed, 23 insertions(+), 76 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Peter Krempa