Re: Fix lossage due to pathname truncation in calls to MODULE_LoadLibraryExA
On Thu, Sep 12, 2002 at 08:38:09PM +1000, Michael Beach wrote:
Recently I've been using WINE to run a Win32 exe which needs to load certain DLLs which are located in the same directory as the exe. I was most perplexed when I found that this worked initially, but when I moved the exe and DLLs to a different directory it stopped working ie WINE would refuse to start the exe, claiming that it was unable to load the required DLLs.
After some debugging, I discovered that the critical factor was the length of the pathname to the exe, when it passed a certain length it stopped working. However this length was way short of the MAX_PATH value of 260. Ah, cool finding !
static BOOL DIR_TryModulePath( LPCWSTR name, DOS_FULL_NAME *full_name, BOOL win32 ) { - /* FIXME: for now, GetModuleFileNameW can't return more */ - /* than OFS_MAXPATHNAME. This may change with Win32. */ - WCHAR bufferW[OFS_MAXPATHNAME]; + WCHAR bufferW[MAX_PATH]; LPWSTR p;
if (!win32) @@ -727,13 +725,13 @@ if (!GetCurrentTask()) return FALSE; if (!GetModuleFileName16( GetCurrentTask(), buffer, sizeof(buffer) )) return FALSE; - MultiByteToWideChar(CP_ACP, 0, buffer, -1, bufferW, OFS_MAXPATHNAME); + MultiByteToWideChar(CP_ACP, 0, buffer, -1, bufferW, MAX_PATH); } else { - if (!GetModuleFileNameW( 0, bufferW, OFS_MAXPATHNAME ) ) + if (!GetModuleFileNameW( 0, bufferW, MAX_PATH ) ) return FALSE; } if (!(p = strrchrW( bufferW, '\\' ))) return FALSE; - if (OFS_MAXPATHNAME - (++p - bufferW) <= strlenW(name)) return FALSE; + if (MAX_PATH - (++p - bufferW) <= strlenW(name)) return FALSE; strcpyW( p, name ); return DOSFS_GetFullName( bufferW, TRUE, full_name ); Argl, why does this code use the buffer size contants instead of sizeof(variable) !? I suggest we always specify buffer length constants only *once*, namely at creation of the buffer. Not doing so can be potentially very harmful if we decide to change the buffer length and then manage to forget one or more length constants...
Maybe you could even also fix that "weirdness" in our code ? Thanks ! :) -- The Declaration of Software Freedom: http://freedevelopers.net/freedomdec/index.php
On Thursday 12 September 2002 20:55, Andreas Mohr wrote:
On Thu, Sep 12, 2002 at 08:38:09PM +1000, Michael Beach wrote:
Recently I've been using WINE to run a Win32 exe which needs to load certain DLLs which are located in the same directory as the exe. I was most perplexed when I found that this worked initially, but when I moved the exe and DLLs to a different directory it stopped working ie WINE would refuse to start the exe, claiming that it was unable to load the required DLLs.
After some debugging, I discovered that the critical factor was the length of the pathname to the exe, when it passed a certain length it stopped working. However this length was way short of the MAX_PATH value of 260.
Ah, cool finding !
Thanks.
static BOOL DIR_TryModulePath( LPCWSTR name, DOS_FULL_NAME *full_name, BOOL win32 ) { - /* FIXME: for now, GetModuleFileNameW can't return more */ - /* than OFS_MAXPATHNAME. This may change with Win32. */ - WCHAR bufferW[OFS_MAXPATHNAME]; + WCHAR bufferW[MAX_PATH]; LPWSTR p;
if (!win32) @@ -727,13 +725,13 @@ if (!GetCurrentTask()) return FALSE; if (!GetModuleFileName16( GetCurrentTask(), buffer, sizeof(buffer) )) return FALSE; - MultiByteToWideChar(CP_ACP, 0, buffer, -1, bufferW, OFS_MAXPATHNAME); + MultiByteToWideChar(CP_ACP, 0, buffer, -1, bufferW, MAX_PATH); } else { - if (!GetModuleFileNameW( 0, bufferW, OFS_MAXPATHNAME ) ) + if (!GetModuleFileNameW( 0, bufferW, MAX_PATH ) ) return FALSE; } if (!(p = strrchrW( bufferW, '\\' ))) return FALSE; - if (OFS_MAXPATHNAME - (++p - bufferW) <= strlenW(name)) return FALSE; + if (MAX_PATH - (++p - bufferW) <= strlenW(name)) return FALSE; strcpyW( p, name ); return DOSFS_GetFullName( bufferW, TRUE, full_name );
Argl, why does this code use the buffer size contants instead of sizeof(variable) !? I suggest we always specify buffer length constants only *once*, namely at creation of the buffer. Not doing so can be potentially very harmful if we decide to change the buffer length and then manage to forget one or more length constants...
Maybe you could even also fix that "weirdness" in our code ?
Thanks ! :)
I see you subscribe to the very popular "it never hurts to ask philosophy" ;-) However I don't think saying something like sizeof(bufferW) is a clear winner here, as we're not interested in the size in terms of the number of bytes (or number of items of data of type char to be pedantic) of bufferW, but rather the number of elements in bufferW. To get the number of elements in bufferW we'd have to use sizeof(bufferW) / sizeof(WCHAR), which is a bit long winded, but I suppose could be wrapped by a macro. However even if we did this then instead of suffering from the "we decide to change the buffer length and then manage to forget one or more length constants" problem, we would instead suffer from the "we decide to change the buffer type and then manage to forget one or more length calculations" problem. So I can't see a compelling reason to switch to sizeof here. Of course we could switch to coding in Ada, and take advantage of attributes to tell us exactly what we wanted to know about the variable, or perhaps coding in C++ where we could accomplish much the same thing using some template-based smoke and mirrors. No, I'm not serious ;-) Regards M.Beach
Michael Beach wrote:
static BOOL DIR_TryModulePath( LPCWSTR name, DOS_FULL_NAME *full_name, BOOL win32 ) { - /* FIXME: for now, GetModuleFileNameW can't return more */ - /* than OFS_MAXPATHNAME. This may change with Win32. */ - WCHAR bufferW[OFS_MAXPATHNAME]; + WCHAR bufferW[MAX_PATH]; LPWSTR p;
if (!win32) @@ -727,13 +725,13 @@ if (!GetCurrentTask()) return FALSE; if (!GetModuleFileName16( GetCurrentTask(), buffer, sizeof(buffer) )) return FALSE; - MultiByteToWideChar(CP_ACP, 0, buffer, -1, bufferW, OFS_MAXPATHNAME); + MultiByteToWideChar(CP_ACP, 0, buffer, -1, bufferW, MAX_PATH); } else { - if (!GetModuleFileNameW( 0, bufferW, OFS_MAXPATHNAME ) ) + if (!GetModuleFileNameW( 0, bufferW, MAX_PATH ) ) return FALSE; } if (!(p = strrchrW( bufferW, '\\' ))) return FALSE; - if (OFS_MAXPATHNAME - (++p - bufferW) <= strlenW(name)) return FALSE; + if (MAX_PATH - (++p - bufferW) <= strlenW(name)) return FALSE; strcpyW( p, name ); return DOSFS_GetFullName( bufferW, TRUE, full_name );
Argl, why does this code use the buffer size contants instead of sizeof(variable) !? I suggest we always specify buffer length constants only *once*, namely at creation of the buffer. Not doing so can be potentially very harmful if we decide to change the buffer length and then manage to forget one or more length constants...
Maybe you could even also fix that "weirdness" in our code ?
Thanks ! :)
I see you subscribe to the very popular "it never hurts to ask philosophy" ;-)
However I don't think saying something like sizeof(bufferW) is a clear winner here, as we're not interested in the size in terms of the number of bytes (or number of items of data of type char to be pedantic) of bufferW, but rather the number of elements in bufferW. To get the number of elements in bufferW we'd have to use sizeof(bufferW) / sizeof(WCHAR), which is a bit long winded, but I suppose could be wrapped by a macro.
However even if we did this then instead of suffering from the "we decide to change the buffer length and then manage to forget one or more length constants" problem, we would instead suffer from the "we decide to change the buffer type and then manage to forget one or more length calculations" problem.
So I can't see a compelling reason to switch to sizeof here. Of course we could switch to coding in Ada, and take advantage of attributes to tell us exactly what we wanted to know about the variable, or perhaps coding in C++ where we could accomplish much the same thing using some template-based smoke and mirrors. No, I'm not serious ;-)
Regards M.Beach
What about defining the length of the buffer as a constant integer const int bufferW_len = MAX_PATH; WCHAR bufferW[bufferW_len]; Then we can use bufferW_len whenever we need to refer to the length of the array in the function, without worrying about the size or type, and still only change it in one place. Also it is clear in the definition what we're doing Just an idea :-)
On Friday 13 September 2002 00:44, David Fraser wrote:
Michael Beach wrote:
static BOOL DIR_TryModulePath( LPCWSTR name, DOS_FULL_NAME *full_name, BOOL win32 ) { - /* FIXME: for now, GetModuleFileNameW can't return more */ - /* than OFS_MAXPATHNAME. This may change with Win32. */ - WCHAR bufferW[OFS_MAXPATHNAME]; + WCHAR bufferW[MAX_PATH]; LPWSTR p;
if (!win32) @@ -727,13 +725,13 @@ if (!GetCurrentTask()) return FALSE; if (!GetModuleFileName16( GetCurrentTask(), buffer, sizeof(buffer) )) return FALSE; - MultiByteToWideChar(CP_ACP, 0, buffer, -1, bufferW, OFS_MAXPATHNAME); + MultiByteToWideChar(CP_ACP, 0, buffer, -1, bufferW, MAX_PATH); } else { - if (!GetModuleFileNameW( 0, bufferW, OFS_MAXPATHNAME ) ) + if (!GetModuleFileNameW( 0, bufferW, MAX_PATH ) ) return FALSE; } if (!(p = strrchrW( bufferW, '\\' ))) return FALSE; - if (OFS_MAXPATHNAME - (++p - bufferW) <= strlenW(name)) return FALSE; + if (MAX_PATH - (++p - bufferW) <= strlenW(name)) return FALSE; strcpyW( p, name ); return DOSFS_GetFullName( bufferW, TRUE, full_name );
Argl, why does this code use the buffer size contants instead of sizeof(variable) !? I suggest we always specify buffer length constants only *once*, namely at creation of the buffer. Not doing so can be potentially very harmful if we decide to change the buffer length and then manage to forget one or more length constants...
Maybe you could even also fix that "weirdness" in our code ?
Thanks ! :)
I see you subscribe to the very popular "it never hurts to ask philosophy" ;-)
However I don't think saying something like sizeof(bufferW) is a clear winner here, as we're not interested in the size in terms of the number of bytes (or number of items of data of type char to be pedantic) of bufferW, but rather the number of elements in bufferW. To get the number of elements in bufferW we'd have to use sizeof(bufferW) / sizeof(WCHAR), which is a bit long winded, but I suppose could be wrapped by a macro.
However even if we did this then instead of suffering from the "we decide to change the buffer length and then manage to forget one or more length constants" problem, we would instead suffer from the "we decide to change the buffer type and then manage to forget one or more length calculations" problem.
So I can't see a compelling reason to switch to sizeof here. Of course we could switch to coding in Ada, and take advantage of attributes to tell us exactly what we wanted to know about the variable, or perhaps coding in C++ where we could accomplish much the same thing using some template-based smoke and mirrors. No, I'm not serious ;-)
Regards M.Beach
What about defining the length of the buffer as a constant integer
const int bufferW_len = MAX_PATH; WCHAR bufferW[bufferW_len];
Then we can use bufferW_len whenever we need to refer to the length of the array in the function, without worrying about the size or type, and still only change it in one place. Also it is clear in the definition what we're doing
Just an idea :-)
And a very good one IMHO. But as for the name bufferW_len, is that consistent with WINE's coding conventions (I don't have much of a feel for them)? I won't be making these changes for another 12 hours or so (my source tree is on another machine), so if anyone would care to comment on the proposed name, please jump right in. Regards M.Beach
Argl, why does this code use the buffer size contants instead of sizeof(variable) !? I suggest we always specify buffer length constants only *once*, namely at creation of the buffer. Not doing so can be potentially very harmful if we decide to change the buffer length and then manage to forget one or more length constants...
However I don't think saying something like sizeof(bufferW) is a clear winner here, as we're not interested in the size in terms of the number of bytes (or number of items of data of type char to be pedantic) of bufferW, but rather the number of elements in bufferW. To get the number of elements in bufferW we'd have to use sizeof(bufferW) / sizeof(WCHAR), which is a bit long winded, but I suppose could be wrapped by a macro.
The 'usual' definition is sizeof bufferW / sizeof *bufferW sometimes encapsulated in a NELEM (or nelem) macro: #define nelem(x) (sizeof (x) / sizeof *(x)) Then the constant only appears once. Indeed you have to ask whether MAX_PATH is an enforced system restraint or just wishful thinking. Certainly the NetBSD kernel doesn't enforce it (although some shells enforce it (or other arbitrary limits) on the length of $PWD. David -- David Laight: david(a)l8s.co.uk
On Friday 13 September 2002 02:29, David Laight wrote:
Argl, why does this code use the buffer size contants instead of sizeof(variable) !? I suggest we always specify buffer length constants only *once*, namely at creation of the buffer. Not doing so can be potentially very harmful if we decide to change the buffer length and then manage to forget one or more length constants...
However I don't think saying something like sizeof(bufferW) is a clear winner here, as we're not interested in the size in terms of the number of bytes (or number of items of data of type char to be pedantic) of bufferW, but rather the number of elements in bufferW. To get the number of elements in bufferW we'd have to use sizeof(bufferW) / sizeof(WCHAR), which is a bit long winded, but I suppose could be wrapped by a macro.
The 'usual' definition is sizeof bufferW / sizeof *bufferW sometimes encapsulated in a NELEM (or nelem) macro: #define nelem(x) (sizeof (x) / sizeof *(x))
Thanks, I'd totally forgotten about the dereferencing trick to get the size of an element!
Then the constant only appears once. Indeed you have to ask whether MAX_PATH is an enforced system restraint or just wishful thinking. Certainly the NetBSD kernel doesn't enforce it (although some shells enforce it (or other arbitrary limits) on the length of $PWD.
I was under the impression that MAX_PATH is a Windows limitation rather than one of the UNIX that WINE is running on. Since Windows doesn't promise to permit any more than MAX_PATH, we gain nothing by allowing for more in WINE, hence the use of MAX_PATH to size the buffers. But I wonder about the wacky //?/ pathnames that Windows supports, and if WINE can cope with them...
David
Regards M.Beach
I was under the impression that MAX_PATH is a Windows limitation rather than one of the UNIX that WINE is running on. Since Windows doesn't promise to permit any more than MAX_PATH, we gain nothing by allowing for more in WINE, hence the use of MAX_PATH to size the buffers.
Dunno about widnows, but posix and unix claim to have a PATH_MAX. Unix file system code doesn't usually check (after all it would have to know the max depth at any point - otherwise you could rename a directory and exceed the limit way down the file tree). It is the sort of limit that is rather painful - you either allocate a massive (on stack) buffer most of which is never needed (and traditiobally fail to check for overflow) or dynamically allocate a buffer that is big enough and don't have a limit at all. David -- David Laight: david(a)l8s.co.uk
On Friday 13 September 2002 09:13, David Laight wrote:
I was under the impression that MAX_PATH is a Windows limitation rather than one of the UNIX that WINE is running on. Since Windows doesn't promise to permit any more than MAX_PATH, we gain nothing by allowing for more in WINE, hence the use of MAX_PATH to size the buffers.
Dunno about widnows, but posix and unix claim to have a PATH_MAX. Unix file system code doesn't usually check (after all it would have to know the max depth at any point - otherwise you could rename a directory and exceed the limit way down the file tree).
Well, I don't think it's intended to enforce a bound on the maximum length of any possible path, but rather the maximum size of a pathname that you can pass as an argument to stuff like open() or stat(). I formed this impression by reading the sections on limits.h and pathconf() in the Open Group (aka POSIX, more or less) specs at http://www.opengroup.org/onlinepubs/007904975/toc.htm http://msdn.microsoft.com/library/default.asp?url=/library/en-us/fileio/base... mentions the role of MAX_PATH in Windows. A peek in the headers shows that it has the value of 260.
It is the sort of limit that is rather painful - you either allocate a massive (on stack) buffer most of which is never needed (and traditiobally fail to check for overflow) or dynamically allocate a buffer that is big enough and don't have a limit at all.
Yes, a pain, but I think the APIs oblige us to take the conservatively sized on-stack buffer option.
David
Regards M.Beach
Well, I don't think it's intended to enforce a bound on the maximum length of any possible path, but rather the maximum size of a pathname that you can pass as an argument to stuff like open() or stat(). I formed this impression by reading the sections on limits.h and pathconf() in the Open Group (aka POSIX, more or less) specs at http://www.opengroup.org/onlinepubs/007904975/toc.htm
I haven't read that bit of that document that closely :-) David -- David Laight: david(a)l8s.co.uk
participants (4)
-
Andreas Mohr -
David Fraser -
David Laight -
Michael Beach