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@xxxxxxxxxx>
> ---
> 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.
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
