Re: [PATCH] net/tap-win32: Fix gcc 14 format truncation errors

[Peter Maydell <peter.maydell@linaro.org>]

On Mon, 7 Oct 2024 at 11:14, Bernhard Beschow <shentey@gmail.com> wrote:
>
> The patch fixes the following errors generated by GCC 14.2:
>
> ../src/net/tap-win32.c:343:19: error: '%s' directive output may be truncated writing up to 255 bytes into a region of size 176 [-Werror=format-truncation=]
>   343 |              "%s\\%s\\Connection",
>       |                   ^~
>   344 |              NETWORK_CONNECTIONS_KEY, enum_name);
>       |                                       ~~~~~~~~~
>
> ../src/net/tap-win32.c:341:9: note: 'snprintf' output between 92 and 347 bytes into a destination of size 256
>   341 |         snprintf(connection_string,
>       |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~
>   342 |              sizeof(connection_string),
>       |              ~~~~~~~~~~~~~~~~~~~~~~~~~~
>   343 |              "%s\\%s\\Connection",
>       |              ~~~~~~~~~~~~~~~~~~~~~
>   344 |              NETWORK_CONNECTIONS_KEY, enum_name);
>       |              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> ../src/net/tap-win32.c:242:58: error: '%s' directive output may be truncated writing up to 255 bytes into a region of size 178 [-Werror=format-truncation=]
>   242 |         snprintf (unit_string, sizeof(unit_string), "%s\\%s",
>       |                                                          ^~
>   243 |                   ADAPTER_KEY, enum_name);
>       |                                ~~~~~~~~~
>
> ../src/net/tap-win32.c:242:9: note: 'snprintf' output between 79 and 334 bytes into a destination of size 256
>   242 |         snprintf (unit_string, sizeof(unit_string), "%s\\%s",
>       |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   243 |                   ADAPTER_KEY, enum_name);
>       |                   ~~~~~~~~~~~~~~~~~~~~~~~
>
> ../src/net/tap-win32.c:620:52: error: '%s' directive output may be truncated writing up to 255 bytes into a region of size 245 [-Werror=format-truncation=]
>   620 |     snprintf (device_path, sizeof(device_path), "%s%s%s",
>       |                                                    ^~
>   621 |               USERMODEDEVICEDIR,
>   622 |               device_guid,
>       |               ~~~~~~~~~~~
> ../src/net/tap-win32.c:620:5: note: 'snprintf' output between 16 and 271 bytes into a destination of size 256
>   620 |     snprintf (device_path, sizeof(device_path), "%s%s%s",
>       |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   621 |               USERMODEDEVICEDIR,
>       |               ~~~~~~~~~~~~~~~~~~
>   622 |               device_guid,
>       |               ~~~~~~~~~~~~
>   623 |               TAPSUFFIX);
>       |               ~~~~~~~~~~
>
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2607
Probably also worth
Cc: qemu-stable@nongnu.org

> ---
>  net/tap-win32.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/net/tap-win32.c b/net/tap-win32.c
> index 7edbd71633..4a4625af2b 100644
> --- a/net/tap-win32.c
> +++ b/net/tap-win32.c
> @@ -214,7 +214,7 @@ static int is_tap_win32_dev(const char *guid)
>
>      for (;;) {
>          char enum_name[256];
> -        char unit_string[256];
> +        char unit_string[512];
>          HKEY unit_key;
>          char component_id_string[] = "ComponentId";
>          char component_id[256];
> @@ -315,7 +315,7 @@ static int get_device_guid(
>      while (!stop)
>      {
>          char enum_name[256];
> -        char connection_string[256];
> +        char connection_string[512];
>          HKEY connection_key;
>          char name_data[256];
>          DWORD name_type;
> @@ -595,7 +595,7 @@ static void tap_win32_free_buffer(tap_win32_overlapped_t *overlapped,
>  static int tap_win32_open(tap_win32_overlapped_t **phandle,
>                            const char *preferred_name)
>  {
> -    char device_path[256];
> +    char device_path[512];
>      char device_guid[0x100];
>      int rc;
>      HANDLE handle;

Rather than just increasing the array sizes, I think we
should use g_autofree and g_strdup_printf(), like:

       g_autofree char* unit_string = NULL;

       [...]
       unit_string = g_strdup_printf("%s\\%s", ADAPTER_KEY, enum_name);

       (then no need for an explicit free)

All this only happens once at open, so we can certainly
happily take the cost of memory allocation, and it saves
us wondering about whether there's actually a maximum
limit on these string values. (Looking at the MS documentation,
I think registry keys have a limit of 255 chars, but
values are 16383 chars, so 512 would be more than needed
for a key and less than the theoretical maximum for a value.)

thanks
-- PMM