[PATCH v3 for-7.2 0/6] Add format attributes and fix format strings

[PATCH v3 for-7.2 0/6] Add format attributes and fix format strings

[Stefan Weil via]

v3:
- Fix description for patch 3
- Add patches 5 and 6

The patches 3 and 5 still need reviews!

[PATCH v3 for-7.2 1/6] libvhost-user: Fix wrong type of argument to
[PATCH v3 for-7.2 2/6] libvhost-user: Fix format strings
[PATCH v3 for-7.2 3/6] libvhost-user: Fix two more format strings
[PATCH v3 for-7.2 4/6] libvhost-user: Add format attribute to local
[PATCH v3 for-7.2 5/6] MAINTAINERS: Add subprojects/libvhost-user to
[PATCH v3 for-7.2 6/6] Add G_GNUC_PRINTF to function

[PATCH v3 for-7.2 1/6] libvhost-user: Fix wrong type of argument to formatting function (reported by LGTM)

[Stefan Weil via]

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: Stefan Weil <sw@weilnetz.de>
Message-Id: <20220422070144.1043697-2-sw@weilnetz.de>
Signed-off-by: Laurent Vivier <laurent@vivier.eu>
---
 subprojects/libvhost-user/libvhost-user.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
index ffed4729a3..d9a6e3e556 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -651,7 +651,7 @@ generate_faults(VuDev *dev) {
 
         if (ioctl(dev->postcopy_ufd, UFFDIO_REGISTER, &reg_struct)) {
             vu_panic(dev, "%s: Failed to userfault region %d "
-                          "@%p + size:%zx offset: %zx: (ufd=%d)%s\n",
+                          "@%" PRIx64 " + size:%zx offset: %zx: (ufd=%d)%s\n",
                      __func__, i,
                      dev_region->mmap_addr,
                      dev_region->size, dev_region->mmap_offset,
-- 
2.35.1

[PATCH v3 for-7.2 2/6] libvhost-user: Fix format strings

[Stefan Weil via]

Signed-off-by: Stefan Weil <sw@weilnetz.de>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Message-Id: <20220422070144.1043697-3-sw@weilnetz.de>
Signed-off-by: Laurent Vivier <laurent@vivier.eu>
---
 subprojects/libvhost-user/libvhost-user.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
index d9a6e3e556..d67953a1c3 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -700,7 +700,7 @@ vu_add_mem_reg(VuDev *dev, VhostUserMsg *vmsg) {
     if (vmsg->size < VHOST_USER_MEM_REG_SIZE) {
         close(vmsg->fds[0]);
         vu_panic(dev, "VHOST_USER_ADD_MEM_REG requires a message size of at "
-                      "least %d bytes and only %d bytes were received",
+                      "least %zu bytes and only %d bytes were received",
                       VHOST_USER_MEM_REG_SIZE, vmsg->size);
         return false;
     }
@@ -826,7 +826,7 @@ vu_rem_mem_reg(VuDev *dev, VhostUserMsg *vmsg) {
     if (vmsg->size < VHOST_USER_MEM_REG_SIZE) {
         vmsg_close_fds(vmsg);
         vu_panic(dev, "VHOST_USER_REM_MEM_REG requires a message size of at "
-                      "least %d bytes and only %d bytes were received",
+                      "least %zu bytes and only %d bytes were received",
                       VHOST_USER_MEM_REG_SIZE, vmsg->size);
         return false;
     }
-- 
2.35.1

[PATCH v3 for-7.2 3/6] libvhost-user: Fix two more format strings

[Stefan Weil via]

This fix is required for 32 bit hosts. The bug was detected by CI
for arm-linux, but is also relevant for i386-linux.

Reported-by: Stefan Hajnoczi <stefanha@gmail.com>
Signed-off-by: Stefan Weil <sw@weilnetz.de>
---
 subprojects/libvhost-user/libvhost-user.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
index d67953a1c3..80f9952e71 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -651,7 +651,8 @@ generate_faults(VuDev *dev) {
 
         if (ioctl(dev->postcopy_ufd, UFFDIO_REGISTER, &reg_struct)) {
             vu_panic(dev, "%s: Failed to userfault region %d "
-                          "@%" PRIx64 " + size:%zx offset: %zx: (ufd=%d)%s\n",
+                          "@%" PRIx64 " + size:%" PRIx64 " offset: %" PRIx64
+                          ": (ufd=%d)%s\n",
                      __func__, i,
                      dev_region->mmap_addr,
                      dev_region->size, dev_region->mmap_offset,
-- 
2.35.1

Re: [PATCH v3 for-7.2 3/6] libvhost-user: Fix two more format strings

[Stefan Hajnoczi]

On Sat, 26 Nov 2022 at 10:25, Stefan Weil <sw@weilnetz.de> wrote:
>
> This fix is required for 32 bit hosts. The bug was detected by CI
> for arm-linux, but is also relevant for i386-linux.
>
> Reported-by: Stefan Hajnoczi <stefanha@gmail.com>
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> ---
>  subprojects/libvhost-user/libvhost-user.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

>
> diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
> index d67953a1c3..80f9952e71 100644
> --- a/subprojects/libvhost-user/libvhost-user.c
> +++ b/subprojects/libvhost-user/libvhost-user.c
> @@ -651,7 +651,8 @@ generate_faults(VuDev *dev) {
>
>          if (ioctl(dev->postcopy_ufd, UFFDIO_REGISTER, &reg_struct)) {
>              vu_panic(dev, "%s: Failed to userfault region %d "
> -                          "@%" PRIx64 " + size:%zx offset: %zx: (ufd=%d)%s\n",
> +                          "@%" PRIx64 " + size:%" PRIx64 " offset: %" PRIx64
> +                          ": (ufd=%d)%s\n",
>                       __func__, i,
>                       dev_region->mmap_addr,
>                       dev_region->size, dev_region->mmap_offset,
> --
> 2.35.1
>

[PATCH v3 for-7.2 4/6] libvhost-user: Add format attribute to local function vu_panic

[Stefan Weil via]

Signed-off-by: Stefan Weil <sw@weilnetz.de>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Message-Id: <20220422070144.1043697-4-sw@weilnetz.de>
Signed-off-by: Laurent Vivier <laurent@vivier.eu>
---
 subprojects/libvhost-user/libvhost-user.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
index 80f9952e71..d6ee6e7d91 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -45,6 +45,17 @@
 #include "libvhost-user.h"
 
 /* usually provided by GLib */
+#if     __GNUC__ > 2 || (__GNUC__ == 2 && __GNUC_MINOR__ > 4)
+#if !defined(__clang__) && (__GNUC__ == 4 && __GNUC_MINOR__ == 4)
+#define G_GNUC_PRINTF(format_idx, arg_idx) \
+  __attribute__((__format__(gnu_printf, format_idx, arg_idx)))
+#else
+#define G_GNUC_PRINTF(format_idx, arg_idx) \
+  __attribute__((__format__(__printf__, format_idx, arg_idx)))
+#endif
+#else   /* !__GNUC__ */
+#define G_GNUC_PRINTF(format_idx, arg_idx)
+#endif  /* !__GNUC__ */
 #ifndef MIN
 #define MIN(x, y) ({                            \
             typeof(x) _min1 = (x);              \
@@ -151,7 +162,7 @@ vu_request_to_string(unsigned int req)
     }
 }
 
-static void
+static void G_GNUC_PRINTF(2, 3)
 vu_panic(VuDev *dev, const char *msg, ...)
 {
     char *buf = NULL;
-- 
2.35.1

Re: [PATCH v3 for-7.2 4/6] libvhost-user: Add format attribute to local function vu_panic

[Stefan Hajnoczi]

On Sat, 26 Nov 2022 at 10:25, Stefan Weil <sw@weilnetz.de> wrote:
>
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Message-Id: <20220422070144.1043697-4-sw@weilnetz.de>
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> ---
>  subprojects/libvhost-user/libvhost-user.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)

I would rather not merge something that can cause new build failures
this late in the release cycle.

If you respin, please make this a separate 8.0 patch.

> diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
> index 80f9952e71..d6ee6e7d91 100644
> --- a/subprojects/libvhost-user/libvhost-user.c
> +++ b/subprojects/libvhost-user/libvhost-user.c
> @@ -45,6 +45,17 @@
>  #include "libvhost-user.h"
>
>  /* usually provided by GLib */
> +#if     __GNUC__ > 2 || (__GNUC__ == 2 && __GNUC_MINOR__ > 4)
> +#if !defined(__clang__) && (__GNUC__ == 4 && __GNUC_MINOR__ == 4)
> +#define G_GNUC_PRINTF(format_idx, arg_idx) \
> +  __attribute__((__format__(gnu_printf, format_idx, arg_idx)))
> +#else
> +#define G_GNUC_PRINTF(format_idx, arg_idx) \
> +  __attribute__((__format__(__printf__, format_idx, arg_idx)))
> +#endif
> +#else   /* !__GNUC__ */
> +#define G_GNUC_PRINTF(format_idx, arg_idx)
> +#endif  /* !__GNUC__ */
>  #ifndef MIN
>  #define MIN(x, y) ({                            \
>              typeof(x) _min1 = (x);              \
> @@ -151,7 +162,7 @@ vu_request_to_string(unsigned int req)
>      }
>  }
>
> -static void
> +static void G_GNUC_PRINTF(2, 3)
>  vu_panic(VuDev *dev, const char *msg, ...)
>  {
>      char *buf = NULL;
> --
> 2.35.1
>

Re: [PATCH v3 for-7.2 4/6] libvhost-user: Add format attribute to local function vu_panic

[Stefan Weil via]

Am 27.11.22 um 19:14 schrieb Stefan Hajnoczi:

> On Sat, 26 Nov 2022 at 10:25, Stefan Weil <sw@weilnetz.de> wrote:
>> Signed-off-by: Stefan Weil <sw@weilnetz.de>
>> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> Message-Id: <20220422070144.1043697-4-sw@weilnetz.de>
>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
>> ---
>>   subprojects/libvhost-user/libvhost-user.c | 13 ++++++++++++-
>>   1 file changed, 12 insertions(+), 1 deletion(-)
> I would rather not merge something that can cause new build failures
> this late in the release cycle.
>
> If you respin, please make this a separate 8.0 patch.


It would only cause a build failure if there remained more format bugs, 
but the last ones were fixed in patch 3. I tested a build for 32 bit ARM 
(which previously failed without patch 3), and it works now.

But you are right, patch 4 is not release critical as it is not a bug 
fix (like the other ones), so not including it in 7.2 might be an option.

Stefan

[PATCH v3 for-7.2 5/6] MAINTAINERS: Add subprojects/libvhost-user to section "vhost"

[Stefan Weil via]

Signed-off-by: Stefan Weil <sw@weilnetz.de>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index cf24910249..6966490c94 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2005,6 +2005,7 @@ F: docs/interop/vhost-user.rst
 F: contrib/vhost-user-*/
 F: backends/vhost-user.c
 F: include/sysemu/vhost-user-backend.h
+F: subprojects/libvhost-user/
 
 virtio
 M: Michael S. Tsirkin <mst@redhat.com>
-- 
2.35.1

Re: [PATCH v3 for-7.2 5/6] MAINTAINERS: Add subprojects/libvhost-user to section "vhost"

[Stefan Hajnoczi]

On Sat, 26 Nov 2022 at 10:25, Stefan Weil <sw@weilnetz.de> wrote:
>
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> ---
>  MAINTAINERS | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index cf24910249..6966490c94 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2005,6 +2005,7 @@ F: docs/interop/vhost-user.rst
>  F: contrib/vhost-user-*/
>  F: backends/vhost-user.c
>  F: include/sysemu/vhost-user-backend.h
> +F: subprojects/libvhost-user/

Requires agreement from Michael. Also including Marc-André who
originally created libvhost-user.

Stefan

Re: [PATCH v3 for-7.2 5/6] MAINTAINERS: Add subprojects/libvhost-user to section "vhost"

[Stefan Hajnoczi]

On Sun, 27 Nov 2022 at 13:09, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>
> On Sat, 26 Nov 2022 at 10:25, Stefan Weil <sw@weilnetz.de> wrote:
> >
> > Signed-off-by: Stefan Weil <sw@weilnetz.de>
> > ---
> >  MAINTAINERS | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index cf24910249..6966490c94 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -2005,6 +2005,7 @@ F: docs/interop/vhost-user.rst
> >  F: contrib/vhost-user-*/
> >  F: backends/vhost-user.c
> >  F: include/sysemu/vhost-user-backend.h
> > +F: subprojects/libvhost-user/
>
> Requires agreement from Michael. Also including Marc-André who
> originally created libvhost-user.

Stefan showed me the email where Michael agreed:
https://lore.kernel.org/qemu-devel/20221123015218-mutt-send-email-mst@kernel.org/

Stefan

[PATCH v3 for-7.2 6/6] Add G_GNUC_PRINTF to function qemu_set_info_str and fix related issues

[Stefan Weil via]

With the G_GNUC_PRINTF function attribute the compiler detects
two potential insecure format strings:

../../../net/stream.c:248:31: warning: format string is not a string literal (potentially insecure) [-Wformat-security]
    qemu_set_info_str(&s->nc, uri);
                              ^~~
../../../net/stream.c:322:31: warning: format string is not a string literal (potentially insecure) [-Wformat-security]
    qemu_set_info_str(&s->nc, uri);
                              ^~~

There are also two other warnings:

../../../net/socket.c:182:35: warning: zero-length gnu_printf format string [-Wformat-zero-length]
  182 |         qemu_set_info_str(&s->nc, "");
      |                                   ^~
../../../net/stream.c:170:35: warning: zero-length gnu_printf format string [-Wformat-zero-length]
  170 |         qemu_set_info_str(&s->nc, "");

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Stefan Weil <sw@weilnetz.de>
---
 include/net/net.h | 3 ++-
 net/socket.c      | 2 +-
 net/stream.c      | 6 +++---
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/include/net/net.h b/include/net/net.h
index 3db75ff841..dc20b31e9f 100644
--- a/include/net/net.h
+++ b/include/net/net.h
@@ -177,7 +177,8 @@ ssize_t qemu_send_packet_async(NetClientState *nc, const uint8_t *buf,
 void qemu_purge_queued_packets(NetClientState *nc);
 void qemu_flush_queued_packets(NetClientState *nc);
 void qemu_flush_or_purge_queued_packets(NetClientState *nc, bool purge);
-void qemu_set_info_str(NetClientState *nc, const char *fmt, ...);
+void qemu_set_info_str(NetClientState *nc,
+                       const char *fmt, ...) G_GNUC_PRINTF(2, 3);
 void qemu_format_nic_info_str(NetClientState *nc, uint8_t macaddr[6]);
 bool qemu_has_ufo(NetClientState *nc);
 bool qemu_has_vnet_hdr(NetClientState *nc);
diff --git a/net/socket.c b/net/socket.c
index 4944bb70d5..e62137c839 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -179,7 +179,7 @@ static void net_socket_send(void *opaque)
         s->fd = -1;
         net_socket_rs_init(&s->rs, net_socket_rs_finalize, false);
         s->nc.link_down = true;
-        qemu_set_info_str(&s->nc, "");
+        qemu_set_info_str(&s->nc, "%s", "");
 
         return;
     }
diff --git a/net/stream.c b/net/stream.c
index 53b7040cc4..37ff727e0c 100644
--- a/net/stream.c
+++ b/net/stream.c
@@ -167,7 +167,7 @@ static gboolean net_stream_send(QIOChannel *ioc,
 
         net_socket_rs_init(&s->rs, net_stream_rs_finalize, false);
         s->nc.link_down = true;
-        qemu_set_info_str(&s->nc, "");
+        qemu_set_info_str(&s->nc, "%s", "");
 
         qapi_event_send_netdev_stream_disconnected(s->nc.name);
 
@@ -245,7 +245,7 @@ static void net_stream_listen(QIONetListener *listener,
     }
     g_assert(addr != NULL);
     uri = socket_uri(addr);
-    qemu_set_info_str(&s->nc, uri);
+    qemu_set_info_str(&s->nc, "%s", uri);
     g_free(uri);
     qapi_event_send_netdev_stream_connected(s->nc.name, addr);
     qapi_free_SocketAddress(addr);
@@ -319,7 +319,7 @@ static void net_stream_client_connected(QIOTask *task, gpointer opaque)
     addr = qio_channel_socket_get_remote_address(sioc, NULL);
     g_assert(addr != NULL);
     uri = socket_uri(addr);
-    qemu_set_info_str(&s->nc, uri);
+    qemu_set_info_str(&s->nc, "%s", uri);
     g_free(uri);
 
     ret = qemu_socket_try_set_nonblock(sioc->fd);
-- 
2.35.1

Re: [PATCH v3 for-7.2 0/6] Add format attributes and fix format strings

[Stefan Hajnoczi]

We need to wait for Michael to agree to maintainership in patch 5. If
we run out of time I suggest splitting out patch 5.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Re: [PATCH v3 for-7.2 0/6] Add format attributes and fix format strings

[Stefan Weil via]

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

Am 27.11.22 um 19:23 schrieb Stefan Hajnoczi:

> We need to wait for Michael to agree to maintainership in patch 5. If
> we run out of time I suggest splitting out patch 5.
>
> Reviewed-by: Stefan Hajnoczi<stefanha@redhat.com>


Citing Michael from a v2 email: "pls do".

Stefan


>     Hello Michael,
>
>     I just noticed that MAINTAINERS has no entry for the files in
>     subprojects/libvhost-user, so I did not cc you in my previous e-mails.
>     Should that directory be added to the "vhost" section"?
>
>     Stefan

    pls do

[-- Attachment #2: Type: text/html, Size: 1178 bytes --]

Re: [PATCH v3 for-7.2 0/6] Add format attributes and fix format strings

[Stefan Hajnoczi]

On Sun, 27 Nov 2022 at 13:31, Stefan Weil <sw@weilnetz.de> wrote:
>
> Am 27.11.22 um 19:23 schrieb Stefan Hajnoczi:
>
> We need to wait for Michael to agree to maintainership in patch 5. If
> we run out of time I suggest splitting out patch 5.
>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>
>
> Citing Michael from a v2 email: "pls do".
>
> Stefan
>
>
> Hello Michael,
>
> I just noticed that MAINTAINERS has no entry for the files in
> subprojects/libvhost-user, so I did not cc you in my previous e-mails.
> Should that directory be added to the "vhost" section"?
>
> Stefan
>
> pls do

Perfect, thank you! I have added a reference to that email in the
commit description.

This series is running through CI now and will be merged upon success
later today.

Thanks,
Stefan

Re: [PATCH v3 for-7.2 0/6] Add format attributes and fix format strings

[Stefan Hajnoczi]

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

Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/7.2 for any user-visible changes.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 for-7.2 0/6] Add format attributes and fix format strings

[Michael S. Tsirkin]

On Sat, Nov 26, 2022 at 04:25:01PM +0100, Stefan Weil wrote:
> v3:
> - Fix description for patch 3
> - Add patches 5 and 6
> 
> The patches 3 and 5 still need reviews!

Series:
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

Stefan you said you want to merge this?

> [PATCH v3 for-7.2 1/6] libvhost-user: Fix wrong type of argument to
> [PATCH v3 for-7.2 2/6] libvhost-user: Fix format strings
> [PATCH v3 for-7.2 3/6] libvhost-user: Fix two more format strings
> [PATCH v3 for-7.2 4/6] libvhost-user: Add format attribute to local
> [PATCH v3 for-7.2 5/6] MAINTAINERS: Add subprojects/libvhost-user to
> [PATCH v3 for-7.2 6/6] Add G_GNUC_PRINTF to function

Re: [PATCH v3 for-7.2 0/6] Add format attributes and fix format strings

[Stefan Hajnoczi]

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

On Mon, Nov 28, 2022, 05:29 Michael S. Tsirkin <mst@redhat.com> wrote:

> On Sat, Nov 26, 2022 at 04:25:01PM +0100, Stefan Weil wrote:
> > v3:
> > - Fix description for patch 3
> > - Add patches 5 and 6
> >
> > The patches 3 and 5 still need reviews!
>
> Series:
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>
> Stefan you said you want to merge this?
>

Yes.

Stefan

>

[-- Attachment #2: Type: text/html, Size: 1031 bytes --]