[PATCH] kernelbase: Handle 2 full paths in PathAllocCombine.

[Zhiyi Zhang]

On 8/20/21 5:58 AM, Daniel Lehman wrote:
> 0001-kernelbase-Handle-2-full-paths-in-PathAllocCombine.txt
>
> From 4da1c096d591b93c3ac44c036293136d2cca5277 Mon Sep 17 00:00:00 2001
> From: Daniel Lehman <dlehman(a)esri.com>
> Date: Thu, 19 Aug 2021 09:35:11 -0700
> Subject: [PATCH] kernelbase: Handle 2 full paths in PathAllocCombine.
>
> Signed-off-by: Daniel Lehman <dlehman(a)esri.com>
> ---
>  dlls/kernelbase/path.c       | 7 ++++---
>  dlls/kernelbase/tests/path.c | 1 +
>  2 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/dlls/kernelbase/path.c b/dlls/kernelbase/path.c
> index 9387705e963..92f85f7b29b 100644
> --- a/dlls/kernelbase/path.c
> +++ b/dlls/kernelbase/path.c
> @@ -401,7 +401,7 @@ HRESULT WINAPI PathAllocCombine(const WCHAR *path1, const WCHAR *path2, DWORD fl
>  {
>      SIZE_T combined_length, length2;
>      WCHAR *combined_path;
> -    BOOL from_path2 = FALSE;
> +    BOOL drive_only = FALSE;
>      HRESULT hr;
>
> TRACE("%s %s %#x %p\n", wine_dbgstr_w(path1), wine_dbgstr_w(path2), flags, out);
> @@ -419,7 +419,8 @@ HRESULT WINAPI PathAllocCombine(const WCHAR *path1, const WCHAR *path2, DWORD fl
>      {
>          path1 = path2;
>          path2 = NULL;
> -        from_path2 = TRUE;
> +        drive_only = (is_drive_spec( path1 ) && !path1[2]) ||
> +                     (is_prefixed_disk( path1 ) && !path1[6]);
>      }
>
> length2 = path2 ? lstrlenW(path2) : 0;
> @@ -435,7 +436,7 @@ HRESULT WINAPI PathAllocCombine(const WCHAR *path1, const WCHAR *path2, DWORD fl
>
> lstrcpyW(combined_path, path1);
>      PathCchStripPrefix(combined_path, combined_length);
> -    if (from_path2) PathCchAddBackslashEx(combined_path, combined_length, NULL, NULL);
> +    if (drive_only) PathCchAddBackslashEx(combined_path, combined_length, NULL, NULL);

Hi Daniel,

Thanks for the patch.

I noticed that the subject is misleading because PathAllocCombine() already handles two full paths.
For example, "Don't add a backslash to the second path if it's not needed" sounds more appropriate.
Then you can change "from_path2" to "add_backslash".
> if (path2 && path2[0])
>      {
> diff --git a/dlls/kernelbase/tests/path.c b/dlls/kernelbase/tests/path.c
> index 8d85542612f..bdd68fa9c0a 100644
> --- a/dlls/kernelbase/tests/path.c
> +++ b/dlls/kernelbase/tests/path.c
> @@ -439,6 +439,7 @@ static const struct combine_test combine_tests[] =
>      {"C:\\a", "",      "C:\\a" },
>      {"\\",    "a",     "\\a"},
>      {"\\a",   "b",     "\\a\\b" },
> +    {"C:\\a", "C:\\b", "C:\\b" },

Please test more path types without a backslash and add them to "/* Second path is fully qualified */" section in combine_tests, e.g.,
driver specs(X:), UNC paths, NT paths, NT UNC paths, etc. And put the test in a separate patch.

Thanks,
Zhiyi
> /* normal UNC paths */
>      {"\\\\192.168.1.1\\test", "a",  "\\\\192.168.1.1\\test\\a" },

[Marvin]

Hi,

While running your changed tests, I think I found new failures.
Being a bot and all I'm not very good at pattern recognition, so I might be
wrong, but could you please double-check?

Full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=95972

Your paranoid android.


=== build (build log) ===

error: patch failed: dlls/kernelbase/path.c:435
error: patch failed: dlls/kernelbase/tests/path.c:439
Task: Patch failed to apply

=== debiant2 (build log) ===

error: patch failed: dlls/kernelbase/path.c:435
error: patch failed: dlls/kernelbase/tests/path.c:439
Task: Patch failed to apply

=== debiant2 (build log) ===

error: patch failed: dlls/kernelbase/path.c:435
error: patch failed: dlls/kernelbase/tests/path.c:439
Task: Patch failed to apply