Re: [PATCH v3 02/28] glib-compat: Introduce g_memdup2() wrapper
Daniel P. Berrangé writes:
> On Thu, Dec 16, 2021 at 02:11:37PM +, Alex Bennée wrote:
>>
>> Philippe Mathieu-Daudé writes:
>>
>> > When experimenting raising GLIB_VERSION_MIN_REQUIRED to 2.68
>> > (Fedora 34 provides GLib 2.68.1) we get:
>> >
>> > hw/virtio/virtio-crypto.c:245:24: error: 'g_memdup' is deprecated: Use
>> > 'g_memdup2' instead [-Werror,-Wdeprecated-declarations]
>> > ...
>> >
>> > g_memdup() has been updated by g_memdup2() to fix eventual security
>> > issues (size argument is 32-bit and could be truncated / wrapping).
>> > GLib recommends to copy their static inline version of g_memdup2():
>> > https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
>> >
>> > Our glib-compat.h provides a comment explaining how to deal with
>> > these deprecated declarations (see commit e71e8cc0355
>> > "glib: enforce the minimum required version and warn about old APIs").
>> >
>> > Following this comment suggestion, implement the g_memdup2_qemu()
>> > wrapper to g_memdup2(), and use the safer equivalent inlined when
>> > we are using pre-2.68 GLib.
>> >
>> > Reported-by: Eric Blake
>> > Signed-off-by: Philippe Mathieu-Daudé
>> > ---
>> > include/glib-compat.h | 37 +
>> > 1 file changed, 37 insertions(+)
>> >
>> > diff --git a/include/glib-compat.h b/include/glib-compat.h
>> > index 9e95c888f54..8d01a8c01fb 100644
>> > --- a/include/glib-compat.h
>> > +++ b/include/glib-compat.h
>> > @@ -68,6 +68,43 @@
>> > * without generating warnings.
>> > */
>> >
>> > +/*
>> > + * g_memdup2_qemu:
>> > + * @mem: (nullable): the memory to copy.
>> > + * @byte_size: the number of bytes to copy.
>> > + *
>> > + * Allocates @byte_size bytes of memory, and copies @byte_size bytes into
>> > it
>> > + * from @mem. If @mem is %NULL it returns %NULL.
>> > + *
>> > + * This replaces g_memdup(), which was prone to integer overflows when
>> > + * converting the argument from a #gsize to a #guint.
>> > + *
>> > + * This static inline version is a backport of the new public API from
>> > + * GLib 2.68, kept internal to GLib for backport to older stable releases.
>> > + * See https://gitlab.gnome.org/GNOME/glib/-/issues/2319.
>> > + *
>> > + * Returns: (nullable): a pointer to the newly-allocated copy of the
>> > memory,
>> > + * or %NULL if @mem is %NULL.
>> > + */
>> > +static inline gpointer g_memdup2_qemu(gconstpointer mem, gsize byte_size)
>> > +{
>> > +#if GLIB_CHECK_VERSION(2, 68, 0)
>> > +return g_memdup2(mem, byte_size);
>> > +#else
>> > +gpointer new_mem;
>> > +
>> > +if (mem && byte_size != 0) {
>> > +new_mem = g_malloc(byte_size);
>> > +memcpy(new_mem, mem, byte_size);
>> > +} else {
>> > +new_mem = NULL;
>> > +}
>> > +
>> > +return new_mem;
>> > +#endif
>> > +}
>> > +#define g_memdup2(m, s) g_memdup2_qemu(m, s)
>> > +
>>
>> As per our style wouldn't it make sense to just call it qemu_memdup(m,
>> s)?
>
> Not in this case. We use suffix as we don't want people calling this
> directly with the suffix.
>
> In the glibcompat.h header we're attempting to transparently/secretly
> replace/wrap standard glib APIs. All the callers should remain using
> the plain glib API name, never call the method with the suffix at
> all. This lets us delete the wrapper later and not have to update
> any callers. The suffix is basically just a hack of the impl we use
> for transparent replacement.
Right - at the risk of bike shedding names maybe we should choose a
suffix the better reflects the purpose like _alt or _internal rather
than overloading qemu?
We already document _locked for example.
> A method with a 'qemu_' prefix by constrast is something that callers
> are explicitly expected to call directly.
>
>
> Regards,
> Daniel
--
Alex Bennée
Re: [PATCH v3 02/28] glib-compat: Introduce g_memdup2() wrapper
On Fri, Dec 17, 2021 at 02:53:05PM +, Alex Bennée wrote:
>
> Daniel P. Berrangé writes:
>
> > On Thu, Dec 16, 2021 at 02:11:37PM +, Alex Bennée wrote:
> >>
> >> Philippe Mathieu-Daudé writes:
> >>
> >> > When experimenting raising GLIB_VERSION_MIN_REQUIRED to 2.68
> >> > (Fedora 34 provides GLib 2.68.1) we get:
> >> >
> >> > hw/virtio/virtio-crypto.c:245:24: error: 'g_memdup' is deprecated: Use
> >> > 'g_memdup2' instead [-Werror,-Wdeprecated-declarations]
> >> > ...
> >> >
> >> > g_memdup() has been updated by g_memdup2() to fix eventual security
> >> > issues (size argument is 32-bit and could be truncated / wrapping).
> >> > GLib recommends to copy their static inline version of g_memdup2():
> >> > https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
> >> >
> >> > Our glib-compat.h provides a comment explaining how to deal with
> >> > these deprecated declarations (see commit e71e8cc0355
> >> > "glib: enforce the minimum required version and warn about old APIs").
> >> >
> >> > Following this comment suggestion, implement the g_memdup2_qemu()
> >> > wrapper to g_memdup2(), and use the safer equivalent inlined when
> >> > we are using pre-2.68 GLib.
> >> >
> >> > Reported-by: Eric Blake
> >> > Signed-off-by: Philippe Mathieu-Daudé
> >> > ---
> >> > include/glib-compat.h | 37 +
> >> > 1 file changed, 37 insertions(+)
> >> >
> >> > diff --git a/include/glib-compat.h b/include/glib-compat.h
> >> > index 9e95c888f54..8d01a8c01fb 100644
> >> > --- a/include/glib-compat.h
> >> > +++ b/include/glib-compat.h
> >> > @@ -68,6 +68,43 @@
> >> > * without generating warnings.
> >> > */
> >> >
> >> > +/*
> >> > + * g_memdup2_qemu:
> >> > + * @mem: (nullable): the memory to copy.
> >> > + * @byte_size: the number of bytes to copy.
> >> > + *
> >> > + * Allocates @byte_size bytes of memory, and copies @byte_size bytes
> >> > into it
> >> > + * from @mem. If @mem is %NULL it returns %NULL.
> >> > + *
> >> > + * This replaces g_memdup(), which was prone to integer overflows when
> >> > + * converting the argument from a #gsize to a #guint.
> >> > + *
> >> > + * This static inline version is a backport of the new public API from
> >> > + * GLib 2.68, kept internal to GLib for backport to older stable
> >> > releases.
> >> > + * See https://gitlab.gnome.org/GNOME/glib/-/issues/2319.
> >> > + *
> >> > + * Returns: (nullable): a pointer to the newly-allocated copy of the
> >> > memory,
> >> > + * or %NULL if @mem is %NULL.
> >> > + */
> >> > +static inline gpointer g_memdup2_qemu(gconstpointer mem, gsize
> >> > byte_size)
> >> > +{
> >> > +#if GLIB_CHECK_VERSION(2, 68, 0)
> >> > +return g_memdup2(mem, byte_size);
> >> > +#else
> >> > +gpointer new_mem;
> >> > +
> >> > +if (mem && byte_size != 0) {
> >> > +new_mem = g_malloc(byte_size);
> >> > +memcpy(new_mem, mem, byte_size);
> >> > +} else {
> >> > +new_mem = NULL;
> >> > +}
> >> > +
> >> > +return new_mem;
> >> > +#endif
> >> > +}
> >> > +#define g_memdup2(m, s) g_memdup2_qemu(m, s)
> >> > +
> >>
> >> As per our style wouldn't it make sense to just call it qemu_memdup(m,
> >> s)?
> >
> > Not in this case. We use suffix as we don't want people calling this
> > directly with the suffix.
> >
> > In the glibcompat.h header we're attempting to transparently/secretly
> > replace/wrap standard glib APIs. All the callers should remain using
> > the plain glib API name, never call the method with the suffix at
> > all. This lets us delete the wrapper later and not have to update
> > any callers. The suffix is basically just a hack of the impl we use
> > for transparent replacement.
>
> Right - at the risk of bike shedding names maybe we should choose a
> suffix the better reflects the purpose like _alt or _internal rather
> than overloading qemu?
Sure, I'm fine with that.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v3 02/28] glib-compat: Introduce g_memdup2() wrapper
On Thu, Dec 16, 2021 at 02:11:37PM +, Alex Bennée wrote:
>
> Philippe Mathieu-Daudé writes:
>
> > When experimenting raising GLIB_VERSION_MIN_REQUIRED to 2.68
> > (Fedora 34 provides GLib 2.68.1) we get:
> >
> > hw/virtio/virtio-crypto.c:245:24: error: 'g_memdup' is deprecated: Use
> > 'g_memdup2' instead [-Werror,-Wdeprecated-declarations]
> > ...
> >
> > g_memdup() has been updated by g_memdup2() to fix eventual security
> > issues (size argument is 32-bit and could be truncated / wrapping).
> > GLib recommends to copy their static inline version of g_memdup2():
> > https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
> >
> > Our glib-compat.h provides a comment explaining how to deal with
> > these deprecated declarations (see commit e71e8cc0355
> > "glib: enforce the minimum required version and warn about old APIs").
> >
> > Following this comment suggestion, implement the g_memdup2_qemu()
> > wrapper to g_memdup2(), and use the safer equivalent inlined when
> > we are using pre-2.68 GLib.
> >
> > Reported-by: Eric Blake
> > Signed-off-by: Philippe Mathieu-Daudé
> > ---
> > include/glib-compat.h | 37 +
> > 1 file changed, 37 insertions(+)
> >
> > diff --git a/include/glib-compat.h b/include/glib-compat.h
> > index 9e95c888f54..8d01a8c01fb 100644
> > --- a/include/glib-compat.h
> > +++ b/include/glib-compat.h
> > @@ -68,6 +68,43 @@
> > * without generating warnings.
> > */
> >
> > +/*
> > + * g_memdup2_qemu:
> > + * @mem: (nullable): the memory to copy.
> > + * @byte_size: the number of bytes to copy.
> > + *
> > + * Allocates @byte_size bytes of memory, and copies @byte_size bytes into
> > it
> > + * from @mem. If @mem is %NULL it returns %NULL.
> > + *
> > + * This replaces g_memdup(), which was prone to integer overflows when
> > + * converting the argument from a #gsize to a #guint.
> > + *
> > + * This static inline version is a backport of the new public API from
> > + * GLib 2.68, kept internal to GLib for backport to older stable releases.
> > + * See https://gitlab.gnome.org/GNOME/glib/-/issues/2319.
> > + *
> > + * Returns: (nullable): a pointer to the newly-allocated copy of the
> > memory,
> > + * or %NULL if @mem is %NULL.
> > + */
> > +static inline gpointer g_memdup2_qemu(gconstpointer mem, gsize byte_size)
> > +{
> > +#if GLIB_CHECK_VERSION(2, 68, 0)
> > +return g_memdup2(mem, byte_size);
> > +#else
> > +gpointer new_mem;
> > +
> > +if (mem && byte_size != 0) {
> > +new_mem = g_malloc(byte_size);
> > +memcpy(new_mem, mem, byte_size);
> > +} else {
> > +new_mem = NULL;
> > +}
> > +
> > +return new_mem;
> > +#endif
> > +}
> > +#define g_memdup2(m, s) g_memdup2_qemu(m, s)
> > +
>
> As per our style wouldn't it make sense to just call it qemu_memdup(m,
> s)?
Not in this case. We use suffix as we don't want people calling this
directly with the suffix.
In the glibcompat.h header we're attempting to transparently/secretly
replace/wrap standard glib APIs. All the callers should remain using
the plain glib API name, never call the method with the suffix at
all. This lets us delete the wrapper later and not have to update
any callers. The suffix is basically just a hack of the impl we use
for transparent replacement.
A method with a 'qemu_' prefix by constrast is something that callers
are explicitly expected to call directly.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v3 02/28] glib-compat: Introduce g_memdup2() wrapper
On Fri, Dec 17, 2021 at 11:10:31AM +, Alex Bennée wrote:
>
> Philippe Mathieu-Daudé writes:
>
> > On 12/16/21 15:11, Alex Bennée wrote:
> >> Philippe Mathieu-Daudé writes:
> >>
> >>> When experimenting raising GLIB_VERSION_MIN_REQUIRED to 2.68
> >>> (Fedora 34 provides GLib 2.68.1) we get:
> >>>
> >>> hw/virtio/virtio-crypto.c:245:24: error: 'g_memdup' is deprecated: Use
> >>> 'g_memdup2' instead [-Werror,-Wdeprecated-declarations]
> >>> ...
> >>>
> >>> g_memdup() has been updated by g_memdup2() to fix eventual security
> >>> issues (size argument is 32-bit and could be truncated / wrapping).
> >>> GLib recommends to copy their static inline version of g_memdup2():
> >>> https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
> >>>
> >>> Our glib-compat.h provides a comment explaining how to deal with
> >>> these deprecated declarations (see commit e71e8cc0355
> >>> "glib: enforce the minimum required version and warn about old APIs").
> >>>
>
> >>> +#define g_memdup2(m, s) g_memdup2_qemu(m, s)
> >>> +
> >>
> >> As per our style wouldn't it make sense to just call it qemu_memdup(m,
> >> s)?
> >
> > I followed the documentation in include/glib-compat.h:
> >
> > /*
> > * Note that because of the GLIB_VERSION_MAX_ALLOWED constant above,
> > allowing
> > * use of functions from newer GLib via this compat header needs a little
> > * trickery to prevent warnings being emitted.
> > *
> > * Consider a function from newer glib-X.Y that we want to use
> > *
> > *int g_foo(const char *wibble)
> > *
> > * We must define a static inline function with the same signature that does
> > * what we need, but with a "_qemu" suffix e.g.
> > *
> > * static inline void g_foo_qemu(const char *wibble)
> > * {
> > * #if GLIB_CHECK_VERSION(X, Y, 0)
> > *g_foo(wibble)
> > * #else
> > *g_something_equivalent_in_older_glib(wibble);
> > * #endif
> > * }
> > *
> > * The #pragma at the top of this file turns off -Wdeprecated-declarations,
> > * ensuring this wrapper function impl doesn't trigger the compiler warning
> > * about using too new glib APIs. Finally we can do
> > *
> > * #define g_foo(a) g_foo_qemu(a)
> > *
> > * So now the code elsewhere in QEMU, which *does* have the
> > * -Wdeprecated-declarations warning active, can call g_foo(...) as normal,
> > * without generating warnings.
> > */
> >
> > which is how g_unix_get_passwd_entry_qemu() is implemented.
>
> Yet later we have qemu_g_test_slow following the style guide. Also I'm
> confused by the usage of g_unix_get_passwd_entry_qemu because the only
> place I see it in qga/commands-posix-ssh.c right before it does:
>
> #define g_unix_get_passwd_entry_qemu(username, err) \
>test_get_passwd_entry(username, err)
The g_unix_get_passwd_entry method is a bad example as it is
not following the guide for transparent replacement.
>
> although I think that only hold when the file is built with
> QGA_BUILD_UNIT_TEST.
>
> > Should we reword the documentation first?
>
> The original wording in glib-compat.h was added by Daniel in 2018 but
> the commit that added the password function comments:
>
> Since the fallback version is still unsafe, I would rather keep the
> _qemu postfix, to make sure it's not being misused by mistake. When/if
> necessary, we can implement a safer fallback and drop the _qemu suffix.
>
> So if we are going to make a distinction between a qemu prefix and
> suffix we should agree that and add it to the style document.
The glibcompat.h header should only be used for cases where
we are doing transparent replacement such that callers continue
using the normal glib API name, and the suffix is a hidden
impl detail only seen in the header.
I think in that particular case we should have just had
'qemu_unix_get_passwd_entry' defined in a completely separate
place like osdep.c.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v3 02/28] glib-compat: Introduce g_memdup2() wrapper
Laurent Vivier writes: > Alex, > > I've added this patch to my trivial patches branch, do you want I drop > it? No - the patch itself is fine. I would like to fix up the consistency later if we can agree on what it should be. -- Alex Bennée
Re: [PATCH v3 02/28] glib-compat: Introduce g_memdup2() wrapper
Alex,
I've added this patch to my trivial patches branch, do you want I drop it?
Thanks,
Laurent
On 17/12/2021 12:10, Alex Bennée wrote:
Philippe Mathieu-Daudé writes:
On 12/16/21 15:11, Alex Bennée wrote:
Philippe Mathieu-Daudé writes:
When experimenting raising GLIB_VERSION_MIN_REQUIRED to 2.68
(Fedora 34 provides GLib 2.68.1) we get:
hw/virtio/virtio-crypto.c:245:24: error: 'g_memdup' is deprecated: Use
'g_memdup2' instead [-Werror,-Wdeprecated-declarations]
...
g_memdup() has been updated by g_memdup2() to fix eventual security
issues (size argument is 32-bit and could be truncated / wrapping).
GLib recommends to copy their static inline version of g_memdup2():
https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
Our glib-compat.h provides a comment explaining how to deal with
these deprecated declarations (see commit e71e8cc0355
"glib: enforce the minimum required version and warn about old APIs").
+#define g_memdup2(m, s) g_memdup2_qemu(m, s)
+
As per our style wouldn't it make sense to just call it qemu_memdup(m,
s)?
I followed the documentation in include/glib-compat.h:
/*
* Note that because of the GLIB_VERSION_MAX_ALLOWED constant above,
allowing
* use of functions from newer GLib via this compat header needs a little
* trickery to prevent warnings being emitted.
*
* Consider a function from newer glib-X.Y that we want to use
*
*int g_foo(const char *wibble)
*
* We must define a static inline function with the same signature that does
* what we need, but with a "_qemu" suffix e.g.
*
* static inline void g_foo_qemu(const char *wibble)
* {
* #if GLIB_CHECK_VERSION(X, Y, 0)
*g_foo(wibble)
* #else
*g_something_equivalent_in_older_glib(wibble);
* #endif
* }
*
* The #pragma at the top of this file turns off -Wdeprecated-declarations,
* ensuring this wrapper function impl doesn't trigger the compiler warning
* about using too new glib APIs. Finally we can do
*
* #define g_foo(a) g_foo_qemu(a)
*
* So now the code elsewhere in QEMU, which *does* have the
* -Wdeprecated-declarations warning active, can call g_foo(...) as normal,
* without generating warnings.
*/
which is how g_unix_get_passwd_entry_qemu() is implemented.
Yet later we have qemu_g_test_slow following the style guide. Also I'm
confused by the usage of g_unix_get_passwd_entry_qemu because the only
place I see it in qga/commands-posix-ssh.c right before it does:
#define g_unix_get_passwd_entry_qemu(username, err) \
test_get_passwd_entry(username, err)
although I think that only hold when the file is built with
QGA_BUILD_UNIT_TEST.
Should we reword the documentation first?
The original wording in glib-compat.h was added by Daniel in 2018 but
the commit that added the password function comments:
Since the fallback version is still unsafe, I would rather keep the
_qemu postfix, to make sure it's not being misused by mistake. When/if
necessary, we can implement a safer fallback and drop the _qemu suffix.
So if we are going to make a distinction between a qemu prefix and
suffix we should agree that and add it to the style document.
Re: [PATCH v3 02/28] glib-compat: Introduce g_memdup2() wrapper
Philippe Mathieu-Daudé writes:
> On 12/16/21 15:11, Alex Bennée wrote:
>> Philippe Mathieu-Daudé writes:
>>
>>> When experimenting raising GLIB_VERSION_MIN_REQUIRED to 2.68
>>> (Fedora 34 provides GLib 2.68.1) we get:
>>>
>>> hw/virtio/virtio-crypto.c:245:24: error: 'g_memdup' is deprecated: Use
>>> 'g_memdup2' instead [-Werror,-Wdeprecated-declarations]
>>> ...
>>>
>>> g_memdup() has been updated by g_memdup2() to fix eventual security
>>> issues (size argument is 32-bit and could be truncated / wrapping).
>>> GLib recommends to copy their static inline version of g_memdup2():
>>> https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
>>>
>>> Our glib-compat.h provides a comment explaining how to deal with
>>> these deprecated declarations (see commit e71e8cc0355
>>> "glib: enforce the minimum required version and warn about old APIs").
>>>
>>> +#define g_memdup2(m, s) g_memdup2_qemu(m, s)
>>> +
>>
>> As per our style wouldn't it make sense to just call it qemu_memdup(m,
>> s)?
>
> I followed the documentation in include/glib-compat.h:
>
> /*
> * Note that because of the GLIB_VERSION_MAX_ALLOWED constant above,
> allowing
> * use of functions from newer GLib via this compat header needs a little
> * trickery to prevent warnings being emitted.
> *
> * Consider a function from newer glib-X.Y that we want to use
> *
> *int g_foo(const char *wibble)
> *
> * We must define a static inline function with the same signature that does
> * what we need, but with a "_qemu" suffix e.g.
> *
> * static inline void g_foo_qemu(const char *wibble)
> * {
> * #if GLIB_CHECK_VERSION(X, Y, 0)
> *g_foo(wibble)
> * #else
> *g_something_equivalent_in_older_glib(wibble);
> * #endif
> * }
> *
> * The #pragma at the top of this file turns off -Wdeprecated-declarations,
> * ensuring this wrapper function impl doesn't trigger the compiler warning
> * about using too new glib APIs. Finally we can do
> *
> * #define g_foo(a) g_foo_qemu(a)
> *
> * So now the code elsewhere in QEMU, which *does* have the
> * -Wdeprecated-declarations warning active, can call g_foo(...) as normal,
> * without generating warnings.
> */
>
> which is how g_unix_get_passwd_entry_qemu() is implemented.
Yet later we have qemu_g_test_slow following the style guide. Also I'm
confused by the usage of g_unix_get_passwd_entry_qemu because the only
place I see it in qga/commands-posix-ssh.c right before it does:
#define g_unix_get_passwd_entry_qemu(username, err) \
test_get_passwd_entry(username, err)
although I think that only hold when the file is built with
QGA_BUILD_UNIT_TEST.
> Should we reword the documentation first?
The original wording in glib-compat.h was added by Daniel in 2018 but
the commit that added the password function comments:
Since the fallback version is still unsafe, I would rather keep the
_qemu postfix, to make sure it's not being misused by mistake. When/if
necessary, we can implement a safer fallback and drop the _qemu suffix.
So if we are going to make a distinction between a qemu prefix and
suffix we should agree that and add it to the style document.
--
Alex Bennée
Re: [PATCH v3 02/28] glib-compat: Introduce g_memdup2() wrapper
Le 03/09/2021 à 19:44, Philippe Mathieu-Daudé a écrit :
When experimenting raising GLIB_VERSION_MIN_REQUIRED to 2.68
(Fedora 34 provides GLib 2.68.1) we get:
hw/virtio/virtio-crypto.c:245:24: error: 'g_memdup' is deprecated: Use
'g_memdup2' instead [-Werror,-Wdeprecated-declarations]
...
g_memdup() has been updated by g_memdup2() to fix eventual security
issues (size argument is 32-bit and could be truncated / wrapping).
GLib recommends to copy their static inline version of g_memdup2():
https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
Our glib-compat.h provides a comment explaining how to deal with
these deprecated declarations (see commit e71e8cc0355
"glib: enforce the minimum required version and warn about old APIs").
Following this comment suggestion, implement the g_memdup2_qemu()
wrapper to g_memdup2(), and use the safer equivalent inlined when
we are using pre-2.68 GLib.
Reported-by: Eric Blake
Signed-off-by: Philippe Mathieu-Daudé
---
include/glib-compat.h | 37 +
1 file changed, 37 insertions(+)
diff --git a/include/glib-compat.h b/include/glib-compat.h
index 9e95c888f54..8d01a8c01fb 100644
--- a/include/glib-compat.h
+++ b/include/glib-compat.h
@@ -68,6 +68,43 @@
* without generating warnings.
*/
+/*
+ * g_memdup2_qemu:
+ * @mem: (nullable): the memory to copy.
+ * @byte_size: the number of bytes to copy.
+ *
+ * Allocates @byte_size bytes of memory, and copies @byte_size bytes into it
+ * from @mem. If @mem is %NULL it returns %NULL.
+ *
+ * This replaces g_memdup(), which was prone to integer overflows when
+ * converting the argument from a #gsize to a #guint.
+ *
+ * This static inline version is a backport of the new public API from
+ * GLib 2.68, kept internal to GLib for backport to older stable releases.
+ * See https://gitlab.gnome.org/GNOME/glib/-/issues/2319.
+ *
+ * Returns: (nullable): a pointer to the newly-allocated copy of the memory,
+ * or %NULL if @mem is %NULL.
+ */
+static inline gpointer g_memdup2_qemu(gconstpointer mem, gsize byte_size)
+{
+#if GLIB_CHECK_VERSION(2, 68, 0)
+return g_memdup2(mem, byte_size);
+#else
+gpointer new_mem;
+
+if (mem && byte_size != 0) {
+new_mem = g_malloc(byte_size);
+memcpy(new_mem, mem, byte_size);
+} else {
+new_mem = NULL;
+}
+
+return new_mem;
+#endif
+}
+#define g_memdup2(m, s) g_memdup2_qemu(m, s)
+
#if defined(G_OS_UNIX)
/*
* Note: The fallback implementation is not MT-safe, and it returns a copy of
Applied to my trivial-patches branch.
Thanks,
Laurent
Re: [PATCH v3 02/28] glib-compat: Introduce g_memdup2() wrapper
On 12/16/21 15:11, Alex Bennée wrote:
> Philippe Mathieu-Daudé writes:
>
>> When experimenting raising GLIB_VERSION_MIN_REQUIRED to 2.68
>> (Fedora 34 provides GLib 2.68.1) we get:
>>
>> hw/virtio/virtio-crypto.c:245:24: error: 'g_memdup' is deprecated: Use
>> 'g_memdup2' instead [-Werror,-Wdeprecated-declarations]
>> ...
>>
>> g_memdup() has been updated by g_memdup2() to fix eventual security
>> issues (size argument is 32-bit and could be truncated / wrapping).
>> GLib recommends to copy their static inline version of g_memdup2():
>> https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
>>
>> Our glib-compat.h provides a comment explaining how to deal with
>> these deprecated declarations (see commit e71e8cc0355
>> "glib: enforce the minimum required version and warn about old APIs").
>>
>> Following this comment suggestion, implement the g_memdup2_qemu()
>> wrapper to g_memdup2(), and use the safer equivalent inlined when
>> we are using pre-2.68 GLib.
>>
>> Reported-by: Eric Blake
>> Signed-off-by: Philippe Mathieu-Daudé
>> ---
>> include/glib-compat.h | 37 +
>> 1 file changed, 37 insertions(+)
>>
>> diff --git a/include/glib-compat.h b/include/glib-compat.h
>> index 9e95c888f54..8d01a8c01fb 100644
>> --- a/include/glib-compat.h
>> +++ b/include/glib-compat.h
>> @@ -68,6 +68,43 @@
>> * without generating warnings.
>> */
>>
>> +/*
>> + * g_memdup2_qemu:
>> + * @mem: (nullable): the memory to copy.
>> + * @byte_size: the number of bytes to copy.
>> + *
>> + * Allocates @byte_size bytes of memory, and copies @byte_size bytes into it
>> + * from @mem. If @mem is %NULL it returns %NULL.
>> + *
>> + * This replaces g_memdup(), which was prone to integer overflows when
>> + * converting the argument from a #gsize to a #guint.
>> + *
>> + * This static inline version is a backport of the new public API from
>> + * GLib 2.68, kept internal to GLib for backport to older stable releases.
>> + * See https://gitlab.gnome.org/GNOME/glib/-/issues/2319.
>> + *
>> + * Returns: (nullable): a pointer to the newly-allocated copy of the memory,
>> + * or %NULL if @mem is %NULL.
>> + */
>> +static inline gpointer g_memdup2_qemu(gconstpointer mem, gsize byte_size)
>> +{
>> +#if GLIB_CHECK_VERSION(2, 68, 0)
>> +return g_memdup2(mem, byte_size);
>> +#else
>> +gpointer new_mem;
>> +
>> +if (mem && byte_size != 0) {
>> +new_mem = g_malloc(byte_size);
>> +memcpy(new_mem, mem, byte_size);
>> +} else {
>> +new_mem = NULL;
>> +}
>> +
>> +return new_mem;
>> +#endif
>> +}
>> +#define g_memdup2(m, s) g_memdup2_qemu(m, s)
>> +
>
> As per our style wouldn't it make sense to just call it qemu_memdup(m,
> s)?
I followed the documentation in include/glib-compat.h:
/*
* Note that because of the GLIB_VERSION_MAX_ALLOWED constant above,
allowing
* use of functions from newer GLib via this compat header needs a little
* trickery to prevent warnings being emitted.
*
* Consider a function from newer glib-X.Y that we want to use
*
*int g_foo(const char *wibble)
*
* We must define a static inline function with the same signature that does
* what we need, but with a "_qemu" suffix e.g.
*
* static inline void g_foo_qemu(const char *wibble)
* {
* #if GLIB_CHECK_VERSION(X, Y, 0)
*g_foo(wibble)
* #else
*g_something_equivalent_in_older_glib(wibble);
* #endif
* }
*
* The #pragma at the top of this file turns off -Wdeprecated-declarations,
* ensuring this wrapper function impl doesn't trigger the compiler warning
* about using too new glib APIs. Finally we can do
*
* #define g_foo(a) g_foo_qemu(a)
*
* So now the code elsewhere in QEMU, which *does* have the
* -Wdeprecated-declarations warning active, can call g_foo(...) as normal,
* without generating warnings.
*/
which is how g_unix_get_passwd_entry_qemu() is implemented.
Should we reword the documentation first?
Re: [PATCH v3 02/28] glib-compat: Introduce g_memdup2() wrapper
Philippe Mathieu-Daudé writes:
> When experimenting raising GLIB_VERSION_MIN_REQUIRED to 2.68
> (Fedora 34 provides GLib 2.68.1) we get:
>
> hw/virtio/virtio-crypto.c:245:24: error: 'g_memdup' is deprecated: Use
> 'g_memdup2' instead [-Werror,-Wdeprecated-declarations]
> ...
>
> g_memdup() has been updated by g_memdup2() to fix eventual security
> issues (size argument is 32-bit and could be truncated / wrapping).
> GLib recommends to copy their static inline version of g_memdup2():
> https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
>
> Our glib-compat.h provides a comment explaining how to deal with
> these deprecated declarations (see commit e71e8cc0355
> "glib: enforce the minimum required version and warn about old APIs").
>
> Following this comment suggestion, implement the g_memdup2_qemu()
> wrapper to g_memdup2(), and use the safer equivalent inlined when
> we are using pre-2.68 GLib.
>
> Reported-by: Eric Blake
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> include/glib-compat.h | 37 +
> 1 file changed, 37 insertions(+)
>
> diff --git a/include/glib-compat.h b/include/glib-compat.h
> index 9e95c888f54..8d01a8c01fb 100644
> --- a/include/glib-compat.h
> +++ b/include/glib-compat.h
> @@ -68,6 +68,43 @@
> * without generating warnings.
> */
>
> +/*
> + * g_memdup2_qemu:
> + * @mem: (nullable): the memory to copy.
> + * @byte_size: the number of bytes to copy.
> + *
> + * Allocates @byte_size bytes of memory, and copies @byte_size bytes into it
> + * from @mem. If @mem is %NULL it returns %NULL.
> + *
> + * This replaces g_memdup(), which was prone to integer overflows when
> + * converting the argument from a #gsize to a #guint.
> + *
> + * This static inline version is a backport of the new public API from
> + * GLib 2.68, kept internal to GLib for backport to older stable releases.
> + * See https://gitlab.gnome.org/GNOME/glib/-/issues/2319.
> + *
> + * Returns: (nullable): a pointer to the newly-allocated copy of the memory,
> + * or %NULL if @mem is %NULL.
> + */
> +static inline gpointer g_memdup2_qemu(gconstpointer mem, gsize byte_size)
> +{
> +#if GLIB_CHECK_VERSION(2, 68, 0)
> +return g_memdup2(mem, byte_size);
> +#else
> +gpointer new_mem;
> +
> +if (mem && byte_size != 0) {
> +new_mem = g_malloc(byte_size);
> +memcpy(new_mem, mem, byte_size);
> +} else {
> +new_mem = NULL;
> +}
> +
> +return new_mem;
> +#endif
> +}
> +#define g_memdup2(m, s) g_memdup2_qemu(m, s)
> +
As per our style wouldn't it make sense to just call it qemu_memdup(m,
s)?
--
Alex Bennée
Re: [PATCH v3 02/28] glib-compat: Introduce g_memdup2() wrapper
On Fri, Sep 03, 2021 at 07:44:44PM +0200, Philippe Mathieu-Daudé wrote: > When experimenting raising GLIB_VERSION_MIN_REQUIRED to 2.68 > (Fedora 34 provides GLib 2.68.1) we get: > > hw/virtio/virtio-crypto.c:245:24: error: 'g_memdup' is deprecated: Use > 'g_memdup2' instead [-Werror,-Wdeprecated-declarations] > ... > > g_memdup() has been updated by g_memdup2() to fix eventual security > issues (size argument is 32-bit and could be truncated / wrapping). > GLib recommends to copy their static inline version of g_memdup2(): > https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538 > > Our glib-compat.h provides a comment explaining how to deal with > these deprecated declarations (see commit e71e8cc0355 > "glib: enforce the minimum required version and warn about old APIs"). > > Following this comment suggestion, implement the g_memdup2_qemu() > wrapper to g_memdup2(), and use the safer equivalent inlined when > we are using pre-2.68 GLib. > > Reported-by: Eric Blake > Signed-off-by: Philippe Mathieu-Daudé > --- > include/glib-compat.h | 37 + > 1 file changed, 37 insertions(+) Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
[PATCH v3 02/28] glib-compat: Introduce g_memdup2() wrapper
When experimenting raising GLIB_VERSION_MIN_REQUIRED to 2.68
(Fedora 34 provides GLib 2.68.1) we get:
hw/virtio/virtio-crypto.c:245:24: error: 'g_memdup' is deprecated: Use
'g_memdup2' instead [-Werror,-Wdeprecated-declarations]
...
g_memdup() has been updated by g_memdup2() to fix eventual security
issues (size argument is 32-bit and could be truncated / wrapping).
GLib recommends to copy their static inline version of g_memdup2():
https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
Our glib-compat.h provides a comment explaining how to deal with
these deprecated declarations (see commit e71e8cc0355
"glib: enforce the minimum required version and warn about old APIs").
Following this comment suggestion, implement the g_memdup2_qemu()
wrapper to g_memdup2(), and use the safer equivalent inlined when
we are using pre-2.68 GLib.
Reported-by: Eric Blake
Signed-off-by: Philippe Mathieu-Daudé
---
include/glib-compat.h | 37 +
1 file changed, 37 insertions(+)
diff --git a/include/glib-compat.h b/include/glib-compat.h
index 9e95c888f54..8d01a8c01fb 100644
--- a/include/glib-compat.h
+++ b/include/glib-compat.h
@@ -68,6 +68,43 @@
* without generating warnings.
*/
+/*
+ * g_memdup2_qemu:
+ * @mem: (nullable): the memory to copy.
+ * @byte_size: the number of bytes to copy.
+ *
+ * Allocates @byte_size bytes of memory, and copies @byte_size bytes into it
+ * from @mem. If @mem is %NULL it returns %NULL.
+ *
+ * This replaces g_memdup(), which was prone to integer overflows when
+ * converting the argument from a #gsize to a #guint.
+ *
+ * This static inline version is a backport of the new public API from
+ * GLib 2.68, kept internal to GLib for backport to older stable releases.
+ * See https://gitlab.gnome.org/GNOME/glib/-/issues/2319.
+ *
+ * Returns: (nullable): a pointer to the newly-allocated copy of the memory,
+ * or %NULL if @mem is %NULL.
+ */
+static inline gpointer g_memdup2_qemu(gconstpointer mem, gsize byte_size)
+{
+#if GLIB_CHECK_VERSION(2, 68, 0)
+return g_memdup2(mem, byte_size);
+#else
+gpointer new_mem;
+
+if (mem && byte_size != 0) {
+new_mem = g_malloc(byte_size);
+memcpy(new_mem, mem, byte_size);
+} else {
+new_mem = NULL;
+}
+
+return new_mem;
+#endif
+}
+#define g_memdup2(m, s) g_memdup2_qemu(m, s)
+
#if defined(G_OS_UNIX)
/*
* Note: The fallback implementation is not MT-safe, and it returns a copy of
--
2.31.1
