Re: [PATCH] dbghelp: Always return a dos path from SymGetLineFromAddr

[Huw Davies]

On Mon, Jan 30, 2017 at 07:04:51AM +0000, Alistair Leslie-Hughes wrote:
> Fixes: https://bugs.winehq.org/show_bug.cgi?id=34687
>
> Bases off the information in MSDN.
>
> https://msdn.microsoft.com/en-us/library/windows/desktop/ms681330%28v=vs.85%...
> "This function returns a pointer to a buffer that may be reused by another function. 
> Therefore, be sure to copy the data returned to another buffer immediately."
> Signed-off-by: Alistair Leslie-Hughes
>
> dlls/dbghelp/symbol.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/dlls/dbghelp/symbol.c b/dlls/dbghelp/symbol.c
> index 08ea834..1638a1e 100644
> --- a/dlls/dbghelp/symbol.c
> +++ b/dlls/dbghelp/symbol.c
> @@ -1435,9 +1435,11 @@ BOOL WINAPI SymGetSymFromName(HANDLE hProcess, PCSTR Name, PIMAGEHLP_SYMBOL Symb
>  BOOL symt_fill_func_line_info(const struct module* module, const struct symt_function* func,
>                                DWORD64 addr, IMAGEHLP_LINE64* line)
>  {
> -
>
> static char path[MAX_PATH];
> struct line_info*   dli = NULL;
> BOOL                found = FALSE;
> int                 i;
> -
>
> WCHAR *dos_path;
>
> assert(func->symt.tag == SymTagFunction);
>
> @@ -1455,7 +1457,10 @@ BOOL symt_fill_func_line_info(const struct module* module, const struct symt_fun
>          }
>          if (found)
>          {
> -
>        line->FileName = (char*)source_get(module, dli->u.source_file);
> -
>        dos_path = wine_get_dos_file_name((char*)source_get(module, dli->u.source_file));
> -
>        WideCharToMultiByte(CP_ACP, 0, dos_path, -1, path, MAX_PATH, NULL, NULL);
> -
>        HeapFree( GetProcessHeap(), 0, dos_path );
> -
>        line->FileName = path;
>        return TRUE;
>    }
> }

This has implications for winedbg that at least need to be discussed.
For example, with this patch, backtraces contain Windows-like paths.
I imagine there will be more serious issues too.

For the patch itself, you now have a Unicode string, so it would make
sense to have symt_fill_func_line_info() return that, then switch the
dependencies so that SymGetLineFromAddr64() calls
SymGetLineFromAddrW64().  To create the ANSI buffer, you probably
want to use fetch_buffer() rather than a static array.

Huw.

[PATCH] dbghelp: Always return a dos path from SymGetLineFromAddr

[Alistair Leslie-Hughes]

Hi Huw,

Thanks for the review.

On 07/02/17 22:17, Huw Davies wrote:
> On Mon, Jan 30, 2017 at 07:04:51AM +0000, Alistair Leslie-Hughes wrote:
>
> This has implications for winedbg that at least need to be discussed.
> For example, with this patch, backtraces contain Windows-like paths.
> I imagine there will be more serious issues too.

I didn't think on how it would affect winedbg. Windows compatible first
then I'll fix the compatible issue with winedbg.
> For the patch itself, you now have a Unicode string, so it would make
> sense to have symt_fill_func_line_info() return that, then switch the
> dependencies so that SymGetLineFromAddr64() calls
> SymGetLineFromAddrW64().  To create the ANSI buffer, you probably
> want to use fetch_buffer() rather than a static array.

This sounds like a good idea.

Best Regards
 Alistair.