[PATCH 0/2] virtio-9p: move G_GNUC_PRINTF to header

[PATCH 0/2] virtio-9p: move G_GNUC_PRINTF to header

[Sean Wei]

v9fs_string_sprintf() and v9fs_path_sprintf() already have
G_GNUC_PRINTF annotations in their own *.c files, but the
prototypes in the corresponding headers lack them.  When another
translation unit includes only the header, -Wformat can no longer
validate the argument list.

This series relocates the annotations to fsdev/9p-marshal.h and
hw/9pfs/9p.h, then drops the now-redundant annotations in
the *.c files.  There is no functional change.

I've checked all call sites for these two helper function, all
of them already passes the correct number of arguments.

A minimal PoC (sent as the next mail in the thread) demo how
G_GNUC_PRINTF behaves differently when the attribute is present
only in code.c or code.h file.

--

Sean Wei (3):
  fsdev/9p-marshal: move G_GNUC_PRINTF to header
  hw/9pfs: move G_GNUC_PRINTF to header

 fsdev/9p-marshal.c |  3 +--
 fsdev/9p-marshal.h |  2 +-
 hw/9pfs/9p.c       |  3 +--
 hw/9pfs/9p.h       |  2 +-
 4 files changed, 4 insertions(+), 6 deletions(-)

-- 
2.49.0

[PATCH 1/2] fsdev/9p-marshal: move G_GNUC_PRINTF to header

[Sean Wei]

v9fs_string_sprintf() is annotated with G_GNUC_PRINTF(2, 3) in
9p-marshal.c, but the prototype in fsdev/9p-marshal.h is missing the
attribute, so callers that include only the header do not get format
checking.

Move the annotation to the header and delete the duplicate in the
source file. No behavior change.

Signed-off-by: Sean Wei <me@sean.taipei>
---
 fsdev/9p-marshal.c | 3 +--
 fsdev/9p-marshal.h | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/fsdev/9p-marshal.c b/fsdev/9p-marshal.c
index f9b0336cd5..3455580703 100644
--- a/fsdev/9p-marshal.c
+++ b/fsdev/9p-marshal.c
@@ -27,8 +27,7 @@ void v9fs_string_free(V9fsString *str)
     str->size = 0;
 }
 
-void G_GNUC_PRINTF(2, 3)
-v9fs_string_sprintf(V9fsString *str, const char *fmt, ...)
+void v9fs_string_sprintf(V9fsString *str, const char *fmt, ...)
 {
     va_list ap;
 
diff --git a/fsdev/9p-marshal.h b/fsdev/9p-marshal.h
index f1abbe151c..e8c0ef0e11 100644
--- a/fsdev/9p-marshal.h
+++ b/fsdev/9p-marshal.h
@@ -76,7 +76,7 @@ static inline void v9fs_string_init(V9fsString *str)
     str->size = 0;
 }
 void v9fs_string_free(V9fsString *str);
-void v9fs_string_sprintf(V9fsString *str, const char *fmt, ...);
+void G_GNUC_PRINTF(2, 3) v9fs_string_sprintf(V9fsString *str, const char *fmt, ...);
 void v9fs_string_copy(V9fsString *lhs, V9fsString *rhs);
 
 #endif
-- 
2.49.0

[PATCH 2/2] hw/9pfs: move G_GNUC_PRINTF to header

[Sean Wei]

v9fs_path_sprintf() is annotated with G_GNUC_PRINTF(2, 3) in
hw/9pfs/9p.c, but the prototype in hw/9pfs/9p.h is missing the
attribute, so callers that include only the header do not get format
checking.

Move the annotation to the header and delete the duplicate in the
source file. No behavior change.

Signed-off-by: Sean Wei <me@sean.taipei>
---
 hw/9pfs/9p.c | 3 +--
 hw/9pfs/9p.h | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 8b001b9112..acfa7db4e1 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -201,8 +201,7 @@ void v9fs_path_free(V9fsPath *path)
 }
 
 
