[libvirt] [PATCH 1/2] util: Fix VIR_ERR_ACCESS_DENIED formatting

[Peter Krempa]

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 at 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20181112/e0c33e69/attachment-0001.sig>