[PATCH] kernel32: CreateDirectory shouldn't return ERROR_ACCESS_DENIED for the root of the drive.

[Dmitry Timoshkov]

According to the testbot results CreateDirectory("C:\\", NULL) fails
with ERROR_ACCESS_DENIED for not administrators. However with UAC enabled
and not and administrator account I get ERROR_ALREADY_EXISTS in that case
with Windows 7 64-bit running on real hardware. Moreover, Wine doesn't
really perform any access checks in that case and blindly assumes that
returning STATUS_ACCESS_DENIED is correct behaviour for the drive's root:
dlls/ntdll/directory.c,lookup_unix_name().

This patch fixes an application that can't find its data files because
after it receives ERROR_ACCESS_DENIED it stops further directory traversing.

Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru>
---
 dlls/kernel32/tests/directory.c | 43 +++++++++++++++++++++++++++++++++
 dlls/ntdll/directory.c          |  3 +--
 2 files changed, 44 insertions(+), 2 deletions(-)

diff --git a/dlls/kernel32/tests/directory.c b/dlls/kernel32/tests/directory.c
index 512dc6d22a..de78c61681 100644
--- a/dlls/kernel32/tests/directory.c
+++ b/dlls/kernel32/tests/directory.c
@@ -541,10 +541,53 @@ static void test_SetCurrentDirectoryA(void)
     ok( GetLastError() == ERROR_PATH_NOT_FOUND, "wrong error %d\n", GetLastError() );
 }
 