-void G_GNUC_PRINTF(2, 3)
-v9fs_path_sprintf(V9fsPath *path, const char *fmt, ...)
+void v9fs_path_sprintf(V9fsPath *path, const char *fmt, ...)
 {
     va_list ap;
 
diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
index 259ad32ed1..f4f4086cf7 100644
--- a/hw/9pfs/9p.h
+++ b/hw/9pfs/9p.h
@@ -456,7 +456,7 @@ static inline uint8_t v9fs_request_cancelled(V9fsPDU *pdu)
 void coroutine_fn v9fs_reclaim_fd(V9fsPDU *pdu);
 void v9fs_path_init(V9fsPath *path);
 void v9fs_path_free(V9fsPath *path);
-void v9fs_path_sprintf(V9fsPath *path, const char *fmt, ...);
+void G_GNUC_PRINTF(2, 3) v9fs_path_sprintf(V9fsPath *path, const char *fmt, ...);
 void v9fs_path_copy(V9fsPath *dst, const V9fsPath *src);
 size_t v9fs_readdir_response_size(V9fsString *name);
 int v9fs_name_to_path(V9fsState *s, V9fsPath *dirpath,
-- 
2.49.0

[PoC] show header-vs-source G_GNUC_PRINTF behavior

[Sean Wei]

This patch is NOT meant for merge.
Just a simple proof-of-concept so reviewers can quickly reproduce
the difference between placing a G_GNUC_PRINTF attribute in *.c versus
*.h file.

What it tests
-------------

* Two identical printf-style helpers (my_printf) are provided:
  - v1: attribute lives in foo-v1.c
  - v2: attribute lives in foo-v2.h
* Two callers (bar-v1.c, bar-v2.c) intentionally pass too few
  arguments: `my_printf("%d %d %d\n", 1)`.
* A trivial Makefile builds each variant and shows whether GCC emits
  the missing-argument warning.

Expected result
---------------

$ make -C poc run
// Compiling version 1 (Place G_GNUC_PRINTF in 'foo-v1.c')
No warning will appear, sliently cause security flaw

// Compiling version 2 (Place G_GNUC_PRINTF in 'foo-v2.h')
bar-v2.c:4:17: warning: more '%' conversions than data arguments [-Wformat-insufficient-args]



Signed-off-by: Sean Wei <me@sean.taipei>
---
 poc/foo-v1.h  |  3 +++
 poc/foo-v1.c  | 10 ++++++++++
 poc/bar-v1.c  |  6 ++++++
 poc/foo-v2.h  |  3 +++
 poc/foo-v2.c  | 10 ++++++++++
 poc/bar-v2.c  |  6 ++++++
 poc/Makefile  | 39 +++++++++++++++++++++++++++++++++++++++
 7 files changed, 77 insertions(+)
 create mode 100644 poc/foo-v1.h
 create mode 100644 poc/foo-v1.c
 create mode 100644 poc/bar-v1.c
 create mode 100644 poc/foo-v2.h
 create mode 100644 poc/foo-v2.c
 create mode 100644 poc/bar-v2.c
 create mode 100644 poc/Makefile

diff --git a/poc/foo-v1.h b/poc/foo-v1.h
new file mode 100644
index 0000000000..37b5403ba6
--- /dev/null
+++ b/poc/foo-v1.h
@@ -0,0 +1,3 @@
+#define G_GNUC_PRINTF(n, m) __attribute__((format(printf, n, m)))
+
+void my_printf(const char *fmt, ...);
diff --git a/poc/foo-v1.c b/poc/foo-v1.c
new file mode 100644
index 0000000000..ce89f95e3e
--- /dev/null
+++ b/poc/foo-v1.c
@@ -0,0 +1,10 @@
+#include <stdio.h>
+#include <stdarg.h>
+#include "foo-v2.h"
+
+void G_GNUC_PRINTF(1, 2) my_printf(const char *fmt, ...) {
+	va_list ap;
+	va_start(ap, fmt);
+	vprintf(fmt, ap);
+	va_end(ap);
+}
diff --git a/poc/bar-v1.c b/poc/bar-v1.c
new file mode 100644
index 0000000000..6e707e13b4
--- /dev/null
+++ b/poc/bar-v1.c
@@ -0,0 +1,6 @@
+#include "foo-v1.h"
+
+int main() {
+	my_printf("%d %d %d\n", 1);  // missing arguments
+	return 0;
+}
diff --git a/poc/foo-v2.h b/poc/foo-v2.h
new file mode 100644
index 0000000000..8bb56d3181
--- /dev/null
+++ b/poc/foo-v2.h
@@ -0,0 +1,3 @@
+#define G_GNUC_PRINTF(n, m) __attribute__((format(printf, n, m)))
+
+void G_GNUC_PRINTF(1, 2) my_printf(const char *fmt, ...);
diff --git a/poc/foo-v2.c b/poc/foo-v2.c
new file mode 100644
index 0000000000..3f18632ee6
--- /dev/null
+++ b/poc/foo-v2.c
@@ -0,0 +1,10 @@
+#include <stdio.h>
+#include <stdarg.h>
+#include "foo-v1.h"
+
+void my_printf(const char *fmt, ...) {
+	va_list ap;
+	va_start(ap, fmt);
+	vprintf(fmt, ap);
+	va_end(ap);
+}
diff --git a/poc/bar-v2.c b/poc/bar-v2.c
new file mode 100644
index 0000000000..e06fd87056
--- /dev/null
+++ b/poc/bar-v2.c
@@ -0,0 +1,6 @@
+#include "foo-v2.h"
+
+int main() {
+	my_printf("%d %d %d\n", 1);  // missing arguments
+	return 0;
+}
diff --git a/poc/Makefile b/poc/Makefile
new file mode 100644
index 0000000000..5725c9ff63
--- /dev/null
+++ b/poc/Makefile
@@ -0,0 +1,39 @@
+CC ?= gcc
+CFLAGS ?= -Wall
+
+TARGET = prog-v1 prog-v2
+SRCS   = foo-v1.c foo-v2.c bar-v1.c bar-v2.c
+OBJS   = $(SRCS:.c=.o)
+
+.PHONY: run
+
+run: clean prog-v1 prog-v2
+
+
+prog-v1:
+	@echo
+	@echo
+	@echo "### Compiling version 1 ###"
+	@echo "Place G_GNUC_PRINTF in 'foo.c' only"
+	@echo "No warning will appear, sliently cause security flaw"
+	@echo
+	$(CC) $(CFLAGS) -c foo-v1.c -o foo-v1.o
+	$(CC) $(CFLAGS) -c bar-v1.c -o bar-v1.o
+	@echo
+	$(CC) foo-v1.o bar-v1.o -o $@
+
+prog-v2:
+	@echo
+	@echo
+	@echo "### Compiling version 2###"
+	@echo "Place G_GNUC_PRINTF in 'foo.h' instead"
+	@echo "Show warning for missing arguments"
+	@echo
+	$(CC) $(CFLAGS) -c foo-v2.c -o foo-v2.o
+	$(CC) $(CFLAGS) -c bar-v2.c -o bar-v2.o
+	@echo
+	$(CC) foo-v2.o bar-v2.o -o $@
+
+clean:
+	@echo "### Clean all artifacts ###"
+	rm -f $(TARGET) $(OBJS)
-- 
2.49.0

Re: [PATCH 1/2] fsdev/9p-marshal: move G_GNUC_PRINTF to header

[Philippe Mathieu-Daudé]

On 14/6/25 04:08, Sean Wei wrote:
> v9fs_string_sprintf() is annotated with G_GNUC_PRINTF(2, 3) in
> 9p-marshal.c, but the prototype in fsdev/9p-marshal.h is missing the
> attribute, so callers that include only the header do not get format
> checking.
> 
> Move the annotation to the header and delete the duplicate in the
> source file. No behavior change.
> 
> Signed-off-by: Sean Wei <me@sean.taipei>
> ---
>   fsdev/9p-marshal.c | 3 +--
>   fsdev/9p-marshal.h | 2 +-
>   2 files changed, 2 insertions(+), 3 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 2/2] hw/9pfs: move G_GNUC_PRINTF to header

[Philippe Mathieu-Daudé]

On 14/6/25 04:09, Sean Wei wrote:
> v9fs_path_sprintf() is annotated with G_GNUC_PRINTF(2, 3) in
> hw/9pfs/9p.c, but the prototype in hw/9pfs/9p.h is missing the
> attribute, so callers that include only the header do not get format
> checking.
> 
> Move the annotation to the header and delete the duplicate in the
> source file. No behavior change.
> 
> Signed-off-by: Sean Wei <me@sean.taipei>
> ---
>   hw/9pfs/9p.c | 3 +--
>   hw/9pfs/9p.h | 2 +-
>   2 files changed, 2 insertions(+), 3 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 0/2] virtio-9p: move G_GNUC_PRINTF to header

