Re: [PATCH] shell32/tests: Increase 6 buffers to avoid potential overflows via sprintf (GCC).

[Gerald Pfeifer]

This patch of mine in 2018 received a note from Nikolay

"I think we should try instead to use more predictable paths. This 
  change scatters magic increments, that looks worse than a warning 
  in my opinion."

as it's only response.  It was not applied.

Nearly to the day two years later pretty exactly the same patch got
applied, from a different author:

commit d1373e8aae1b15b96e847488e4b6617789f8fb62
  Author: Rémi Bernon 
  Date:   Tue Feb 11 19:09:30 2020 +0100

shell32/tests: Fix some format-overflow warnings.

Signed-off-by: Rémi Bernon 
    Signed-off-by: Alexandre Julliard

This is the second such case I learned about this week.

Gerald

On Sun, 18 Feb 2018, Gerald Pfeifer wrote:
> This sheds off 10 warnings from builds with current versions of GCC.
>
> The pattern is printing a pathname plus some additional text into a 
> buffer the size of a pathname.
>
> Gerald
> Signed-off-by: Gerald Pfeifer
>
> dlls/shell32/tests/shlexec.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/dlls/shell32/tests/shlexec.c b/dlls/shell32/tests/shlexec.c
> index 9db12ce42a..4f00db44e6 100644
> --- a/dlls/shell32/tests/shlexec.c
> +++ b/dlls/shell32/tests/shlexec.c
> @@ -1654,7 +1654,7 @@ static void test_argify(void)
>
> static void test_filename(void)
>  {
> - char filename[MAX_PATH];
> - char filename[MAX_PATH+16];
> const filename_tests_t* test;
> char* c;
> INT_PTR rc;
>
> @@ -1920,7 +1920,7 @@ static void test_fileurls(void)
>
> static void test_urls(void)
>  {
> - char url[MAX_PATH];
> -
>
> char url[MAX_PATH+15];
> INT_PTR rc;
>
> if (!create_test_class("fakeproto", FALSE))
>
> @@ -2015,7 +2015,7 @@ static void test_urls(void)
>  static void test_find_executable(void)
>  {
>      char notepad_path[MAX_PATH];
> - char filename[MAX_PATH];
> - char filename[MAX_PATH+17];
> char command[MAX_PATH];
> const filename_tests_t* test;
> INT_PTR rc;
>
> @@ -2433,7 +2433,7 @@ static void hook_WaitForInputIdle(DWORD (WINAPI *new_func)(HANDLE, DWORD))
>
> static void test_dde(void)
>  {
> - char filename[MAX_PATH], defApplication[MAX_PATH];
> - char filename[MAX_PATH+14], defApplication[MAX_PATH];
> const dde_tests_t* test;
> char params[1024];
> INT_PTR rc;
>
> @@ -2600,7 +2600,7 @@ static DWORD CALLBACK ddeThread(LPVOID arg)
>
> static void test_dde_default_app(void)
>  {
> - char filename[MAX_PATH];
> - char filename[MAX_PATH+15];
> HSZ hszApplication;
> dde_thread_info_t info = { filename, GetCurrentThreadId() };
> const dde_default_app_tests_t* test;
>
> @@ -2702,7 +2702,7 @@ static void init_test(void)
>  {
>      HMODULE hdll;
>      HRESULT (WINAPI *pDllGetVersion)(DLLVERSIONINFO*);
> - char filename[MAX_PATH];
> - char filename[MAX_PATH+26];
> WCHAR lnkfile[MAX_PATH];
> char params[1024];
> const char* const * testfile;

[PATCH] shell32/tests: Increase 6 buffers to avoid potential overflows via sprintf (GCC).

[Michael Stefaniuc]

Wow! Only 2 years later?
Between my first attempt of introducing ARRAY_SIZE() with an "I'm not convinced"
reply and my standardization of the different ARRAY_SIZE() that others
introduced it took 5+ years.

Wine is just not for visionaries...
/me laughs

You seem to have an interest in compiler development and some of the advanced
warnings turn out to be not that helpful. So there is a reluctance to add
workarounds for those. Once they make it even to Debian then they became a
problem so a fix is more likely to get in even though it is "ugly".

bye
    michael

On 11/22/20 5:29 PM, Gerald Pfeifer wrote:
> This patch of mine in 2018 received a note from Nikolay
>
> "I think we should try instead to use more predictable paths. This 
>   change scatters magic increments, that looks worse than a warning 
>   in my opinion."
>
> as it's only response.  It was not applied.
>
> Nearly to the day two years later pretty exactly the same patch got
> applied, from a different author: