diff mbox series

tap-win32: fix format-truncation warning

Message ID 20241023183009.1041419-1-pierrick.bouvier@linaro.org
State New
Headers show
Series tap-win32: fix format-truncation warning | expand

Commit Message

Pierrick Bouvier Oct. 23, 2024, 6:30 p.m. UTC
Simply increase destination buffer size so truncation can't happen.

"cc" "-m64" "-Ilibcommon.a.p" "-Isubprojects/dtc/libfdt" "-I../subprojects/dtc/libfdt" "-ID:/a/_temp/msys64/mingw64/include/pixman-1" "-ID:/a/_temp/msys64/mingw64/include/glib-2.0" "-ID:/a/_temp/msys64/mingw64/lib/glib-2.0/include" "-ID:/a/_temp/msys64/mingw64/include/ncursesw" "-fdiagnostics-color=auto" "-Wall" "-Winvalid-pch" "-Werror" "-std=gnu11" "-O2" "-g" "-fstack-protector-strong" "-Wempty-body" "-Wendif-labels" "-Wexpansion-to-defined" "-Wformat-security" "-Wformat-y2k" "-Wignored-qualifiers" "-Wimplicit-fallthrough=2" "-Winit-self" "-Wmissing-format-attribute" "-Wmissing-prototypes" "-Wnested-externs" "-Wold-style-declaration" "-Wold-style-definition" "-Wredundant-decls" "-Wshadow=local" "-Wstrict-prototypes" "-Wtype-limits" "-Wundef" "-Wvla" "-Wwrite-strings" "-Wno-missing-include-dirs" "-Wno-psabi" "-Wno-shift-negative-value" "-iquote" "." "-iquote" "D:/a/qemu/qemu" "-iquote" "D:/a/qemu/qemu/include" "-iquote" "D:/a/qemu/qemu/host/include/x86_64" "-iquote" "D:/a/qemu/qemu/host/include/generic" "-iq
../net/tap-win32.c: In function 'tap_win32_open':
../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);
      |                                       ~~~~~~~~~
In function 'get_device_guid',
    inlined from 'tap_win32_open' at ../net/tap-win32.c:616:10:
../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);
      |              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../net/tap-win32.c: In function 'tap_win32_open':
../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);
      |                                ~~~~~~~~~
In function 'is_tap_win32_dev',
    inlined from 'get_device_guid' at ../net/tap-win32.c:368:21,
    inlined from 'tap_win32_open' at ../net/tap-win32.c:616:10:
../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);
      |                   ~~~~~~~~~~~~~~~~~~~~~~~
../net/tap-win32.c: In function 'tap_win32_open':
../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,
      |               ~~~~~~~~~~~
../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: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 net/tap-win32.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Alex Bennée Oct. 23, 2024, 7:50 p.m. UTC | #1
Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:

> Simply increase destination buffer size so truncation can't happen.
>
> "cc" "-m64" "-Ilibcommon.a.p" "-Isubprojects/dtc/libfdt"
> "-I../subprojects/dtc/libfdt"
> "-ID:/a/_temp/msys64/mingw64/include/pixman-1"
> "-ID:/a/_temp/msys64/mingw64/include/glib-2.0"
> "-ID:/a/_temp/msys64/mingw64/lib/glib-2.0/include"
> "-ID:/a/_temp/msys64/mingw64/include/ncursesw"
> "-fdiagnostics-color=auto" "-Wall" "-Winvalid-pch" "-Werror"
> "-std=gnu11" "-O2" "-g" "-fstack-protector-strong" "-Wempty-body"
> "-Wendif-labels" "-Wexpansion-to-defined" "-Wformat-security"
> "-Wformat-y2k" "-Wignored-qualifiers" "-Wimplicit-fallthrough=2"
> "-Winit-self" "-Wmissing-format-attribute" "-Wmissing-prototypes"
> "-Wnested-externs" "-Wold-style-declaration" "-Wold-style-definition"
> "-Wredundant-decls" "-Wshadow=local" "-Wstrict-prototypes"
> "-Wtype-limits" "-Wundef" "-Wvla" "-Wwrite-strings"
> "-Wno-missing-include-dirs" "-Wno-psabi" "-Wno-shift-negative-value"
> "-iquote" "." "-iquote" "D:/a/qemu/qemu" "-iquote"
> "D:/a/qemu/qemu/include" "-iquote"
> "D:/a/qemu/qemu/host/include/x86_64" "-iquote"
> "D:/a/qemu/qemu/host/include/generic" "-iq
> ../net/tap-win32.c: In function 'tap_win32_open':
> ../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);
>       |                                       ~~~~~~~~~
> In function 'get_device_guid',
>     inlined from 'tap_win32_open' at ../net/tap-win32.c:616:10:
> ../net/tap-win32.c:341:9: note: 'snprintf' output between 92 and 347
> bytes into a destination of size 256