[Christian Schoenebeck]

On Saturday, June 14, 2025 4:07:40 AM CEST Sean Wei wrote:
> v9fs_string_sprintf() and v9fs_path_sprintf() already have
> G_GNUC_PRINTF annotations in their own *.c files, but the
> prototypes in the corresponding headers lack them.  When another
> translation unit includes only the header, -Wformat can no longer
> validate the argument list.
> 
> This series relocates the annotations to fsdev/9p-marshal.h and
> hw/9pfs/9p.h, then drops the now-redundant annotations in
> the *.c files.  There is no functional change.
> 
> I've checked all call sites for these two helper function, all
> of them already passes the correct number of arguments.
> 
> A minimal PoC (sent as the next mail in the thread) demo how
> G_GNUC_PRINTF behaves differently when the attribute is present
> only in code.c or code.h file.
> 
> --
> 
> Sean Wei (3):
>   fsdev/9p-marshal: move G_GNUC_PRINTF to header
>   hw/9pfs: move G_GNUC_PRINTF to header
> 
>  fsdev/9p-marshal.c |  3 +--
>  fsdev/9p-marshal.h |  2 +-
>  hw/9pfs/9p.c       |  3 +--
>  hw/9pfs/9p.h       |  2 +-
>  4 files changed, 4 insertions(+), 6 deletions(-)
> 
>

