On Fri, Aug 31, 2018 at 04:16:45PM +0200, Andrea Bolognani wrote:
On Fri, 2018-08-31 at 11:15 +0100, Daniel P. Berrangé wrote:
The result of libssh2_userauth_password is being assigned to 'ret' in one branch and 'rc' in the other branch. Checks are all done against the 'ret' variable, so one branch never does the correct check.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/rpc/virnetsshsession.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
The fix itself is correct, so:
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
However, see below.
/* determine exist status */ - if (ret == LIBSSH2_ERROR_AUTHENTICATION_FAILED) + if (rc == LIBSSH2_ERROR_AUTHENTICATION_FAILED) return 1; else return -1;
Since we have a cleanup label right after this, it would make sense to change both 'return' to 'ret = ': that would ensure the function has a single return and also prevent 'password' from leaking when an error which is not AUTHENTICATION_FAILED is returned by libssh2_userauth_password().
Yes, makes sense. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|