+static void test_CreateDirectory_root(void)
+{
+    static const WCHAR drive_c_root[] = { 'C',':','\\',0 };
+    static const WCHAR drive_c[] = { 'C',':',0 };
+    char curdir[MAX_PATH];
+    BOOL ret;
+
+    GetCurrentDirectoryA(sizeof(curdir), curdir);
+
+    ret = SetCurrentDirectoryA("C:\\");
+    ok(ret, "SetCurrentDirectory error %u\n", GetLastError());
+
+    SetLastError(0xdeadbeef);
+    ret = CreateDirectoryA("C:\\", NULL);
+    ok(!ret, "CreateDirectory should fail\n");
+    if (GetLastError() == ERROR_ACCESS_DENIED)
+    {
+        win_skip("not an administrator\n");
+        SetCurrentDirectoryA(curdir);
+        return;
+    }
+    ok(GetLastError() == ERROR_ALREADY_EXISTS, "got %u\n", GetLastError());
+
+    SetLastError(0xdeadbeef);
+    ret = CreateDirectoryA("C:", NULL);
+    ok(!ret, "CreateDirectory should fail\n");
+    ok(GetLastError() == ERROR_ALREADY_EXISTS, "got %u\n", GetLastError());
+
+    SetLastError(0xdeadbeef);
+    ret = CreateDirectoryW(drive_c_root, NULL);
+    ok(!ret, "CreateDirectory should fail\n");
+    ok(GetLastError() == ERROR_ALREADY_EXISTS, "got %u\n", GetLastError());
+
+    SetLastError(0xdeadbeef);
+    ret = CreateDirectoryW(drive_c, NULL);
+    ok(!ret, "CreateDirectory should fail\n");
+    ok(GetLastError() == ERROR_ALREADY_EXISTS, "got %u\n", GetLastError());
+
+    SetCurrentDirectoryA(curdir);
+}
+
 START_TEST(directory)
 {
     init();
 
+    test_CreateDirectory_root();
+
     test_GetWindowsDirectoryA();
     test_GetWindowsDirectoryW();
 
diff --git a/dlls/ntdll/directory.c b/dlls/ntdll/directory.c
index c48b9e97ea..2719958f67 100644
--- a/dlls/ntdll/directory.c
+++ b/dlls/ntdll/directory.c
@@ -2650,9 +2650,8 @@ static NTSTATUS lookup_unix_name( const WCHAR *name, int name_len, char **buffer
         {
             if (!stat( unix_name, &st ))
             {
-                /* creation fails with STATUS_ACCESS_DENIED for the root of the drive */
                 if (disposition == FILE_CREATE)
-                    return name_len ? STATUS_OBJECT_NAME_COLLISION : STATUS_ACCESS_DENIED;
+                    return STATUS_OBJECT_NAME_COLLISION;
                 return STATUS_SUCCESS;
             }
         }
-- 
2.19.2

[Alexandre Julliard]

Dmitry Timoshkov <dmitry(a)baikal.ru> writes:
> According to the testbot results CreateDirectory("C:\\", NULL) fails
> with ERROR_ACCESS_DENIED for not administrators. However with UAC enabled
> and not and administrator account I get ERROR_ALREADY_EXISTS in that case
> with Windows 7 64-bit running on real hardware. Moreover, Wine doesn't
> really perform any access checks in that case and blindly assumes that
> returning STATUS_ACCESS_DENIED is correct behaviour for the drive's root:
> dlls/ntdll/directory.c,lookup_unix_name().
>
> This patch fixes an application that can't find its data files because
> after it receives ERROR_ACCESS_DENIED it stops further directory traversing.

What app is that? Does it fail on Windows when not administrator?

-- 
Alexandre Julliard
julliard(a)winehq.org

[Dmitry Timoshkov]

Alexandre Julliard <julliard(a)winehq.org> wrote:
> > According to the testbot results CreateDirectory("C:\\", NULL) fails
> > with ERROR_ACCESS_DENIED for not administrators. However with UAC enabled
> > and not and administrator account I get ERROR_ALREADY_EXISTS in that case
> > with Windows 7 64-bit running on real hardware. Moreover, Wine doesn't
> > really perform any access checks in that case and blindly assumes that
> > returning STATUS_ACCESS_DENIED is correct behaviour for the drive's root:
> > dlls/ntdll/directory.c,lookup_unix_name().
> >
> > This patch fixes an application that can't find its data files because
> > after it receives ERROR_ACCESS_DENIED it stops further directory traversing.
>
> What app is that? Does it fail on Windows when not administrator?

The application doesn't fail on Windows, and as I mentioned above under
a not administrator account and UAC enabled I don't get ERROR_ACCESS_DENIED
error with the tests included in the patch.

-- 
Dmitry.

[Alexandre Julliard]

Dmitry Timoshkov <dmitry(a)baikal.ru> writes:
> Alexandre Julliard <julliard(a)winehq.org> wrote:
> > > According to the testbot results CreateDirectory("C:\\", NULL) fails
> > > with ERROR_ACCESS_DENIED for not administrators. However with UAC enabled
> > > and not and administrator account I get ERROR_ALREADY_EXISTS in that case
> > > with Windows 7 64-bit running on real hardware. Moreover, Wine doesn't
> > > really perform any access checks in that case and blindly assumes that
> > > returning STATUS_ACCESS_DENIED is correct behaviour for the drive's root:
> > > dlls/ntdll/directory.c,lookup_unix_name().
> > >
> > > This patch fixes an application that can't find its data files because
> > > after it receives ERROR_ACCESS_DENIED it stops further directory traversing.
> >
> > What app is that? Does it fail on Windows when not administrator?
>
> The application doesn't fail on Windows, and as I mentioned above under
> a not administrator account and UAC enabled I don't get ERROR_ACCESS_DENIED
> error with the tests included in the patch.

Your tests get ERROR_ACCESS_DENIED on every single testbot vm except
w8adm, so that's not very convincing... Is the app going to fail on all
these vms?

-- 
Alexandre Julliard
julliard(a)winehq.org

[Dmitry Timoshkov]

Alexandre Julliard <julliard(a)winehq.org> wrote:
> > > > According to the testbot results CreateDirectory("C:\\", NULL) fails
> > > > with ERROR_ACCESS_DENIED for not administrators. However with UAC enabled
> > > > and not and administrator account I get ERROR_ALREADY_EXISTS in that case
> > > > with Windows 7 64-bit running on real hardware. Moreover, Wine doesn't
> > > > really perform any access checks in that case and blindly assumes that
> > > > returning STATUS_ACCESS_DENIED is correct behaviour for the drive's root:
> > > > dlls/ntdll/directory.c,lookup_unix_name().
> > > >
> > > > This patch fixes an application that can't find its data files because
> > > > after it receives ERROR_ACCESS_DENIED it stops further directory traversing.
> > >
> > > What app is that? Does it fail on Windows when not administrator?
> >
> > The application doesn't fail on Windows, and as I mentioned above under
> > a not administrator account and UAC enabled I don't get ERROR_ACCESS_DENIED
> > error with the tests included in the patch.
>
> Your tests get ERROR_ACCESS_DENIED on every single testbot vm except
> w8adm, so that's not very convincing... Is the app going to fail on all
> these vms?

I'd guess if the CreateDirectory("C:\") returns ERROR_ACCESS_DENIED
then the app would fail. It's not clear how the VMs are configured
and why I don't get ERROR_ACCESS_DENIED on real hardware under a not
admin account, but it should be pretty obvious that since Wine doesn't
perform any real administrator access checks, and if it would the checks
should be done on the server side, ntdll checks shouldn't return access
denied error.

-- 
Dmitry.

[Alexandre Julliard]

Dmitry Timoshkov <dmitry(a)baikal.ru> writes:
> Alexandre Julliard <julliard(a)winehq.org> wrote:
> > > > > According to the testbot results CreateDirectory("C:\\", NULL) fails
> > > > > with ERROR_ACCESS_DENIED for not administrators. However with UAC enabled
> > > > > and not and administrator account I get ERROR_ALREADY_EXISTS in that case
> > > > > with Windows 7 64-bit running on real hardware. Moreover, Wine doesn't
> > > > > really perform any access checks in that case and blindly assumes that
> > > > > returning STATUS_ACCESS_DENIED is correct behaviour for the drive's root:
> > > > > dlls/ntdll/directory.c,lookup_unix_name().
> > > > >
> > > > > This patch fixes an application that can't find its data files because
> > > > > after it receives ERROR_ACCESS_DENIED it stops further directory traversing.
> > > >
> > > > What app is that? Does it fail on Windows when not administrator?
> > >
> > > The application doesn't fail on Windows, and as I mentioned above under
> > > a not administrator account and UAC enabled I don't get ERROR_ACCESS_DENIED
> > > error with the tests included in the patch.
> >
> > Your tests get ERROR_ACCESS_DENIED on every single testbot vm except
> > w8adm, so that's not very convincing... Is the app going to fail on all
> > these vms?
>
> I'd guess if the CreateDirectory("C:\") returns ERROR_ACCESS_DENIED
> then the app would fail. It's not clear how the VMs are configured
> and why I don't get ERROR_ACCESS_DENIED on real hardware under a not
> admin account, but it should be pretty obvious that since Wine doesn't
> perform any real administrator access checks, and if it would the checks
> should be done on the server side, ntdll checks shouldn't return access
> denied error.

It's also pretty obvious that this check was added for a reason, so it
would need a more convincing argument to remove it.

-- 
Alexandre Julliard
julliard(a)winehq.org

[Dmitry Timoshkov]

Alexandre Julliard <julliard(a)winehq.org> wrote:
> > > > > > According to the testbot results CreateDirectory("C:\\", NULL) fails
> > > > > > with ERROR_ACCESS_DENIED for not administrators. However with UAC enabled
> > > > > > and not and administrator account I get ERROR_ALREADY_EXISTS in that case
> > > > > > with Windows 7 64-bit running on real hardware. Moreover, Wine doesn't
> > > > > > really perform any access checks in that case and blindly assumes that
> > > > > > returning STATUS_ACCESS_DENIED is correct behaviour for the drive's root:
> > > > > > dlls/ntdll/directory.c,lookup_unix_name().
> > > > > >
> > > > > > This patch fixes an application that can't find its data files because
> > > > > > after it receives ERROR_ACCESS_DENIED it stops further directory traversing.
> > > > >
> > > > > What app is that? Does it fail on Windows when not administrator?
> > > >
> > > > The application doesn't fail on Windows, and as I mentioned above under
> > > > a not administrator account and UAC enabled I don't get ERROR_ACCESS_DENIED
> > > > error with the tests included in the patch.
> > >
> > > Your tests get ERROR_ACCESS_DENIED on every single testbot vm except
> > > w8adm, so that's not very convincing... Is the app going to fail on all
> > > these vms?
> >
> > I'd guess if the CreateDirectory("C:\") returns ERROR_ACCESS_DENIED
> > then the app would fail. It's not clear how the VMs are configured
> > and why I don't get ERROR_ACCESS_DENIED on real hardware under a not
> > admin account, but it should be pretty obvious that since Wine doesn't
> > perform any real administrator access checks, and if it would the checks
> > should be done on the server side, ntdll checks shouldn't return access
> > denied error.
>
> It's also pretty obvious that this check was added for a reason, so it
> would need a more convincing argument to remove it.

Do you recall the reason why that check was added? I can't find any
specific test case for this behaviour either.

-- 
Dmitry.

[Alexandre Julliard]

Dmitry Timoshkov <dmitry(a)baikal.ru> writes:
> Alexandre Julliard <julliard(a)winehq.org> wrote:
> > > > > > > According to the testbot results CreateDirectory("C:\\", NULL) fails
> > > > > > > with ERROR_ACCESS_DENIED for not administrators. However with UAC enabled
> > > > > > > and not and administrator account I get ERROR_ALREADY_EXISTS in that case
> > > > > > > with Windows 7 64-bit running on real hardware. Moreover, Wine doesn't
> > > > > > > really perform any access checks in that case and blindly assumes that
> > > > > > > returning STATUS_ACCESS_DENIED is correct behaviour for the drive's root:
> > > > > > > dlls/ntdll/directory.c,lookup_unix_name().
> > > > > > >
> > > > > > > This patch fixes an application that can't find its data files because
> > > > > > > after it receives ERROR_ACCESS_DENIED it stops further directory traversing.
> > > > > >
> > > > > > What app is that? Does it fail on Windows when not administrator?
> > > > >
> > > > > The application doesn't fail on Windows, and as I mentioned above under
> > > > > a not administrator account and UAC enabled I don't get ERROR_ACCESS_DENIED
> > > > > error with the tests included in the patch.
> > > >
> > > > Your tests get ERROR_ACCESS_DENIED on every single testbot vm except
> > > > w8adm, so that's not very convincing... Is the app going to fail on all
> > > > these vms?
> > >
> > > I'd guess if the CreateDirectory("C:\") returns ERROR_ACCESS_DENIED
> > > then the app would fail. It's not clear how the VMs are configured
> > > and why I don't get ERROR_ACCESS_DENIED on real hardware under a not
> > > admin account, but it should be pretty obvious that since Wine doesn't
> > > perform any real administrator access checks, and if it would the checks
> > > should be done on the server side, ntdll checks shouldn't return access
> > > denied error.
> >
> > It's also pretty obvious that this check was added for a reason, so it
> > would need a more convincing argument to remove it.
>
> Do you recall the reason why that check was added? I can't find any
> specific test case for this behaviour either.

As far as I can tell, you added it ;-)

https://source.winehq.org/git/wine.git/commit/d75aed2c92435e8ae4d5c260e31e81...

-- 
Alexandre Julliard
julliard(a)winehq.org

[Dmitry Timoshkov]

Alexandre Julliard <julliard(a)winehq.org> wrote:
> > > > > > > > According to the testbot results CreateDirectory("C:\\", NULL) fails
> > > > > > > > with ERROR_ACCESS_DENIED for not administrators. However with UAC enabled
> > > > > > > > and not and administrator account I get ERROR_ALREADY_EXISTS in that case
> > > > > > > > with Windows 7 64-bit running on real hardware. Moreover, Wine doesn't
> > > > > > > > really perform any access checks in that case and blindly assumes that
> > > > > > > > returning STATUS_ACCESS_DENIED is correct behaviour for the drive's root:
> > > > > > > > dlls/ntdll/directory.c,lookup_unix_name().
> > > > > > > >
> > > > > > > > This patch fixes an application that can't find its data files because
> > > > > > > > after it receives ERROR_ACCESS_DENIED it stops further directory traversing.
> > > > > > > 
> > > > > > > What app is that? Does it fail on Windows when not administrator?
> > > > > >
> > > > > > The application doesn't fail on Windows, and as I mentioned above under
> > > > > > a not administrator account and UAC enabled I don't get ERROR_ACCESS_DENIED
> > > > > > error with the tests included in the patch.
> > > > >
> > > > > Your tests get ERROR_ACCESS_DENIED on every single testbot vm except
> > > > > w8adm, so that's not very convincing... Is the app going to fail on all
> > > > > these vms?
> > > >
> > > > I'd guess if the CreateDirectory("C:\") returns ERROR_ACCESS_DENIED
> > > > then the app would fail. It's not clear how the VMs are configured
> > > > and why I don't get ERROR_ACCESS_DENIED on real hardware under a not
> > > > admin account, but it should be pretty obvious that since Wine doesn't
> > > > perform any real administrator access checks, and if it would the checks
> > > > should be done on the server side, ntdll checks shouldn't return access
> > > > denied error.
> > >
> > > It's also pretty obvious that this check was added for a reason, so it
> > > would need a more convincing argument to remove it.
> >
> > Do you recall the reason why that check was added? I can't find any
> > specific test case for this behaviour either.
>
> As far as I can tell, you added it ;-)
>
> https://source.winehq.org/git/wine.git/commit/d75aed2c92435e8ae4d5c260e31e81...