Is the compiler min/max maxing what UCS-16 or UTF-8 might pack into that string?
>
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.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 7edbd716337..4a4625af2b2 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];

If this isn't perf sensitive code lets just get rid of this stack
allocation and be done with some autofree'd g_strdup_printfs?
Pierrick Bouvier Oct. 23, 2024, 8:15 p.m. UTC | #2
On 10/23/24 12:50, Alex Bennée wrote:
> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
> 
>> Simply increase destination buffer size so truncation can't happen.
>>
>> "cc" "-m64" "-Ilibcommon.a.p" "-Isubprojects/dtc/libfdt"
>> "-I../subprojects/dtc/libfdt"
>> "-ID:/a/_temp/msys64/mingw64/include/pixman-1"
>> "-ID:/a/_temp/msys64/mingw64/include/glib-2.0"
>> "-ID:/a/_temp/msys64/mingw64/lib/glib-2.0/include"
>> "-ID:/a/_temp/msys64/mingw64/include/ncursesw"
>> "-fdiagnostics-color=auto" "-Wall" "-Winvalid-pch" "-Werror"
>> "-std=gnu11" "-O2" "-g" "-fstack-protector-strong" "-Wempty-body"
>> "-Wendif-labels" "-Wexpansion-to-defined" "-Wformat-security"
>> "-Wformat-y2k" "-Wignored-qualifiers" "-Wimplicit-fallthrough=2"
>> "-Winit-self" "-Wmissing-format-attribute" "-Wmissing-prototypes"
>> "-Wnested-externs" "-Wold-style-declaration" "-Wold-style-definition"
>> "-Wredundant-decls" "-Wshadow=local" "-Wstrict-prototypes"
>> "-Wtype-limits" "-Wundef" "-Wvla" "-Wwrite-strings"
>> "-Wno-missing-include-dirs" "-Wno-psabi" "-Wno-shift-negative-value"
>> "-iquote" "." "-iquote" "D:/a/qemu/qemu" "-iquote"
>> "D:/a/qemu/qemu/include" "-iquote"
>> "D:/a/qemu/qemu/host/include/x86_64" "-iquote"
>> "D:/a/qemu/qemu/host/include/generic" "-iq
>> ../net/tap-win32.c: In function 'tap_win32_open':
>> ../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);
>>        |                                       ~~~~~~~~~
>> In function 'get_device_guid',
>>      inlined from 'tap_win32_open' at ../net/tap-win32.c:616:10:
>> ../net/tap-win32.c:341:9: note: 'snprintf' output between 92 and 347
>> bytes into a destination of size 256
> 
> Is the compiler min/max maxing what UCS-16 or UTF-8 might pack into that string?
>>

Yes, enum_name (used to compose final string) is already sized 256, so 
result *may* be bigger. I'm not sure it would happen in the real world 
though.

The result strings are used to access registry keys, which have a max 
size of 255. And device_path is used to open a file, which is limited in 
size too.
https://learn.microsoft.com/en-us/windows/win32/sysinfo/registry-element-size-limits

>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.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 7edbd716337..4a4625af2b2 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];
> 
> If this isn't perf sensitive code lets just get rid of this stack
> allocation and be done with some autofree'd g_strdup_printfs?
> 

Yes, will do that.
All this is used only when opening the device, so it's not on any hot path.

Thanks,
Pierrick
Richard Henderson Oct. 23, 2024, 9:30 p.m. UTC | #3
On 10/23/24 13:15, Pierrick Bouvier wrote:
> On 10/23/24 12:50, Alex Bennée wrote:
>> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
>>
>>> Simply increase destination buffer size so truncation can't happen.
>>>
>>> "cc" "-m64" "-Ilibcommon.a.p" "-Isubprojects/dtc/libfdt"
>>> "-I../subprojects/dtc/libfdt"
>>> "-ID:/a/_temp/msys64/mingw64/include/pixman-1"
>>> "-ID:/a/_temp/msys64/mingw64/include/glib-2.0"
>>> "-ID:/a/_temp/msys64/mingw64/lib/glib-2.0/include"
>>> "-ID:/a/_temp/msys64/mingw64/include/ncursesw"
>>> "-fdiagnostics-color=auto" "-Wall" "-Winvalid-pch" "-Werror"
>>> "-std=gnu11" "-O2" "-g" "-fstack-protector-strong" "-Wempty-body"
>>> "-Wendif-labels" "-Wexpansion-to-defined" "-Wformat-security"
>>> "-Wformat-y2k" "-Wignored-qualifiers" "-Wimplicit-fallthrough=2"
>>> "-Winit-self" "-Wmissing-format-attribute" "-Wmissing-prototypes"
>>> "-Wnested-externs" "-Wold-style-declaration" "-Wold-style-definition"
>>> "-Wredundant-decls" "-Wshadow=local" "-Wstrict-prototypes"
>>> "-Wtype-limits" "-Wundef" "-Wvla" "-Wwrite-strings"
>>> "-Wno-missing-include-dirs" "-Wno-psabi" "-Wno-shift-negative-value"
>>> "-iquote" "." "-iquote" "D:/a/qemu/qemu" "-iquote"
>>> "D:/a/qemu/qemu/include" "-iquote"
>>> "D:/a/qemu/qemu/host/include/x86_64" "-iquote"
>>> "D:/a/qemu/qemu/host/include/generic" "-iq
>>> ../net/tap-win32.c: In function 'tap_win32_open':
>>> ../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);
>>>        |                                       ~~~~~~~~~
>>> In function 'get_device_guid',
>>>      inlined from 'tap_win32_open' at ../net/tap-win32.c:616:10:
>>> ../net/tap-win32.c:341:9: note: 'snprintf' output between 92 and 347
>>> bytes into a destination of size 256
>>
>> Is the compiler min/max maxing what UCS-16 or UTF-8 might pack into that string?
>>>
> 
> Yes, enum_name (used to compose final string) is already sized 256, so result *may* be 
> bigger. I'm not sure it would happen in the real world though.

