On Fri, Nov 09, 2018 at 19:39:36 -0500, John Ferlan wrote: > In commit ccc72d5cb I thought I had the brilliant idea that I > could alter an error message in virErrorMsg to take two arguments > "drivername" and NULL. Then passing the failed drivername for > VIR_ERR_ACCESS_DENIED users would magically allow any "real" > error message to be included. Of course it worked for the > majority of errors, except for 1 teeny, tiny, problem - there > was a caller using VIR_ERR_ACCESS_DENIED that actually passed > "real" error message - virAccessDriverPolkitGetCaller, but > didn't pass a driver name. So when this code went to call > virErrorMsg it resulted in garbage being printed in that > second argument because that's not how things are written. > > So in order to avoid that issue, reset the error back to the > usual if info == NULL, the print just some text; otherwise, > print a formatted output. For a majority of error messages > this will print the driver name after "access deined from:". > For the one case in question, the correct error message will > be displayed and no garbage chars. > > Signed-off-by: John Ferlan <jferlan redhat com> > --- > src/util/virerror.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/src/util/virerror.c b/src/util/virerror.c > index 10f1b55c5f..df239bf371 100644 > --- a/src/util/virerror.c > +++ b/src/util/virerror.c > @@ -1442,9 +1442,9 @@ virErrorMsg(virErrorNumber error, const char *info) > break; > case VIR_ERR_ACCESS_DENIED: > if (info == NULL) > - errmsg = _("access denied from '%s'"); > + errmsg = _("access denied"); > else > - errmsg = _("access denied from '%s': %s"); > + errmsg = _("access denied from: %s"); > break; The referenced patch modifies virAccessManagerSanitizeError which passes a bogus NULL to virAccessError. Ideally the format string will be changed to just "%s" so that 'driverName' can't be interpreted as a format string. Additionally the referenced patch also modifies src/rpc/gendispatch.pl which also passes a bogus NULL to virReportError, which should be fixed as well. As a side-note, the original commit also attempted to print NULL pointers with %s modifier with *printf which is undefined behavior.
Description: PGP signature