This doesn't look right, especially without any reasonable explanation.
Considering that now we have an application that depends on this, can
that change be reverted?

-- 
Dmitry.

[Alexandre Julliard]

Dmitry Timoshkov <dmitry(a)baikal.ru> writes:
> Alexandre Julliard <julliard(a)winehq.org> wrote:
> > > > > > >> > According to the testbot results CreateDirectory("C:\\", NULL) fails
> > > > > > >> > with ERROR_ACCESS_DENIED for not administrators. However with UAC enabled
> > > > > > >> > and not and administrator account I get ERROR_ALREADY_EXISTS in that case
> > > > > > >> > with Windows 7 64-bit running on real hardware. Moreover, Wine doesn't
> > > > > > >> > really perform any access checks in that case and blindly assumes that
> > > > > > >> > returning STATUS_ACCESS_DENIED is correct behaviour for the drive's root:
> > > > > > >> > dlls/ntdll/directory.c,lookup_unix_name().
> > > > > > >> >
> > > > > > >> > This patch fixes an application that can't find its data files because
> > > > > > >> > after it receives ERROR_ACCESS_DENIED it stops further directory traversing.
> > > > > > >> 
> > > > > > >> What app is that? Does it fail on Windows when not administrator?
> > > > > > >
> > > > > > > The application doesn't fail on Windows, and as I mentioned above under
> > > > > > > a not administrator account and UAC enabled I don't get ERROR_ACCESS_DENIED
> > > > > > > error with the tests included in the patch.
> > > > > >
> > > > > > Your tests get ERROR_ACCESS_DENIED on every single testbot vm except
> > > > > > w8adm, so that's not very convincing... Is the app going to fail on all
> > > > > > these vms?
> > > > >
> > > > > I'd guess if the CreateDirectory("C:\") returns ERROR_ACCESS_DENIED
> > > > > then the app would fail. It's not clear how the VMs are configured
> > > > > and why I don't get ERROR_ACCESS_DENIED on real hardware under a not
> > > > > admin account, but it should be pretty obvious that since Wine doesn't
> > > > > perform any real administrator access checks, and if it would the checks
> > > > > should be done on the server side, ntdll checks shouldn't return access
> > > > > denied error.
> > > >
> > > > It's also pretty obvious that this check was added for a reason, so it
> > > > would need a more convincing argument to remove it.
> > >
> > > Do you recall the reason why that check was added? I can't find any
> > > specific test case for this behaviour either.
> >
> > As far as I can tell, you added it ;-)
> >
> > https://source.winehq.org/git/wine.git/commit/d75aed2c92435e8ae4d5c260e31e81...
>
> This doesn't look right, especially without any reasonable explanation.
> Considering that now we have an application that depends on this, can
> that change be reverted?

