> -----Original Message-----
> From: Magnus.Svensson@stripped [mailto:Magnus.Svensson@stripped]
> Sent: Tuesday, September 16, 2008 11:06 AM
> To: Vladislav Vaintroub
> Cc: 'Vladislav Vaintroub'; 'magnus 'msvensson' Svensson';
> commits@stripped
> Subject: Re: bzr commit into mysql-5.1-bugteam branch (vvaintroub:2678)
> Bug#35987
>
> Vladislav Vaintroub skrev:
> >
> >> -----Original Message-----
> >> From: Magnus.Svensson@stripped [mailto:Magnus.Svensson@stripped]
> >> Sent: Monday, September 15, 2008 9:22 PM
> >> To: Vladislav Vaintroub; magnus 'msvensson' Svensson
> >> Cc: commits@stripped
> >> Subject: Re: bzr commit into mysql-5.1-bugteam branch
> (vvaintroub:2678)
> >> Bug#35987
> >>
> >> + char *p= strrchr(module_dir,'\\');
> >> + if (!p)
> >> + {
> >> + /*
> >> + Path separator was not found. Not known to happen, if
> ever
> > happens,
> >> + will indicate current directory.
> >> + */
> >> + module_dir[0]= '.';
> >> + p= module_dir + 1;
> >> + }
> >> magnus: still feels like this could potentially go wrong. Will look
> more
> >> tomorrow. But maybe I'm just overly careful?
> >
> > My guess is overly careful, and this is actually good :)
> >
> > Maybe it helps to know which cases I have tested before claiming
> there is no
> > possibility to have relative paths in Module32First/Next and that
> unix path
> > separators are translated to Windows.
> >
> > I created a dummy DLL in C:\tools. I created a new standalone
> application
> > that does LoadLibrary and get_symbol_path after it and looked if the
> output
> > looks ok.
> > I tried
> > 1) LoadLibrary("mydll.dll") (with PATH previously set to
> "%PATH%;C:\tools")
> > 2) LoadLibrary ("C:\\tools\\mydll.dll")
> > 3) LoadLibrary("C:/tools/mydll.dll")
> >
> > In all cases, after load library the Module32Next returned
> > "C:\tools\mydll.dll" in mod.szExePath.
> >
> > I repeated 1) with mydll.dll copied in the same directory as the test
> > program , and also after that mod.szExePath was an absolute path.
> >
>
> Looks good. So you are basically saying that there is no way we can get
> p==NULL after strrchr?
>
> What I was suspicious about is:
>
> 1. to modify the data that "module_dir" is pointing to, are we sure
> it's
> pointing to a a buffer we can modify? But I don't see how MODULEENTRY32
> is defined, so maybe there is a big buffer in there ie that should be
> safe.
module_dir points to a big buffer we can modify, it points to a mod.szExe ,
which is char[MAX_PATH], according to MODULEENTRY32 definition here
http://msdn.microsoft.com/en-us/library/ms684225(VS.85).aspx
mod is allocated on stack, hence the char array also on our stack, thus we
can modify it.
> 2. to modify two bytes in the string that "p" points to. The last "\"
> could potentially point to the last position in that buffer and thus,
> the second assignment of '\0' would be written after end of buffer.
I assume mod.szExe is NULL-terminated (sz prefix in Hungarian notation is
string zero-terminated), also Microsoft examples like this one
http://msdn.microsoft.com/en-us/library/ms686849(VS.85).aspx use %s with
printf to output it.
Under this assumption it is safe to do strrchr() on it (would not be
generally possible with zero-terminated buffer). Even if backslash is the
last char before '\0', the code
char *p= strrchr(module_dir,'\\');
if (p)
{
*p++= ';';
*p= '\0';
}
would replace backslash with ';' and '\0' with '\0', and we will not
overwrite the end of buffer.
> I would suggest:
> char *module_dir= mod.szExePath;
> char *p= strrchr(module_dir,'\\');
> if (p)
> *p= '\0'; // Truncate string at last \
>
> if (!strstr(path, module_dir))
> {
> strncat(path, module_dir, size);
> strcat(path, ";");
> size-= strlen(module_dir)+1; // << From 3) below
> }
Well, this one has a small problem.
Assume you have an executable in C:\dir1 and a DLL in C:\dir.
The symbol path should be C:\dir1;C:\dir; but will only be C:\dir1 in your
variant
The difference is in the second parameter to strstr - it should be
';'-terminated, otherwise
strstr("C:\dir1;","C:\dir") will return not-null and C:\dir won't be
appended.
> 3. I also just remembered that the size argument for strncat actually
> means how many chars to take from the src string, thus it's .actually
> not a way of preventing us from writing beyond end of "path".
True, thanks a lot for reminding me this.
