Re: bzr commit into mysql-5.1-bugteam branch (vvaintroub:2678) Bug#35987

[Magnus Svensson]

Vladislav Vaintroub skrev:
> 
>> -----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. 
> 

Yes, this is true.

The patch you wrote looks fine. Altough it woud have been nice with 
"strncat_s" :)

/ Magnus