x11drv: XDND: Fix file drop to properly support file:/// URIs (debugging problem)

[Maciej Katafiasz]

Hi,

(please ignore the other copy I sent from the wrong address by accident)

I'm working on a bug in winex11drv/xdnd.c, where file:/// URIs are not
correctly interpreted, resulting in DnD broken most of the time. There
are two parts of that bug:

1) X11DRV_XDND_BuildDropFiles() fails to properly handle absolute unix
paths, which is exactly what you get from file:///, so it won't find the
file unless $PWD and the constructed Windows drive-absolute path happen
to have the same root. For example, file:///home/mathrick/foo.txt will
yield "\\home\\mathrick\\foo.txt" and work only if the current directory
is on a drive letter on which that path is valid.

2) X11DRV_XDND_DeconstructTextPlain() fails to accommodate for the fact
file:/// URIs are, in fact, URIs and not paths, and doesn't check for
validity nor does it unescape % sequences. The result is that paths with
spaces, etc. in them won't work.

The attached patch is my first take at 1), it works, but there is a
problem. My test application is μTorrent, and it'll randomly lock up
with the following errors printed:

err:ntdll:RtlpWaitForCriticalSection section 0x110020 "heap.c: main
process heap section" wait timed out in thread 001f, blocked by 0019,
retrying (60 sec)
err:ntdll:RtlpWaitForCriticalSection section 0x110020 "heap.c: main
process heap section" wait timed out in thread 001e, blocked by 0019,
retrying (60 sec)

I can reproduce them by attempting to drop a file on µT's window right
after startup. If I wait a while, it works. It also happens if I switch
to another virtual desktop, then back to the one µT is on and then try
to drop.

I tried to check the code as well as I could (win32 API is not exactly
my everyday environment), and I believe it's correct. At least all the
allocations appear to be correct, nothing is overwritten. I've tried
debugging in several ways using winedbg, but it fails pretty hard with
various exceptions that aren't there when running outside the debugger,
or fails to attach to the process, etc. From the circumstances, I
believe there's some kind of race condition in allocation from process
heap. However, I'm unable to find it as I have no idea about wine
internals and how exactly DnD happens, and what threads do what and
when. I'd be therefore very grateful if someone knowledgeable would take
a look at it and see what's wrong, or at least help me with debugging
the issue, as my ideas how to attack it have run out.

Cheers,

-- 
Being really good at C++ is like being really good at using rocks to
sharpen sticks. (Thant Tessman)
--------
Maciej Katafiasz <ml(a)mathrick.org>
http://mathrick.org

x11drv: XDND: Fix file drop to properly support file:/// URIs(debugging problem)

[Dmitry Timoshkov]

Maciej Katafiasz <ml(a)mathrick.org> wrote:
> +     if(!strncasecmp(p, "://localhost/", 13))
> +     {
> +       i = 12;
> +     } else if (!strncasecmp(p, ":///", 4))
> +     {
> +       i = 3;
> +     } else
> +     {
> +       TRACE("Not a valid file: URI: %s\n", (char*)data);
> +       return count;
> +     }

Please use the same indentation style as the existing does, i.e. 4 spaces,
not 2.
> +    pathlen = MultiByteToWideChar(CP_UNIXCP, 0, filename, len, NULL, 0);
> +
> +    wfn = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, pathlen);

You allocate not enough space for a wide char string.
> +    TRACE("Convert to WCHAR: filename: %s, len: %d\n", debugstr_a(filename), pathlen);
> +    pathlen = MultiByteToWideChar(CP_UNIXCP, 0, filename, len, wfn, pathlen);
> +    if(GetLastError())
> +    {
> +      TRACE("Can't convert to WCHAR: %d\n", GetLastError());
> +      goto clean_wfn;
> +    }

This is not an appropriate way of testing for an API failure.
> +    TRACE("WCHAR filename: %s\n", debugstr_w(wfn));
> +    wpath = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, pathlen * sizeof(WCHAR));

