Re: [libvirt PATCH] src: fix mixup of stack and heap allocated data in auth callback

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(a)redhat.com&gt; > 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(a)redhat.com&gt; > --- > 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