There are several patches for this, most recently:

https://lore.kernel.org/qemu-devel/20241008202842.4478-1-shentey@gmail.com/


r~
Pierrick Bouvier Oct. 23, 2024, 9:43 p.m. UTC | #4
On 10/23/24 14:30, Richard Henderson wrote:
> On 10/23/24 13:15, Pierrick Bouvier wrote:
>> On 10/23/24 12:50, Alex Bennée wrote:
>>> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
>>>
>>>> Simply increase destination buffer size so truncation can't happen.
>>>>
>>>> "cc" "-m64" "-Ilibcommon.a.p" "-Isubprojects/dtc/libfdt"
>>>> "-I../subprojects/dtc/libfdt"
>>>> "-ID:/a/_temp/msys64/mingw64/include/pixman-1"
>>>> "-ID:/a/_temp/msys64/mingw64/include/glib-2.0"
>>>> "-ID:/a/_temp/msys64/mingw64/lib/glib-2.0/include"
>>>> "-ID:/a/_temp/msys64/mingw64/include/ncursesw"
>>>> "-fdiagnostics-color=auto" "-Wall" "-Winvalid-pch" "-Werror"
>>>> "-std=gnu11" "-O2" "-g" "-fstack-protector-strong" "-Wempty-body"
>>>> "-Wendif-labels" "-Wexpansion-to-defined" "-Wformat-security"
>>>> "-Wformat-y2k" "-Wignored-qualifiers" "-Wimplicit-fallthrough=2"
>>>> "-Winit-self" "-Wmissing-format-attribute" "-Wmissing-prototypes"
>>>> "-Wnested-externs" "-Wold-style-declaration" "-Wold-style-definition"
>>>> "-Wredundant-decls" "-Wshadow=local" "-Wstrict-prototypes"
>>>> "-Wtype-limits" "-Wundef" "-Wvla" "-Wwrite-strings"
>>>> "-Wno-missing-include-dirs" "-Wno-psabi" "-Wno-shift-negative-value"
>>>> "-iquote" "." "-iquote" "D:/a/qemu/qemu" "-iquote"
>>>> "D:/a/qemu/qemu/include" "-iquote"
>>>> "D:/a/qemu/qemu/host/include/x86_64" "-iquote"
>>>> "D:/a/qemu/qemu/host/include/generic" "-iq
>>>> ../net/tap-win32.c: In function 'tap_win32_open':
>>>> ../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);
>>>>         |                                       ~~~~~~~~~
>>>> In function 'get_device_guid',
>>>>       inlined from 'tap_win32_open' at ../net/tap-win32.c:616:10:
>>>> ../net/tap-win32.c:341:9: note: 'snprintf' output between 92 and 347
>>>> bytes into a destination of size 256
>>>
>>> Is the compiler min/max maxing what UCS-16 or UTF-8 might pack into that string?
>>>>
>>
>> Yes, enum_name (used to compose final string) is already sized 256, so result *may* be
>> bigger. I'm not sure it would happen in the real world though.
> 
> There are several patches for this, most recently:
> 
> https://lore.kernel.org/qemu-devel/20241008202842.4478-1-shentey@gmail.com/
> 
> 
> r~

Thanks for pointing it Richard.
The linked patch is better than current approach, so it should be favored.

Regards,
Pierrick
diff mbox series

Patch

diff --git a/net/tap-win32.c b/net/tap-win32.c
index 7edbd716337..4a4625af2b2 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;