I'd like to see some more convincing test cases.

-- 
Alexandre Julliard
julliard(a)winehq.org

[Dmitry Timoshkov]

Alexandre Julliard <julliard(a)winehq.org> wrote:
> > > > > > > >> > According to the testbot results CreateDirectory("C:\\", NULL) fails
> > > > > > > >> > with ERROR_ACCESS_DENIED for not administrators. However with UAC enabled
> > > > > > > >> > and not and administrator account I get ERROR_ALREADY_EXISTS in that case
> > > > > > > >> > with Windows 7 64-bit running on real hardware. Moreover, Wine doesn't
> > > > > > > >> > really perform any access checks in that case and blindly assumes that
> > > > > > > >> > returning STATUS_ACCESS_DENIED is correct behaviour for the drive's root:
> > > > > > > >> > dlls/ntdll/directory.c,lookup_unix_name().
> > > > > > > >> >
> > > > > > > >> > This patch fixes an application that can't find its data files because
> > > > > > > >> > after it receives ERROR_ACCESS_DENIED it stops further directory traversing.
> > > > > > > >> 
> > > > > > > >> What app is that? Does it fail on Windows when not administrator?
> > > > > > > >
> > > > > > > > The application doesn't fail on Windows, and as I mentioned above under
> > > > > > > > a not administrator account and UAC enabled I don't get ERROR_ACCESS_DENIED
> > > > > > > > error with the tests included in the patch.
> > > > > > > 
> > > > > > > Your tests get ERROR_ACCESS_DENIED on every single testbot vm except
> > > > > > > w8adm, so that's not very convincing... Is the app going to fail on all
> > > > > > > these vms?
> > > > > >
> > > > > > I'd guess if the CreateDirectory("C:\") returns ERROR_ACCESS_DENIED
> > > > > > then the app would fail. It's not clear how the VMs are configured
> > > > > > and why I don't get ERROR_ACCESS_DENIED on real hardware under a not
> > > > > > admin account, but it should be pretty obvious that since Wine doesn't
> > > > > > perform any real administrator access checks, and if it would the checks
> > > > > > should be done on the server side, ntdll checks shouldn't return access
> > > > > > denied error.
> > > > >
> > > > > It's also pretty obvious that this check was added for a reason, so it
> > > > > would need a more convincing argument to remove it.
> > > >
> > > > Do you recall the reason why that check was added? I can't find any
> > > > specific test case for this behaviour either.
> > >
> > > As far as I can tell, you added it ;-)
> > >
> > > https://source.winehq.org/git/wine.git/commit/d75aed2c92435e8ae4d5c260e31e81...
> >
> > This doesn't look right, especially without any reasonable explanation.
> > Considering that now we have an application that depends on this, can
> > that change be reverted?
>
> I'd like to see some more convincing test cases.