Memory space calculations and allocations don't look right all over the place.
> -        strcpy(((char*)lpDrop)+lpDrop->pFiles, path);
> +        memcpy(((char*)lpDrop)+lpDrop->pFiles, (char*)wpath, fullpathlen * sizeof(WCHAR));

Use lstrcpyW here instead of memcpy.

-- 
Dmitry.

x11drv: XDND: Fix file drop to properly support file:/// URIs(debugging problem)

[Robert Shearman]

Dmitry Timoshkov wrote:
> > -        strcpy(((char*)lpDrop)+lpDrop->pFiles, path);
> > +        memcpy(((char*)lpDrop)+lpDrop->pFiles, (char*)wpath, 
> > fullpathlen * sizeof(WCHAR));
>
> Use lstrcpyW here instead of memcpy.

Why? Maciej has already got the length, so why not do the appropriate 
calculation on fullpathlen and use memcpy?

-- 
Rob Shearman

APPDB now appears to be working at normal acceptable speed for administrator login's + backlog of APPDB data requiring approval.

[Nick Law]

Thanks go out to whoever it was that fixed the AppDB so that it's now 
running nice and quick again. I can now login and submit 
applications/tests etc. There's a backlog of 500+ submissions which will 
take 2-4 hours to clear, hopefully this should be cleared within the 
next few days.

Nick

x11drv: XDND: Fix file drop to properly support file:/// URIs(debugging problem)

[Maciej Katafiasz]

On tor, 2007-03-22 at 12:02 +0800, Dmitry Timoshkov wrote:
> Please use the same indentation style as the existing does, i.e. 4 spaces,
> not 2.

Yep, I didn't fix that in that patch, I just forgot to set the indent
style properly, and it's not a patch ready for submission anyway. But
noted.
> > +    pathlen = MultiByteToWideChar(CP_UNIXCP, 0, filename, len, NULL, 0);
> > +
> > +    wfn = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, pathlen);
>
> You allocate not enough space for a wide char string.

Æh, my bad, I blame it on the confusing WCHAR vs. bytes semantics. I
thought it was returned in bytes, now I see that particular one is
WCHARs.
> > +    TRACE("Convert to WCHAR: filename: %s, len: %d\n", debugstr_a(filename), pathlen);
> > +    pathlen = MultiByteToWideChar(CP_UNIXCP, 0, filename, len, wfn, pathlen);
> > +    if(GetLastError())
> > +    {
> > +      TRACE("Can't convert to WCHAR: %d\n", GetLastError());
> > +      goto clean_wfn;
> > +    }
>
> This is not an appropriate way of testing for an API failure.

What is inappropriate and how should I fix it?
> > +    TRACE("WCHAR filename: %s\n", debugstr_w(wfn));
> > +    wpath = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, pathlen * sizeof(WCHAR));
>
> Memory space calculations and allocations don't look right all over the place.

Apart from the one above, I couldn't find any mistakes. I tried looking
for WCHARs vs. bytes too.
> > -        strcpy(((char*)lpDrop)+lpDrop->pFiles, path);
> > +        memcpy(((char*)lpDrop)+lpDrop->pFiles, (char*)wpath, fullpathlen * sizeof(WCHAR));
>
> Use lstrcpyW here instead of memcpy.

Is there any particular benefit of doing so, when I have the size known
beforehand?

Cheers,

-- 
There are only two things wrong with C++: The initial concept and the
implementation. (Bertrand Meyer)
--------
Maciej Katafiasz <email(a)mathrick.org>
http://mathrick.org

x11drv: XDND: Fix file drop to properly support file:/// URIs(debugging problem)

[Felix Nawothnig]

Maciej Katafiasz wrote:
> > > +    if(GetLastError())
> > > +    {
> > > +      TRACE("Can't convert to WCHAR: %d\n", GetLastError());
> > > +      goto clean_wfn;
> > > +    }
> > > This is not an appropriate way of testing for an API failure.
> > > What is inappropriate and how should I fix it?

Most APIs don't change GLE on success. Some do but most often the 
behaviour is undocumented or changed between 9x/NT. You should check the 
return value which usually is either 0 or -1 (depends on the API) on 
failure.

Felix