With code style fix queued on 9p.next:
https://github.com/cschoenebeck/qemu/commits/9p.next

Please run scripts/checkpatch.pl next time.

Thanks!

/Christian

Re: [PATCH 0/2] virtio-9p: move G_GNUC_PRINTF to header

[Sean Wei]

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

Hi Christian,

On 2025/6/20 10:17 AM, Christian Schoenebeck wrote:
> On Saturday, June 14, 2025 4:07:40 AM CEST Sean Wei wrote:
>> v9fs_string_sprintf() and v9fs_path_sprintf() already have
>> G_GNUC_PRINTF annotations in their own *.c files, but the
>> prototypes in the corresponding headers lack them.  When another
>> translation unit includes only the header, -Wformat can no longer
>> validate the argument list.
>>
>> This series relocates the annotations to fsdev/9p-marshal.h and
>> hw/9pfs/9p.h, then drops the now-redundant annotations in
>> the *.c files.  There is no functional change.
>>
>> I've checked all call sites for these two helper function, all
>> of them already passes the correct number of arguments.
>>
>> A minimal PoC (sent as the next mail in the thread) demo how
>> G_GNUC_PRINTF behaves differently when the attribute is present
>> only in code.c or code.h file.
>>
>> --
>>
>> Sean Wei (3):
>>    fsdev/9p-marshal: move G_GNUC_PRINTF to header
>>    hw/9pfs: move G_GNUC_PRINTF to header
>>
>>   fsdev/9p-marshal.c |  3 +--
>>   fsdev/9p-marshal.h |  2 +-
>>   hw/9pfs/9p.c       |  3 +--
>>   hw/9pfs/9p.h       |  2 +-
>>   4 files changed, 4 insertions(+), 6 deletions(-)
>>
>>
> 
> With code style fix queued on 9p.next:
> https://github.com/cschoenebeck/qemu/commits/9p.next
> 
> Please run scripts/checkpatch.pl next time.
> 
> Thanks!
> 
> /Christian
> 

Apologies for missing the check-patch step, I’ll make sure to run 
scripts/checkpatch.pl next time.

Thank you for fixing the code style and queuing the patch!

All the best,
Sean Wei

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4894 bytes --]