Sure, I can add more tests. Do you have a suggestion what kind of checks
they should perform? I tried to find an existing test that somehow elevates
privileges and then executes CreateDirecory tests, but couldn't find such
a test.

-- 
Dmitry.

[Alexandre Julliard]

Dmitry Timoshkov <dmitry(a)baikal.ru> writes:
> Alexandre Julliard <julliard(a)winehq.org> wrote:
> > > > > > >> >> > According to the testbot results CreateDirectory("C:\\", NULL) fails
> > > > > > >> >> > with ERROR_ACCESS_DENIED for not administrators. However with UAC enabled
> > > > > > >> >> > and not and administrator account I get ERROR_ALREADY_EXISTS in that case
> > > > > > >> >> > with Windows 7 64-bit running on real hardware. Moreover, Wine doesn't
> > > > > > >> >> > really perform any access checks in that case and blindly assumes that
> > > > > > >> >> > returning STATUS_ACCESS_DENIED is correct behaviour for the drive's root:
> > > > > > >> >> > dlls/ntdll/directory.c,lookup_unix_name().
> > > > > > >> >> >
> > > > > > >> >> > This patch fixes an application that can't find its data files because
> > > > > > >> >> > after it receives ERROR_ACCESS_DENIED it stops further directory traversing.
> > > > > > >> >> 
> > > > > > >> >> What app is that? Does it fail on Windows when not administrator?
> > > > > > >> >
> > > > > > >> > The application doesn't fail on Windows, and as I mentioned above under
> > > > > > >> > a not administrator account and UAC enabled I don't get ERROR_ACCESS_DENIED
> > > > > > >> > error with the tests included in the patch.
> > > > > > >> 
> > > > > > >> Your tests get ERROR_ACCESS_DENIED on every single testbot vm except
> > > > > > >> w8adm, so that's not very convincing... Is the app going to fail on all
> > > > > > >> these vms?
> > > > > > >
> > > > > > > I'd guess if the CreateDirectory("C:\") returns ERROR_ACCESS_DENIED
> > > > > > > then the app would fail. It's not clear how the VMs are configured
> > > > > > > and why I don't get ERROR_ACCESS_DENIED on real hardware under a not
> > > > > > > admin account, but it should be pretty obvious that since Wine doesn't
> > > > > > > perform any real administrator access checks, and if it would the checks
> > > > > > > should be done on the server side, ntdll checks shouldn't return access
> > > > > > > denied error.
> > > > > >
> > > > > > It's also pretty obvious that this check was added for a reason, so it
> > > > > > would need a more convincing argument to remove it.
> > > > >
> > > > > Do you recall the reason why that check was added? I can't find any
> > > > > specific test case for this behaviour either.
> > > >
> > > > As far as I can tell, you added it ;-)
> > > >
> > > > https://source.winehq.org/git/wine.git/commit/d75aed2c92435e8ae4d5c260e31e81...
> > >
> > > This doesn't look right, especially without any reasonable explanation.
> > > Considering that now we have an application that depends on this, can
> > > that change be reverted?
> >
> > I'd like to see some more convincing test cases.
>
> Sure, I can add more tests. Do you have a suggestion what kind of checks
> they should perform? I tried to find an existing test that somehow elevates
> privileges and then executes CreateDirecory tests, but couldn't find such
> a test.

