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@xxxxxxxxxx> > > --- > > src/rpc/virnetsshsession.c | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > The fix itself is correct, so: > > Reviewed-by: Andrea Bolognani <abologna@xxxxxxxxxx> > > 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 :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
