Hi, could anyone try the attached patch for the qedit test on a windows machine? It works around bug 16548. But according to the comment in the source I think this could be a mistake. I'd like to know if it works without renaming the file on windows. On wine it works.
Cheers Rico
diff --git a/dlls/qedit/tests/mediadet.c b/dlls/qedit/tests/mediadet.c index 2b43124..558ff3a 100644 --- a/dlls/qedit/tests/mediadet.c +++ b/dlls/qedit/tests/mediadet.c @@ -68,8 +68,10 @@ static BOOL unpack_avi_file(int id, WCHAR name[MAX_PATH]) return FALSE;
DeleteFileW(name); +/* + Renaming isn't a good way to solve this, see bug 16548. lstrcpyW(name + lstrlenW(name) - 3, avi); - +*/ fh = CreateFileW(name, GENERIC_WRITE, 0, NULL, CREATE_NEW, FILE_ATTRIBUTE_NORMAL, NULL); if (fh == INVALID_HANDLE_VALUE)
On Thu, Dec 18, 2008 at 4:22 PM, Rico Schüller kgbricola@web.de wrote:
Hi, could anyone try the attached patch for the qedit test on a windows machine? It works around bug 16548. But according to the comment in the source I think this could be a mistake. I'd like to know if it works without renaming the file on windows. On wine it works.
Cheers Rico
diff --git a/dlls/qedit/tests/mediadet.c b/dlls/qedit/tests/mediadet.c index 2b43124..558ff3a 100644 --- a/dlls/qedit/tests/mediadet.c +++ b/dlls/qedit/tests/mediadet.c @@ -68,8 +68,10 @@ static BOOL unpack_avi_file(int id, WCHAR name[MAX_PATH]) return FALSE;
DeleteFileW(name);+/*
- Renaming isn't a good way to solve this, see bug 16548. lstrcpyW(name + lstrlenW(name) - 3, avi);
+*/ fh = CreateFileW(name, GENERIC_WRITE, 0, NULL, CREATE_NEW, FILE_ATTRIBUTE_NORMAL, NULL); if (fh == INVALID_HANDLE_VALUE)
Tested on XP SP 3:
-Original- C:\Documents and Settings\Austin\Desktop>qedit_crosstest.exe mediadet: 62 tests executed (0 marked as todo, 0 failures), 0 skipped.
-Your patch- C:\Documents and Settings\Austinl\Desktop>qedit_crosstest_new.exe mediadet.c:303: Tests skipped: Couldn't initialize tests! mediadet: 0 tests executed (0 marked as todo, 0 failures), 1 skipped.
Austin English schrieb:
On Thu, Dec 18, 2008 at 4:22 PM, Rico Schüller kgbricola@web.de wrote:
Hi, could anyone try the attached patch for the qedit test on a windows machine? It works around bug 16548. But according to the comment in the source I think this could be a mistake. I'd like to know if it works without renaming the file on windows. On wine it works.
Cheers Rico
diff --git a/dlls/qedit/tests/mediadet.c b/dlls/qedit/tests/mediadet.c index 2b43124..558ff3a 100644 --- a/dlls/qedit/tests/mediadet.c +++ b/dlls/qedit/tests/mediadet.c @@ -68,8 +68,10 @@ static BOOL unpack_avi_file(int id, WCHAR name[MAX_PATH]) return FALSE;
DeleteFileW(name);+/*
- Renaming isn't a good way to solve this, see bug 16548. lstrcpyW(name + lstrlenW(name) - 3, avi);
+*/ fh = CreateFileW(name, GENERIC_WRITE, 0, NULL, CREATE_NEW, FILE_ATTRIBUTE_NORMAL, NULL); if (fh == INVALID_HANDLE_VALUE)
Tested on XP SP 3:
-Original- C:\Documents and Settings\Austin\Desktop>qedit_crosstest.exe mediadet: 62 tests executed (0 marked as todo, 0 failures), 0 skipped.
-Your patch- C:\Documents and Settings\Austinl\Desktop>qedit_crosstest_new.exe mediadet.c:303: Tests skipped: Couldn't initialize tests! mediadet: 0 tests executed (0 marked as todo, 0 failures), 1 skipped.
Thanks for your test. I checked this on 3 different XP machines and all run the tests with my patch fine. I've no glue why it fails for you.
Probably, could you first verify that there is a bug in wine? If not it's probably a bug with my machine. A simple example which should trigger the bug in wine is attached. You could build it with mingw. Command: "wine main.exe | uniq | wc" Output on wine: 142 142 4118 Output on windows: 1000 1000 28000 Wine used the same unique temp names two times or more in a row, so that the number is smaller than 1000. This makes it possible to fail the qedit test on my machine. On windows I get always 1000.
Cheers Rico
#include <stdio.h> #include <stdlib.h> #include <windows.h>
int main() { char name[1024]; int i; for(i=0;i<1000;i++) { GetTempFileName("C:\windows\temp\", "murks", 0, name); DeleteFile(name); printf("%s\n", name); } return 0; }
On Fri, Dec 19, 2008 at 2:32 PM, Rico Schüller kgbricola@web.de wrote:
Austin English schrieb:
On Thu, Dec 18, 2008 at 4:22 PM, Rico Schüller kgbricola@web.de wrote:
Hi, could anyone try the attached patch for the qedit test on a windows machine? It works around bug 16548. But according to the comment in the source I think this could be a mistake. I'd like to know if it works without renaming the file on windows. On wine it works.
Cheers Rico
diff --git a/dlls/qedit/tests/mediadet.c b/dlls/qedit/tests/mediadet.c index 2b43124..558ff3a 100644 --- a/dlls/qedit/tests/mediadet.c +++ b/dlls/qedit/tests/mediadet.c @@ -68,8 +68,10 @@ static BOOL unpack_avi_file(int id, WCHAR name[MAX_PATH]) return FALSE;
DeleteFileW(name); +/*
- Renaming isn't a good way to solve this, see bug 16548. lstrcpyW(name + lstrlenW(name) - 3, avi);
+*/ fh = CreateFileW(name, GENERIC_WRITE, 0, NULL, CREATE_NEW, FILE_ATTRIBUTE_NORMAL, NULL); if (fh == INVALID_HANDLE_VALUE)
Tested on XP SP 3:
-Original- C:\Documents and Settings\Austin\Desktop>qedit_crosstest.exe mediadet: 62 tests executed (0 marked as todo, 0 failures), 0 skipped.
-Your patch- C:\Documents and Settings\Austinl\Desktop>qedit_crosstest_new.exe mediadet.c:303: Tests skipped: Couldn't initialize tests! mediadet: 0 tests executed (0 marked as todo, 0 failures), 1 skipped.
Thanks for your test. I checked this on 3 different XP machines and all run the tests with my patch fine. I've no glue why it fails for you.
Probably, could you first verify that there is a bug in wine? If not it's probably a bug with my machine. A simple example which should trigger the bug in wine is attached. You could build it with mingw. Command: "wine main.exe | uniq | wc" Output on wine: 142 142 4118 Output on windows: 1000 1000 28000 Wine used the same unique temp names two times or more in a row, so that the number is smaller than 1000. This makes it possible to fail the qedit test on my machine. On windows I get always 1000.
Cheers Rico
#include <stdio.h> #include <stdlib.h> #include <windows.h>
int main() { char name[1024]; int i; for(i=0;i<1000;i++) { GetTempFileName("C:\windows\temp\", "murks", 0, name); DeleteFile(name); printf("%s\n", name); } return 0; }
Windows gets 1000 here as well. Wine gets random numbers, always less than 1000, and usually around 150-190.
On Thu, Dec 18, 2008 at 4:22 PM, Rico Schüller kgbricola@web.de wrote:
Hi, could anyone try the attached patch for the qedit test on a windows machine? It works around bug 16548. But according to the comment in the source I think this could be a mistake. I'd like to know if it works without renaming the file on windows. On wine it works.
Cheers Rico
diff --git a/dlls/qedit/tests/mediadet.c b/dlls/qedit/tests/mediadet.c index 2b43124..558ff3a 100644 --- a/dlls/qedit/tests/mediadet.c +++ b/dlls/qedit/tests/mediadet.c @@ -68,8 +68,10 @@ static BOOL unpack_avi_file(int id, WCHAR name[MAX_PATH]) return FALSE;
DeleteFileW(name);+/*
- Renaming isn't a good way to solve this, see bug 16548. lstrcpyW(name + lstrlenW(name) - 3, avi);
+*/ fh = CreateFileW(name, GENERIC_WRITE, 0, NULL, CREATE_NEW, FILE_ATTRIBUTE_NORMAL, NULL); if (fh == INVALID_HANDLE_VALUE)
Tested on my 2k machine, works fine though. That XP machine has always been a bit buggy, so I wouldn't worry about it too much (times out on the tests a lot, even though it's got 2 GB ram/dual core CPU).
Austin English schrieb:
On Thu, Dec 18, 2008 at 4:22 PM, Rico Schüller kgbricola@web.de wrote:
Hi, could anyone try the attached patch for the qedit test on a windows machine? It works around bug 16548. But according to the comment in the source I think this could be a mistake. I'd like to know if it works without renaming the file on windows. On wine it works.
Cheers Rico
diff --git a/dlls/qedit/tests/mediadet.c b/dlls/qedit/tests/mediadet.c index 2b43124..558ff3a 100644 --- a/dlls/qedit/tests/mediadet.c +++ b/dlls/qedit/tests/mediadet.c @@ -68,8 +68,10 @@ static BOOL unpack_avi_file(int id, WCHAR name[MAX_PATH]) return FALSE;
DeleteFileW(name);+/*
- Renaming isn't a good way to solve this, see bug 16548. lstrcpyW(name + lstrlenW(name) - 3, avi);
+*/ fh = CreateFileW(name, GENERIC_WRITE, 0, NULL, CREATE_NEW, FILE_ATTRIBUTE_NORMAL, NULL); if (fh == INVALID_HANDLE_VALUE)
Tested on my 2k machine, works fine though. That XP machine has always been a bit buggy, so I wouldn't worry about it too much (times out on the tests a lot, even though it's got 2 GB ram/dual core CPU).
Thanks for your test.
I think I've found the problem ... it is in UINT WINAPI GetTempFileNameW( LPCWSTR path, LPCWSTR prefix, UINT unique, LPWSTR buffer ) in line UINT num = GetTickCount() & 0xffff; .
On fast machines GetTickCount has the same value when it is called two times in a short time. Therefore it generates the same time and as a result the same file, which isn't the case on windows. On windows it looks like there is a number increasing each run by one, something like this static UINT num; num = num++ & 0xffff; should be the solution.
To make the qedit test happy a Sleep(...) between the two calls should be enough.
Cheers Rico
On Sat, 20 Dec 2008, Rico Schüller wrote: [...]
I think I've found the problem ... it is in UINT WINAPI GetTempFileNameW( LPCWSTR path, LPCWSTR prefix, UINT unique, LPWSTR buffer ) in line UINT num = GetTickCount() & 0xffff; .
On fast machines GetTickCount has the same value when it is called two times in a short time. Therefore it generates the same time and as a result the same file, which isn't the case on windows. On windows it looks like there is a number increasing each run by one, something like this static UINT num; num = num++ & 0xffff; should be the solution.
To make the qedit test happy a Sleep(...) between the two calls should be enough.
In either cases, shouldn't we worry about the filename being predictable with the security issues this implies? I know this very issue has generated a lot of security issues on Unix...
Francois Gouget schrieb:
On Sat, 20 Dec 2008, Rico Schüller wrote: [...]
I think I've found the problem ... it is in UINT WINAPI GetTempFileNameW( LPCWSTR path, LPCWSTR prefix, UINT unique, LPWSTR buffer ) in line UINT num = GetTickCount() & 0xffff; . On fast machines GetTickCount has the same value when it is called two times in a short time. Therefore it generates the same time and as a result the same file, which isn't the case on windows. On windows it looks like there is a number increasing each run by one, something like this static UINT num; num = num++ & 0xffff; should be the solution.
To make the qedit test happy a Sleep(...) between the two calls should be enough.
In either cases, shouldn't we worry about the filename being predictable with the security issues this implies? I know this very issue has generated a lot of security issues on Unix...
The filename which is generated by GetTempFileName is always predictable. You simply have to generate 0xffff-1 files in the temp folder which match this name "prefix{xxxx}.tmp" and then GetTempFileName could only return the missing one! So I didn't see a security reason here. Security could only be given by generating the file with the correct rights. So no other app/user could change the files.
We should use the generated tmp file as it is, because there is a chance, when the qedit test deletes and generates a new one another app could have opened a file with this name already. Then the test would be skipped without a real reason.
Any suggestions how to fix the test? Should I also change the behaviour for GetTempFileName so that it matches the one from XP?
Cheers Rico
-
Austin English -
Francois Gouget -
Rico Schüller