Re: [PATCH 1/3] msvcp: Implement the wchar version of _Open_dir.
Hi Stefan, On 05/18/17 13:33, Stefan Dösinger wrote:
+/* ??_Open_dir(a)sys@tr2(a)std@@YAPAXPA_WPB_WAAHAAW4file_type(a)123@@Z */ +/* ??_Open_dir(a)sys@tr2(a)std@@YAPEAXPEA_WPEB_WAEAHAEAW4file_type(a)123@@Z */ +void* __cdecl tr2_sys__Open_dir_wchar(wchar_t* target, wchar_t const* dest, int* err_code, enum file_type* type) { HANDLE handle; - WIN32_FIND_DATAA data; - char temppath[MAX_PATH]; - - TRACE("(%p %s %p %p)\n", target, debugstr_a(dest), err_code, type); - if(strlen(dest) > MAX_PATH - 3) { + WIN32_FIND_DATAW data; + wchar_t temppath[MAX_PATH]; + static const wchar_t dot[] = {'.', 0}; + static const wchar_t dotdot[] = {'.', '.', 0}; + static const wchar_t asterisk[] = {'\\', '*', 0}; + + TRACE("(%p %s %p %p)\n", target, debugstr_w(dest), err_code, type); + if(wcslen(dest) > MAX_PATH - 3) { *err_code = ERROR_BAD_PATHNAME; return NULL; } Could you please check if there really is such a length limit in wchar version of the function?
+/* ?_Open_dir(a)sys@tr2(a)std@@YAPAXAAY0BAE(a)DPBDAAHAAW4file_type@123@@Z */ +/* ?_Open_dir(a)sys@tr2(a)std@@YAPEAXAEAY0BAE(a)DPEBDAEAHAEAW4file_type@123@@Z */ +void* __cdecl tr2_sys__Open_dir(char* target, char const* dest, int* err_code, enum file_type* type) +{ + void *handle; + size_t len; + wchar_t target_w[MAX_PATH]; + wchar_t *dest_w = NULL; + + TRACE("(%p %s %p %p)\n", target, debugstr_a(dest), err_code, type); + + if (dest) + { + len = strlen(dest) + 1; + dest_w = malloc(sizeof(*dest_w) * len); You don't need to allocate dest_w dynamically. Its length is limited to ~MAX_PATH anyway.
Thanks, Piotr
Am 2017-05-18 um 14:35 schrieb Piotr Caban:
Could you please check if there really is such a length limit in wchar version of the function? I'm working on it. What is the deal with wchar_t vs WCHAR in the implementation vs the tests?
Also I am not sure the existing test establishes the -3 for the A version. I'll extend it if my reading is right.
+ WideCharToMultiByte(CP_ACP, 0, target_w, -1, target, wcslen(target_w) + 1, NULL, NULL); You can use MAX_PATH here instead of wcslen(target_w)+1. I guess the app is required to pass a pointer to a string that's at least MAX_PATH in size because there is no way to communicate the length. Given that constraint it should be safe to convert MAX_PATH chars.
However, with the actual implementation an app could get away with a shorter array if the actual path is shorter I think, and the app may be able to know the length in advance. So blindly converting MAX_PATH characters could cause a problem.
Am 2017-05-18 um 17:46 schrieb Stefan Dösinger:
Am 2017-05-18 um 14:35 schrieb Piotr Caban:
Could you please check if there really is such a length limit in wchar version of the function? I'm working on it. What is the deal with wchar_t vs WCHAR in the implementation vs the tests?
Also I am not sure the existing test establishes the -3 for the A version. I'll extend it if my reading is right. Yes, the restriction exists. I guess it is to make sure concatenating "\\.." is safe, or something like that.
I had to create a bunch of (too) long directories on my Linux computer and share it with Samba on Windows to test this. Windows apparently refuses to create a directory with a name longer than 240 characters. I guess this is why we don't have an actual test for this. msvcp doesn't make a difference between a directory that is too long and one that doesn't exist. I guess we could try to create long directories by creating short ones in a a subdirectory, and then moving this subdirectory into a longer subdirectory. This smells like the entrance to a dragon lair housing 260 dragons and I'd rather not venture inside.
On 05/18/17 18:52, Stefan Dösinger wrote:
Am 2017-05-18 um 17:46 schrieb Stefan Dösinger:
Am 2017-05-18 um 14:35 schrieb Piotr Caban:
Could you please check if there really is such a length limit in wchar version of the function? I'm working on it. What is the deal with wchar_t vs WCHAR in the implementation vs the tests?
Also I am not sure the existing test establishes the -3 for the A version. I'll extend it if my reading is right. Yes, the restriction exists. I guess it is to make sure concatenating "\\.." is safe, or something like that.
I had to create a bunch of (too) long directories on my Linux computer and share it with Samba on Windows to test this. Windows apparently refuses to create a directory with a name longer than 240 characters. I guess this is why we don't have an actual test for this. msvcp doesn't make a difference between a directory that is too long and one that doesn't exist.
I guess we could try to create long directories by creating short ones in a a subdirectory, and then moving this subdirectory into a longer subdirectory. This smells like the entrance to a dragon lair housing 260 dragons and I'd rather not venture inside. I don't think it's a test that should go into wine. It was just something worth checking. Thanks for doing that.
Piotr
Hi, On 05/18/17 17:46, Stefan Dösinger wrote:
Am 2017-05-18 um 14:35 schrieb Piotr Caban:
Could you please check if there really is such a length limit in wchar version of the function? I'm working on it. What is the deal with wchar_t vs WCHAR in the implementation vs the tests? It can probably use some cleanup :)
Also I am not sure the existing test establishes the -3 for the A version. I'll extend it if my reading is right.
+ WideCharToMultiByte(CP_ACP, 0, target_w, -1, target, wcslen(target_w) + 1, NULL, NULL); You can use MAX_PATH here instead of wcslen(target_w)+1. I guess the app is required to pass a pointer to a string that's at least MAX_PATH in size because there is no way to communicate the length. Given that constraint it should be safe to convert MAX_PATH chars.
However, with the actual implementation an app could get away with a shorter array if the actual path is shorter I think, and the app may be able to know the length in advance. So blindly converting MAX_PATH characters could cause a problem. There's no change in function behavior if MAX_PATH is passed to WideCharToMultiByte here. This argument means that at most MAX_PATH characters will be written to target buffer. It will still work if application passes smaller buffer.
The application is actually required to pass a MAX_PATH buffer to tr2_sys__Open_dir function. Demangled function signature is following: void * __cdecl std::tr2::sys::_Open_dir(char (&)[260],char const *,int &,enum std::tr2::sys::file_type &) (of course application can ignore this restriction with a simple cast) Thanks, Piotr
Am 2017-05-18 um 14:35 schrieb Piotr Caban:
You don't need to allocate dest_w dynamically. Its length is limited to ~MAX_PATH anyway. I'd have to duplicate the length check from the wchar version to make sure of that though. Either way works for me, just tell me what you prefer.
On 05/18/17 18:54, Stefan Dösinger wrote:
Am 2017-05-18 um 14:35 schrieb Piotr Caban:
You don't need to allocate dest_w dynamically. Its length is limited to ~MAX_PATH anyway. I'd have to duplicate the length check from the wchar version to make sure of that though. Either way works for me, just tell me what you prefer. I think it's better if it's not allocated dynamically. Also I don't think you need to duplicate the error checking. You should check if MultiByteToWideChar fails when converting dest to dest_w anyway.
participants (2)
-
Piotr Caban -
Stefan Dösinger