Re: Fix lossage due to pathname truncation in calls to MODULE_LoadLibraryExA

[Michael Beach]

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