Re: kernel32: WideCharToMultiByte: return error on negative dest len (take 2)

[Alexandre Julliard]

"Dan Kegel"  writes:
> While investigating a crash in gs-auftrag in bug 9039, noticed
> WideCharToMultiByte wasn't quite conformant for negative destlen.
> Fixed, with tests.  Passes for me in Wine and Win XP.

This would require reviewing all uses of WideCharToMultiByte in Wine,
many places get this wrong. Unless you have an app that requires this
fix I'd suggest to leave the test as todo_wine for now.

kernel32: WideCharToMultiByte: return error on negative dest len (take 2)

[Michael Stefaniuc]

Alexandre Julliard wrote:
> "Dan Kegel"  writes:
> > While investigating a crash in gs-auftrag in bug 9039, noticed
> > WideCharToMultiByte wasn't quite conformant for negative destlen.
> > Fixed, with tests.  Passes for me in Wine and Win XP.
>
> This would require reviewing all uses of WideCharToMultiByte in Wine,
> many places get this wrong. Unless you have an app that requires this
> fix I'd suggest to leave the test as todo_wine for now.

Would that be a good janitorial work to be done before Wine 1.0? I feel
like I'm getting tired of translating ...

bye
    michael

kernel32: WideCharToMultiByte: return error on negative dest len (take 2)

[Alexandre Julliard]

Michael Stefaniuc  writes:
> Would that be a good janitorial work to be done before Wine 1.0? I feel
> like I'm getting tired of translating ...

Sure, if you feel like doing it go ahead. I'm not sure the fix will go
in before 1.0, but already fixing as many callers as possible certainly
can't hurt.

kernel32: WideCharToMultiByte: return error on negative dest len (take 2)

[Dan Kegel]

On Wed, Mar 26, 2008 at 4:04 AM, Michael Stefaniuc  wrote:
> > This would require reviewing all uses of WideCharToMultiByte in Wine,
> > many places get this wrong. Unless you have an app that requires this
> > fix I'd suggest to leave the test as todo_wine for now.
>
> Would that be a good janitorial work to be done before Wine 1.0? I feel
>  like I'm getting tired of translating ...

Sure.  A quick grep finds some obvious ones:

$ find . -name '*.c' | xargs grep WideCharToMultiByte | grep ' -1, NULL, NULL)'
./wininet/urlcache.c:        WideCharToMultiByte(CP_ACP, 0,
lpszLocalFileName, -1, achFile, -1, NULL, NULL);
./mshtml/editor.c:        WideCharToMultiByte(CP_ACP, 0, V_BSTR(in),
-1, stra, -1, NULL, NULL);
./mshtml/persist.c:            WideCharToMultiByte(CP_ACP, 0, headers,
-1, data, -1, NULL, NULL);
./mshtml/install.c:    WideCharToMultiByte(CP_ACP, 0, file_name, -1,
file_name_a, -1, NULL, NULL);
./mshtml/nsio.c:            WideCharToMultiByte(CP_ACP, 0,
container->doc->mime, -1, This->content, -1, NULL, NULL);
./mshtml/navigate.c:        WideCharToMultiByte(CP_ACP, 0,
szStatusText, -1, This->nschannel->content, -1, NULL, NULL);
./advapi32/cred.c:        string_len = WideCharToMultiByte(CP_ACP, 0,
CredentialW->TargetName, -1, CredentialA->TargetName, -1, NULL, NULL);
./advapi32/cred.c:        string_len = WideCharToMultiByte(CP_ACP, 0,
CredentialW->Comment, -1, CredentialA->Comment, -1, NULL, NULL);
./advapi32/cred.c:        string_len = WideCharToMultiByte(CP_ACP, 0,
CredentialW->TargetAlias, -1, CredentialA->TargetAlias, -1, NULL,
NULL);
./advapi32/cred.c:        string_len = WideCharToMultiByte(CP_ACP, 0,
CredentialW->UserName, -1, CredentialA->UserName, -1, NULL, NULL);
./urlmon/sec_mgr.c:    WideCharToMultiByte(CP_ACP, 0, url, -1,
(LPSTR)pbSecurityId, -1, NULL, NULL)

(I peeked at that last one.  The code is enforcing the length limit itself and
then telling WideCharToMultiByte that there's no limit.  May as well
pass the proper
limit to WideCharToMultiByte and let it do the length checking?)

The first step, as Alexandre says, is to add the test with a todo_wine
around the negative destlen test.
I would have done it but I'm more than happy to let you run with it.

I haven't quite figured out the fix for bug 9039, this is just something
I noticed along the way.
- Dan

kernel32: WideCharToMultiByte: return error on negative dest len (take 2)

[Michael Stefaniuc]

Dan Kegel wrote:
> On Wed, Mar 26, 2008 at 4:04 AM, Michael Stefaniuc  wrote:
> > > This would require reviewing all uses of WideCharToMultiByte in Wine,
> > > many places get this wrong. Unless you have an app that requires this
> > > fix I'd suggest to leave the test as todo_wine for now.
> >
> > Would that be a good janitorial work to be done before Wine 1.0? I feel
> >  like I'm getting tired of translating ...
>
> Sure.  A quick grep finds some obvious ones:
>
> $ find . -name '*.c' | xargs grep WideCharToMultiByte | grep ' -1, NULL, NULL)'
> The first step, as Alexandre says, is to add the test with a todo_wine
> around the negative destlen test.
> I would have done it but I'm more than happy to let you run with it.

Please go ahead and resubmit the patch with todo_wine around it. I don't
want to claim ownership for the patch you wrote.

bye
    michael

kernel32: WideCharToMultiByte: return error on negative dest len (take 2)

[Dan Kegel]

On Wed, Mar 26, 2008 at 6:38 AM, Michael Stefaniuc  wrote:
> > The first step, as Alexandre says, is to add the test with a todo_wine
> > around the negative destlen test.
> > I would have done it but I'm more than happy to let you run with it.
>
> Please go ahead and resubmit the patch with todo_wine around it.

Done.
> I don't want to claim ownership for the patch you wrote.

Don't worry, Alexandre usually gets that right.

kernel32: WideCharToMultiByte: return error on negative dest len (take 2)

[Alexandre Julliard]

"Dan Kegel"  writes:
> > I don't want to claim ownership for the patch you wrote.
>
> Don't worry, Alexandre usually gets that right.

Maybe, but I don't like it. I really prefer for people to send their own
patches instead of having others do it for them.