It's possible that elevating privileges would avoid the access denied
error, but that doesn't explain why the app would work on Windows,
unless it also elevates privileges itself. Does it?

And FWIW I'm getting access denied on Vista even running as
administrator. If the behavior changed in recent Windows that would also
need specific tests.

-- 
Alexandre Julliard
julliard(a)winehq.org

[Dmitry Timoshkov]

Alexandre Julliard <julliard(a)winehq.org> wrote:
> > > > > > > >> >> > According to the testbot results CreateDirectory("C:\\", NULL) fails
> > > > > > > >> >> > with ERROR_ACCESS_DENIED for not administrators. However with UAC enabled
> > > > > > > >> >> > and not and administrator account I get ERROR_ALREADY_EXISTS in that case
> > > > > > > >> >> > with Windows 7 64-bit running on real hardware. Moreover, Wine doesn't
> > > > > > > >> >> > really perform any access checks in that case and blindly assumes that
> > > > > > > >> >> > returning STATUS_ACCESS_DENIED is correct behaviour for the drive's root:
> > > > > > > >> >> > dlls/ntdll/directory.c,lookup_unix_name().
> > > > > > > >> >> >
> > > > > > > >> >> > This patch fixes an application that can't find its data files because
> > > > > > > >> >> > after it receives ERROR_ACCESS_DENIED it stops further directory traversing.
> > > > > > > >> >> 
> > > > > > > >> >> What app is that? Does it fail on Windows when not administrator?
> > > > > > > >> >
> > > > > > > >> > The application doesn't fail on Windows, and as I mentioned above under
> > > > > > > >> > a not administrator account and UAC enabled I don't get ERROR_ACCESS_DENIED
> > > > > > > >> > error with the tests included in the patch.
> > > > > > > >> 
> > > > > > > >> Your tests get ERROR_ACCESS_DENIED on every single testbot vm except
> > > > > > > >> w8adm, so that's not very convincing... Is the app going to fail on all
> > > > > > > >> these vms?
> > > > > > > >
> > > > > > > > I'd guess if the CreateDirectory("C:\") returns ERROR_ACCESS_DENIED
> > > > > > > > then the app would fail. It's not clear how the VMs are configured
> > > > > > > > and why I don't get ERROR_ACCESS_DENIED on real hardware under a not
> > > > > > > > admin account, but it should be pretty obvious that since Wine doesn't
> > > > > > > > perform any real administrator access checks, and if it would the checks
> > > > > > > > should be done on the server side, ntdll checks shouldn't return access
> > > > > > > > denied error.
> > > > > > > 
> > > > > > > It's also pretty obvious that this check was added for a reason, so it
> > > > > > > would need a more convincing argument to remove it.
> > > > > >
> > > > > > Do you recall the reason why that check was added? I can't find any
> > > > > > specific test case for this behaviour either.
> > > > >
> > > > > As far as I can tell, you added it ;-)
> > > > >
> > > > > https://source.winehq.org/git/wine.git/commit/d75aed2c92435e8ae4d5c260e31e81...
> > > >
> > > > This doesn't look right, especially without any reasonable explanation.
> > > > Considering that now we have an application that depends on this, can
> > > > that change be reverted?
> > >
> > > I'd like to see some more convincing test cases.
> >
> > Sure, I can add more tests. Do you have a suggestion what kind of checks
> > they should perform? I tried to find an existing test that somehow elevates
> > privileges and then executes CreateDirecory tests, but couldn't find such
> > a test.
>
> It's possible that elevating privileges would avoid the access denied
> error, but that doesn't explain why the app would work on Windows,
> unless it also elevates privileges itself. Does it?

