On Thu, Mar 05, 2020 at 06:25:20PM +0000, Richard W.M. Jones wrote:
> On Thu, Mar 05, 2020 at 04:54:10PM +0000, Daniel P. Berrangé wrote:
> > 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);
> > }
> > }
>
> It's clearly better to use getline here instead of fgets and
> the large fixed-size stack buffer, so
Damn, doesn't exist on Mingw, so I'll have to rework to keep
fgets and fix it differently.
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 :|
