In the following recent change:
commit db72866310d1e520efa8ed2d4589bdb5e76a1c95
Author: Daniel P. Berrangé <berrange@xxxxxxxxxx>
Date: Tue Jan 14 10:40:52 2020 +0000
util: add API for reading password from the console
the fact that "bufptr" pointer may point to either heap or stack
allocated data was overlooked. As a result, when the strdup was
removed, we ended up returning a pointer to the local stack to
the caller. When the caller referenced this stack pointer they
got out garbage which fairly quickly resulted in a crash.
Switching from fgets() to getline() lets to avoid the need for
the stack allocated buffer entirely, which makes both cases
of the switch consistent.
Test case addition is inspired by the libguestfs test which
caught this bug.
Signed-off-by: Daniel P. Berrangé <berrange@xxxxxxxxxx>
---
src/libvirt.c | 30 +++++++++++------------
tests/Makefile.am | 2 ++
tests/virsh-auth | 57 ++++++++++++++++++++++++++++++++++++++++++++
tests/virsh-auth.xml | 5 ++++
4 files changed, 79 insertions(+), 15 deletions(-)
create mode 100755 tests/virsh-auth
create mode 100644 tests/virsh-auth.xml
diff --git a/src/libvirt.c b/src/libvirt.c
index a30eaa7590..cc9c2eb5a2 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -110,9 +110,8 @@ virConnectAuthCallbackDefault(virConnectCredentialPtr cred,
size_t i;
for (i = 0; i < ncred; i++) {
- char buf[1024];
- char *bufptr = buf;
- size_t len;
+ char *buf = NULL;
+ size_t len = 0;
switch (cred[i].type) {
case VIR_CRED_EXTERNAL: {
@@ -136,16 +135,17 @@ virConnectAuthCallbackDefault(virConnectCredentialPtr cred,
if (fflush(stdout) != 0)
return -1;
- if (!fgets(buf, sizeof(buf), stdin)) {
- if (feof(stdin)) { /* Treat EOF as "" */
- buf[0] = '\0';
- break;
+ if (getline(&buf, &len, stdin) < 0) {
+ if (!feof(stdin)) {
+ return -1;
}
- return -1;
+ /* Use creddefault on EOF */
+ buf = NULL;
+ } else {
+ len = strlen(buf);
+ if (len != 0 && buf[len-1] == '\n')
+ buf[len-1] = '\0';
}
- len = strlen(buf);
- if (len != 0 && buf[len-1] == '\n')
- buf[len-1] = '\0';
break;
case VIR_CRED_PASSPHRASE:
@@ -155,9 +155,9 @@ virConnectAuthCallbackDefault(virConnectCredentialPtr cred,
if (fflush(stdout) != 0)
return -1;
- bufptr = virGetPassword();
- if (STREQ(bufptr, ""))
- VIR_FREE(bufptr);
+ buf = virGetPassword();
+ if (STREQ(buf, ""))
+ VIR_FREE(buf);
break;
default:
@@ -165,7 +165,7 @@ virConnectAuthCallbackDefault(virConnectCredentialPtr cred,
}
if (cred[i].type != VIR_CRED_EXTERNAL) {
- cred[i].result = bufptr ? bufptr : g_strdup(cred[i].defresult ? cred[i].defresult : "");
+ cred[i].result = buf ? buf : g_strdup(cred[i].defresult ? cred[i].defresult : "");
cred[i].resultlen = strlen(cred[i].result);
}
}
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 83326dbaa9..3b5abcc12b 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -164,6 +164,7 @@ EXTRA_DIST = \
xlconfigdata \
xmconfigdata \
xml2vmxdata \
+ virsh-auth.xml \
virstorageutildata \
virfilecachedata \
virresctrldata \
@@ -406,6 +407,7 @@ test_scripts =
libvirtd_test_scripts = \
libvirtd-fail \
libvirtd-pool \
+ virsh-auth \
virsh-cpuset \
virsh-define-dev-segfault \
virsh-int-overflow \
diff --git a/tests/virsh-auth b/tests/virsh-auth
new file mode 100755
index 0000000000..d548694190
--- /dev/null
+++ b/tests/virsh-auth
@@ -0,0 +1,57 @@
+#!/usr/bin/env python3
+# run virsh to validate interactive auth
+
+# Copyright (C) 2020 Red Hat, Inc.
+
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 2 of the License, or
+# (at your option) any later version.
+
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see
+# <http://www.gnu.org/licenses/>.
+
+import os
+import os.path
+import sys
+import subprocess
+
+builddir = os.getenv("abs_top_builddir")
+if builddir is None:
+ builddir = os.path.join(os.getcwd(), "..")
+
+srcdir = os.getenv("abs_top_srcdir")
+if srcdir is None:
+ srcdir = os.path.abspath(os.path.join(os.path.dirname(__file__), ".."))
+
+uri = "test://" + os.path.join(srcdir, "tests", "virsh-auth.xml")
+
+virsh = os.path.join(builddir, "tools", "virsh")
+
+proc = subprocess.Popen([virsh, "-c", uri, "uri"],
+ universal_newlines=True,
+ start_new_session=True,
+ stdin=subprocess.PIPE,
+ stdout=subprocess.PIPE,
+ stderr=subprocess.PIPE)
+out, err = proc.communicate("astrochicken")
+
+if proc.returncode != 0:
+ print("virsh failed with code %d" % proc.returncode, file=sys.stderr)
+ if out != "":
+ print("stdout=%s" % out)
+ if err != "":
+ print("stderr=%s" % err)
+ sys.exit(1)
+
+if uri not in out:
+ print("Expected '%s' in '%s'" % (uri, out), file=sys.stderr)
+ sys.exit(1)
+
+sys.exit(0)
diff --git a/tests/virsh-auth.xml b/tests/virsh-auth.xml
new file mode 100644
index 0000000000..a045a0746e
--- /dev/null
+++ b/tests/virsh-auth.xml
@@ -0,0 +1,5 @@
+<node>
+ <auth>
+ <user>astrochicken</user>
+ </auth>
+</node>
--
2.24.1
