[Qemu-devel] [PATCH] qga/commands-posix.c: Use correct types with g_base64_decode()

[Qemu-devel] [PATCH] qga/commands-posix.c: Use correct types with g_base64_decode()

[Peter Maydell]

The second argument of g_base64_decode() is a 'gsize *', not a
'size_t *'. Some compilation environments (like building 32-bit PPC
binaries on a PPC64 system) will complain about the mismatch:

  CC    qga/commands-posix.o
qga/commands-posix.c: In function 'qmp_guest_set_user_password':
qga/commands-posix.c:1908:5: error: passing argument 2 of 'g_base64_decode' from incompatible pointer type [-Werror]
In file included from /usr/include/glib-2.0/glib.h:37:0,
                 from qga/commands-posix.c:14:
/usr/include/glib-2.0/glib/gbase64.h:49:9: note: expected ‘gsize *’ but argument is of type ‘size_t *’

(We previously fixed errors of this type in commit 3d1bba20.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 qga/commands-posix.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index ba8de62..9fde348 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1901,7 +1901,7 @@ void qmp_guest_set_user_password(const char *username,
     int status;
     int datafd[2] = { -1, -1 };
     char *rawpasswddata = NULL;
-    size_t rawpasswdlen;
+    gsize rawpasswdlen;
     char *chpasswddata = NULL;
     size_t chpasswdlen;
 
-- 
1.9.1

Re: [Qemu-devel] [PATCH] qga/commands-posix.c: Use correct types with g_base64_decode()

[Michael Roth]

Quoting Peter Maydell (2015-04-08 15:02:49)
> The second argument of g_base64_decode() is a 'gsize *', not a
> 'size_t *'. Some compilation environments (like building 32-bit PPC
> binaries on a PPC64 system) will complain about the mismatch:
> 
>   CC    qga/commands-posix.o
> qga/commands-posix.c: In function 'qmp_guest_set_user_password':
> qga/commands-posix.c:1908:5: error: passing argument 2 of 'g_base64_decode' from incompatible pointer type [-Werror]
> In file included from /usr/include/glib-2.0/glib.h:37:0,
>                  from qga/commands-posix.c:14:
> /usr/include/glib-2.0/glib/gbase64.h:49:9: note: expected ‘gsize *’ but argument is of type ‘size_t *’
> 
> (We previously fixed errors of this type in commit 3d1bba20.)
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>

Thanks for the catch/patch! Assuming you're applying directly, but otherwise
let me know and I'll send a pull within the hour.

> ---
>  qga/commands-posix.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index ba8de62..9fde348 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -1901,7 +1901,7 @@ void qmp_guest_set_user_password(const char *username,
>      int status;
>      int datafd[2] = { -1, -1 };
>      char *rawpasswddata = NULL;
> -    size_t rawpasswdlen;
> +    gsize rawpasswdlen;
>      char *chpasswddata = NULL;
>      size_t chpasswdlen;
> 
> -- 
> 1.9.1

Re: [Qemu-devel] [PATCH] qga/commands-posix.c: Use correct types with g_base64_decode()

[Thomas Huth]

Am Wed,  8 Apr 2015 21:02:49 +0100
schrieb Peter Maydell <peter.maydell@linaro.org>:

> The second argument of g_base64_decode() is a 'gsize *', not a
> 'size_t *'. Some compilation environments (like building 32-bit PPC
> binaries on a PPC64 system) will complain about the mismatch:
> 
>   CC    qga/commands-posix.o
> qga/commands-posix.c: In function 'qmp_guest_set_user_password':
> qga/commands-posix.c:1908:5: error: passing argument 2 of 'g_base64_decode' from incompatible pointer type [-Werror]
> In file included from /usr/include/glib-2.0/glib.h:37:0,
>                  from qga/commands-posix.c:14:
> /usr/include/glib-2.0/glib/gbase64.h:49:9: note: expected ‘gsize *’ but argument is of type ‘size_t *’
> 
> (We previously fixed errors of this type in commit 3d1bba20.)
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  qga/commands-posix.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index ba8de62..9fde348 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -1901,7 +1901,7 @@ void qmp_guest_set_user_password(const char *username,
>      int status;
>      int datafd[2] = { -1, -1 };
>      char *rawpasswddata = NULL;
> -    size_t rawpasswdlen;
> +    gsize rawpasswdlen;
>      char *chpasswddata = NULL;
>      size_t chpasswdlen;
>  

I've just ran into this issue as well on my x86 laptop when playing
around with "--extra-cflags=-m32"... so should this be fixed for the
2.3 release already? I didn't spot your patch on the master branch
yet...

Anyway, your patch looks like the right solution:

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH] qga/commands-posix.c: Use correct types with g_base64_decode()

[Peter Maydell]

On 14 April 2015 at 14:38, Thomas Huth <thuth@redhat.com> wrote:
> Am Wed,  8 Apr 2015 21:02:49 +0100
> schrieb Peter Maydell <peter.maydell@linaro.org>:
>
>> The second argument of g_base64_decode() is a 'gsize *', not a
>> 'size_t *'. Some compilation environments (like building 32-bit PPC
>> binaries on a PPC64 system) will complain about the mismatch:
>>
>>   CC    qga/commands-posix.o
>> qga/commands-posix.c: In function 'qmp_guest_set_user_password':
>> qga/commands-posix.c:1908:5: error: passing argument 2 of 'g_base64_decode' from incompatible pointer type [-Werror]
>> In file included from /usr/include/glib-2.0/glib.h:37:0,
>>                  from qga/commands-posix.c:14:
>> /usr/include/glib-2.0/glib/gbase64.h:49:9: note: expected ‘gsize *’ but argument is of type ‘size_t *’
>>
>> (We previously fixed errors of this type in commit 3d1bba20.)
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>  qga/commands-posix.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>> index ba8de62..9fde348 100644
>> --- a/qga/commands-posix.c
>> +++ b/qga/commands-posix.c
>> @@ -1901,7 +1901,7 @@ void qmp_guest_set_user_password(const char *username,
>>      int status;
>>      int datafd[2] = { -1, -1 };
>>      char *rawpasswddata = NULL;
>> -    size_t rawpasswdlen;
>> +    gsize rawpasswdlen;
>>      char *chpasswddata = NULL;
>>      size_t chpasswdlen;
>>
>
> I've just ran into this issue as well on my x86 laptop when playing
> around with "--extra-cflags=-m32"... so should this be fixed for the
> 2.3 release already? I didn't spot your patch on the master branch
> yet...

No, this is not-for-2.3. I think that the issue here is that
merely adding -m32 isn't sufficient, because pkg-config when
run by configure will end up pointing at a bunch of incorrect
glib includes (for x86-64, not i386). So the actual solution
is to give configure the right pkg-config, and then gsize and
size_t are the same. There's a patch by John Snow for
configure on-list for that.

-- PMM

Re: [Qemu-devel] [PATCH] qga/commands-posix.c: Use correct types with g_base64_decode()

[Paolo Bonzini]

On 08/04/2015 22:02, Peter Maydell wrote:
> The second argument of g_base64_decode() is a 'gsize *', not a
> 'size_t *'. Some compilation environments (like building 32-bit PPC
> binaries on a PPC64 system) will complain about the mismatch:
> 
>   CC    qga/commands-posix.o
> qga/commands-posix.c: In function 'qmp_guest_set_user_password':
> qga/commands-posix.c:1908:5: error: passing argument 2 of 'g_base64_decode' from incompatible pointer type [-Werror]
> In file included from /usr/include/glib-2.0/glib.h:37:0,
>                  from qga/commands-posix.c:14:
> /usr/include/glib-2.0/glib/gbase64.h:49:9: note: expected ‘gsize *’ but argument is of type ‘size_t *’
> 
> (We previously fixed errors of this type in commit 3d1bba20.)
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

I think this patch is wrong.  Considering what Thomas was doing
("playing around with --extra-cflags=-m32" on x86) this looks like
you're using the 64-bit glib headers while doing a 32-bit compilation.

Paolo
> ---
>  qga/commands-posix.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index ba8de62..9fde348 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -1901,7 +1901,7 @@ void qmp_guest_set_user_password(const char *username,
>      int status;
>      int datafd[2] = { -1, -1 };
>      char *rawpasswddata = NULL;
> -    size_t rawpasswdlen;
> +    gsize rawpasswdlen;
>      char *chpasswddata = NULL;
>      size_t chpasswdlen;
>  
> 

Re: [Qemu-devel] [PATCH] qga/commands-posix.c: Use correct types with g_base64_decode()

[Peter Maydell]

On 14 April 2015 at 14:42, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 08/04/2015 22:02, Peter Maydell wrote:
>> The second argument of g_base64_decode() is a 'gsize *', not a
>> 'size_t *'. Some compilation environments (like building 32-bit PPC
>> binaries on a PPC64 system) will complain about the mismatch:
>>
>>   CC    qga/commands-posix.o
>> qga/commands-posix.c: In function 'qmp_guest_set_user_password':
>> qga/commands-posix.c:1908:5: error: passing argument 2 of 'g_base64_decode' from incompatible pointer type [-Werror]
>> In file included from /usr/include/glib-2.0/glib.h:37:0,
>>                  from qga/commands-posix.c:14:
>> /usr/include/glib-2.0/glib/gbase64.h:49:9: note: expected ‘gsize *’ but argument is of type ‘size_t *’
>>
>> (We previously fixed errors of this type in commit 3d1bba20.)
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>
> I think this patch is wrong.  Considering what Thomas was doing
> ("playing around with --extra-cflags=-m32" on x86) this looks like
> you're using the 64-bit glib headers while doing a 32-bit compilation.

I sort of agree, but I don't think the patch is wrong as such.
The API of the function we're calling demands a pointer to a
'gsize', so we should do that, not rely implicitly on 'gsize'
and 'size_t' being interchangeable.

(I have no idea why glib sees fit to define its own versions
of the standard types, but given that it does we should get
the interfacing to them right...)

-- PMM

Re: [Qemu-devel] [PATCH] qga/commands-posix.c: Use correct types with g_base64_decode()

[Paolo Bonzini]

On 14/04/2015 15:45, Peter Maydell wrote:
> On 14 April 2015 at 14:42, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>>
>> On 08/04/2015 22:02, Peter Maydell wrote:
>>> The second argument of g_base64_decode() is a 'gsize *', not a
>>> 'size_t *'. Some compilation environments (like building 32-bit PPC
>>> binaries on a PPC64 system) will complain about the mismatch:
>>>
>>>   CC    qga/commands-posix.o
>>> qga/commands-posix.c: In function 'qmp_guest_set_user_password':
>>> qga/commands-posix.c:1908:5: error: passing argument 2 of 'g_base64_decode' from incompatible pointer type [-Werror]
>>> In file included from /usr/include/glib-2.0/glib.h:37:0,
>>>                  from qga/commands-posix.c:14:
>>> /usr/include/glib-2.0/glib/gbase64.h:49:9: note: expected ‘gsize *’ but argument is of type ‘size_t *’
>>>
>>> (We previously fixed errors of this type in commit 3d1bba20.)
>>>
>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>
>> I think this patch is wrong.  Considering what Thomas was doing
>> ("playing around with --extra-cflags=-m32" on x86) this looks like
>> you're using the 64-bit glib headers while doing a 32-bit compilation.
> 
> I sort of agree, but I don't think the patch is wrong as such.
> The API of the function we're calling demands a pointer to a
> 'gsize', so we should do that, not rely implicitly on 'gsize'
> and 'size_t' being interchangeable.

But you can: gsize is defined to be "An unsigned integer type of the
result of the sizeof operator, corresponding to the size_t type defined
in C99. This type is wide enough to hold the numeric value of a pointer,
so it is usually 32bit wide on a 32bit platform and 64bit wide on a
64bit platform".

So it's impossible to disagree---the patch is correct in the sense that
it fixes a warning.  But it is not correct as far as its original
rationale (compiling with -m32) goes.  What you'll get is a library
compiled for 32-bit gsize, receving arguments from a program that passes
64-bit gsize.  That's an ABI mismatch, and one which is unlikely to work
on most 32-bit architectures; in this sense the patch is wrong.

If anything, I would add a QEMU_BUILD_BUG_ON(sizeof(gsize) !=
sizeof(size_t)) to catch the problem, since we've had many experienced
developers caught unprepared.  At which point we've added another safety
net and the patch becomes 101% correct---but you get a compilation error
anyway, so the original purpose of the patch is not fulfilled.

All in all, I don't think this patch is 2.3 material.

Paolo

> (I have no idea why glib sees fit to define its own versions
> of the standard types, but given that it does we should get
> the interfacing to them right...)

That's 1995-vintage portability for you. :(

Paolo

Re: [Qemu-devel] [PATCH] qga/commands-posix.c: Use correct types with g_base64_decode()

[Paolo Bonzini]

On 14/04/2015 15:41, Peter Maydell wrote:
> No, this is not-for-2.3. I think that the issue here is that
> merely adding -m32 isn't sufficient, because pkg-config when
> run by configure will end up pointing at a bunch of incorrect
> glib includes (for x86-64, not i386). So the actual solution
> is to give configure the right pkg-config, and then gsize and
> size_t are the same. There's a patch by John Snow for
> configure on-list for that.

Ah, that's reassuring. :)  I was surprised by the timing and by Michael
asking whether he should have posted a pull request.

Paolo

Re: [Qemu-devel] [PATCH] qga/commands-posix.c: Use correct types with g_base64_decode()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 723 bytes --]

On 04/14/2015 08:29 AM, Paolo Bonzini wrote:
> But you can: gsize is defined to be "An unsigned integer type of the
> result of the sizeof operator, corresponding to the size_t type defined
> in C99.

Good so far.

> This type is wide enough to hold the numeric value of a pointer,

Not true on platforms where size_t is 32 bits but pointers are 64 bits.
 Bug in glib documentation.

> 
> If anything, I would add a QEMU_BUILD_BUG_ON(sizeof(gsize) !=
> sizeof(size_t)) to catch the problem, since we've had many experienced
> developers caught unprepared.

That would be my preference as well.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] qga/commands-posix.c: Use correct types with g_base64_decode()

[Peter Maydell]

On 14 April 2015 at 15:29, Paolo Bonzini <pbonzini@redhat.com> wrote:
> If anything, I would add a QEMU_BUILD_BUG_ON(sizeof(gsize) !=
> sizeof(size_t)) to catch the problem, since we've had many experienced
> developers caught unprepared.

...put it in configure, since it's a configuration error?

-- PMM

Re: [Qemu-devel] [PATCH] qga/commands-posix.c: Use correct types with g_base64_decode()

[Paolo Bonzini]

On 14/04/2015 16:35, Eric Blake wrote:
> On 04/14/2015 08:29 AM, Paolo Bonzini wrote:
>> But you can: gsize is defined to be "An unsigned integer type of
>> the result of the sizeof operator, corresponding to the size_t
>> type defined in C99.
> 
> Good so far.
> 
>> This type is wide enough to hold the numeric value of a pointer,
> 
> Not true on platforms where size_t is 32 bits but pointers are 64
> bits.

Are there any?  I don't think QEMU supports them anyway.

Paolo

> Bug in glib documentation.
> 
>> 
>> If anything, I would add a QEMU_BUILD_BUG_ON(sizeof(gsize) != 
>> sizeof(size_t)) to catch the problem, since we've had many
>> experienced developers caught unprepared.
> 
> That would be my preference as well.
> 

Re: [Qemu-devel] [PATCH] qga/commands-posix.c: Use correct types with g_base64_decode()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1160 bytes --]

On 04/14/2015 08:50 AM, Paolo Bonzini wrote:
> 
> 
> On 14/04/2015 16:35, Eric Blake wrote:
>> On 04/14/2015 08:29 AM, Paolo Bonzini wrote:
>>> But you can: gsize is defined to be "An unsigned integer type of
>>> the result of the sizeof operator, corresponding to the size_t
>>> type defined in C99.
>>
>> Good so far.
>>
>>> This type is wide enough to hold the numeric value of a pointer,
>>
>> Not true on platforms where size_t is 32 bits but pointers are 64
>> bits.
> 
> Are there any?  I don't think QEMU supports them anyway.

Hmm. I looked, but mingw64 (32-bit long, 64-bit size_t and pointers) and
x32 ABI on x86_64 (32-bit size_t, but also 32-bit pointers) are both
safe, and I couldn't think of any other common platforms likely to be an
oddball.  Still, the point remains that the glib statement is not
required to be true, and only happens to work on the common platforms it
is ported to, because it IS within the bounds of the standard to
restrict maximum object size to be less than the range of a pointer.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]