[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v3 02/28] glib-compat: Introduce g_memdup2() wrapper

From: Laurent Vivier
Subject: Re: [PATCH v3 02/28] glib-compat: Introduce g_memdup2() wrapper
Date: Fri, 17 Dec 2021 12:41:47 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.3.0


I've added this patch to my trivial patches branch, do you want I drop it?


On 17/12/2021 12:10, Alex Bennée wrote:
Philippe Mathieu-Daudé <philmd@redhat.com> writes:

On 12/16/21 15:11, Alex Bennée wrote:
Philippe Mathieu-Daudé <philmd@redhat.com> 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():

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,
I followed the documentation in include/glib-compat.h:

  * Note that because of the GLIB_VERSION_MAX_ALLOWED constant above,
  * 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

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.

reply via email to

[Prev in Thread] Current Thread [Next in Thread]