| Message ID | 20241023183009.1041419-1-pierrick.bouvier@linaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | tap-win32: fix format-truncation warning | expand |
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?
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
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~
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 --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;
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(-)