According to the log the doesn't elevate the privileges.
> And FWIW I'm getting access denied on Vista even running as
> administrator. If the behavior changed in recent Windows that would also
> need specific tests.

I just tested it under XP in a VM and I also get the access denied errors
for both an ordinal user and an administrator. So it looks like the behaviour
has changed (unless running in a VirtualBox changes the behaviour of a file
system).

-- 
Dmitry.

[Mathew Hodson]

On Tue, 11 Dec 2018 at 13:13, Dmitry Timoshkov <dmitry(a)baikal.ru> wrote:
> > > > > I'd guess if the CreateDirectory("C:\") returns ERROR_ACCESS_DENIED
> > > > > then the app would fail. It's not clear how the VMs are configured
> > > > > and why I don't get ERROR_ACCESS_DENIED on real hardware under a not
> > > > > admin account, but it should be pretty obvious that since Wine doesn't
> > > > > perform any real administrator access checks, and if it would the checks
> > > > > should be done on the server side, ntdll checks shouldn't return access
> > > > > denied error.
> > > >
> > > > It's also pretty obvious that this check was added for a reason, so it
> > > > would need a more convincing argument to remove it.
> > >
> > > Do you recall the reason why that check was added? I can't find any
> > > specific test case for this behaviour either.
> >
> > As far as I can tell, you added it ;-)
> >
> > https://source.winehq.org/git/wine.git/commit/d75aed2c92435e8ae4d5c260e31e81...
>
> This doesn't look right, especially without any reasonable explanation.
> Considering that now we have an application that depends on this, can
> that change be reverted?

I also found this commit, which adds the check as it most recently
existed. https://source.winehq.org/git/wine.git/commitdiff/cf67839bc4094678772858727b...