[libvirt] [PATCH 1/3] conf: Properly truncate wide character names in virDomainObjGetShortName

[John Ferlan]

On 08/25/2017 02:30 AM, Martin Kletzander wrote:
> On Wed, Aug 23, 2017 at 04:47:03PM -0400, John Ferlan wrote:
>>
>>
>> On 08/23/2017 07:47 AM, Martin Kletzander wrote:
>>> We always truncated the name at 20 bytes instead of characters.  In
>>> case 20 bytes were in the middle of a multi-byte character, then the
>>> string became invalid and various parts of the code would error
>>> out (e.g. XML parsing of that string).  Let's instead properly
>>> truncate it after 20 characters instead.
>>>
>>> In some cases the truncation didn't even work (as can be seen from the
>>> modified tests), which is fixed by this as well.
>>
>> Didn't work as well?  Try at all...
>>
>> As a test I changed the name of pcie-expander-bus to 80 characters and
>> all 80 were printed.  I think this goes against your original intention
>> of a shortname...
>>
>>>
>>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1448766
>>>
>>> Signed-off-by: Martin Kletzander <mkletzan at redhat.com>
>>> ---
>>>  src/conf/domain_conf.c                             | 30
>>> +++++++++++++++++++---
>>>  .../qemuxml2argv-aarch64-virt-default-nic.args     |  2 +-
>>>  .../qemuxml2argv-hugepages-pages2.args             |  4 +--
>>>  .../qemuxml2argv-hugepages-pages3.args             |  5 ++--
>>>  .../qemuxml2argv-hugepages-pages5.args             |  4 +--
>>>  .../qemuxml2argv-hugepages-pages6.args             |  2 +-
>>>  .../qemuxml2argv-pcie-expander-bus.args            |  2 +-
>>>  7 files changed, 37 insertions(+), 12 deletions(-)
>>>
>>
>> Can we add a multibyte name test?  These just modify existing tests (I
>> think wrongly too).
>>
> 
> Good point, I don't know why I didn't add one.  I created one especially
> for testing this, but didn't add it to the repo it seems.  However I'm
> not sure we will be able to force the characters to be multibyte, we
> would have to modify the locale of the test, but also skip the test if
> the NLS for the filesystem is something like ISO 8859-1 or so.  I can
> add one anyway, at least on some systems it might check for something.
> 
>>
>>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>>> index 47eba4dbb315..9a46ceece2d6 100644
>>> --- a/src/conf/domain_conf.c
>>> +++ b/src/conf/domain_conf.c
>>> @@ -27131,6 +27131,8 @@ virDomainDefHasMemballoon(const virDomainDef
>>> *def)
>>>  }
>>>
>>>
>>> +#define VIR_DOMAIN_SHORT_NAME_MAX 20
>>> +
>>>  /**
>>>   * virDomainObjGetShortName:
>>>   * @vm: Machine for which to get a name
>>> @@ -27141,15 +27143,37 @@ virDomainDefHasMemballoon(const
>>> virDomainDef *def)
>>>  char *
>>>  virDomainObjGetShortName(const virDomainDef *def)
>>>  {
>>> -    const int dommaxlen = 20;
>>> +    wchar_t wshortname[VIR_DOMAIN_SHORT_NAME_MAX + 1] = {0};
>>> +    size_t len = 0;
>>> +    char *shortname = NULL;
>>>      char *ret = NULL;
>>>
>>
>> It would seem that we could figure there were multibyte chars in the
>> name "if strlen(def->name) != mbstowcs(NULL, def->name, 0)" and not do
>> any conversion, leaving the old code "as is" (mostly) and then
>> implementing something for these wide characters.
>>
> 
> Good point, we can do that.
> 
>> Of course I'm not getting anything other than -1 returned for the
>> mbstowcs and I didn't really want to dig any further, so I leave it up
>> to you!
>>
> 
> What the errno in that case?
> 

"Invalid or incomplete multibyte or wide character"

Same message even after adding a virGettextInitialize call in the
function as a test...  It was one of those annoying end of the day
things too - as in this *should* work, why is it not working.

>> I tried to modify the same test and change the name to "ÄèÎØÛ", but the
>> mbstowcs kept returning -1, until I added:
>>
>>    if (setlocale(LC_ALL, "en_US.UTF-8") == NULL) {
>>
> 
> What is the output of `locale` on your machine?
> 

en_US.UTF-8


>> But how does one know which locale to use? From my quick read of the
>> mbstowcs man page it seems a locale needs to be set first. I also tried
>> the man page example of "Grüße!" and that worked with UTF-8.
>>
> 
> It should use the locale of the system.  Maybe if you have e.g. 8859-1
> set, there will be no multibyte characters, so it does not do anything.
> If we can gather that info from the errno, then we can safely skip the
> conversion as well.
> 
>>> -    ignore_value(virAsprintf(&ret, "%d-%.*s",
>>> -                             def->id, dommaxlen, def->name));
>>> +    if (mbstowcs(wshortname, def->name, VIR_DOMAIN_SHORT_NAME_MAX)
>>> == (size_t) -1) {
>>> +        virReportError(VIR_ERR_INVALID_ARG, "%s",
>>> +                       _("Cannot convert domain name to wide
>>> character string"));
>>> +        return NULL;
>>> +    }
>>> +
>>> +    len = wcstombs(NULL, wshortname, 0);
>>> +    if (len == (size_t) -1)
>>> +        return NULL;
>>>
>>> +    if (VIR_ALLOC_N(shortname, len + 1) < 0)
>>> +        return NULL;
>>> +
>>> +    if (wcstombs(shortname, wshortname, len) == (size_t) -1) {
>>> +        virReportError(VIR_ERR_INVALID_ARG, "%s",
>>> +                       _("Cannot convert domain name from wide
>>> character string"));
>>> +        goto cleanup;
>>> +    }
>>> +
>>> +    ignore_value(virAsprintf(&ret, "%d-%s", def->id, def->name));
>>
>> You go through all this trouble and still write def->name! I didn't
>> realize that at first either - kept wondering why the whole damn name
>> was being printed!
>>
> 
> What the fsck have I sent?!?!?
> 

Yeah well I only saw it after spending more than a few minutes messing
around with the code.

> I see what I did wrong, interesting.  So I ran the tests with
> VIR_TEST_DEBUG=1 and I've seen some truncation not happening and I (by
> mistake) though that new truncation *is* happening now, so I re-ran with
> VIR_TEST_REGENERATE_OUTPUT=1.  And it fixed the issue because there was
> no more truncation happening.

Ahhh... If I run the test directly:

VIR_TEST_DEBUG=1 ./run tests/qemuxml2argvtest

then things work as (mostly) expected as long as locale is set via a
virGettextInitialize in virDomainObjGetShortName; however, if I run via:

VIR_TEST_DEBUG=1 make -C tests check TESTS=qemuxml2argvtest

then I get the mbtowcs failure util I explicitly set the locale to
en_US.UTF-8 in virDomainObjGetShortName.

It's very odd/strange...

Since I only was messing with pcie-expander-bus-test I can also use
VIR_TEST_RANGE=565 in order to see just that output...

> 
> Anyway, thanks for seeing that, that's what the reviews are for ;) v2 on
> the way.

I see v2 hit the list - I'll look at that...

John