Re: msvcrt: dont overwrite the standard streams (with test, try 5)

[Eric Pouech]

André Hentschel a écrit :
> Some Apps e.g. mingw-apps use lpReserved2 to pass some data to the child-process, this data starts with 4 Byte of Zeros(unsinged 0) to tell msvcrt not to read that as an inherit-block.

Hi André
a couple of comments to your patch
> See also Bug 18479
> ---
>  dlls/msvcrt/file.c            |    2 +-
>  dlls/msvcrt/tests/Makefile.in |    2 +-
>  dlls/msvcrt/tests/file.c      |   50 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 52 insertions(+), 2 deletions(-)
>
> diff --git a/dlls/msvcrt/file.c b/dlls/msvcrt/file.c
> index 6671f2f..8f63e8d 100644
> --- a/dlls/msvcrt/file.c
> +++ b/dlls/msvcrt/file.c
> @@ -349,7 +349,7 @@ void msvcrt_init_io(void)
>    InitializeCriticalSection(&MSVCRT_file_cs);
>    MSVCRT_file_cs.DebugInfo->Spare[0] = (DWORD_PTR)(__FILE__ ": MSVCRT_file_cs");
>    GetStartupInfoA(&si);
> -  if (si.cbReserved2 != 0 && si.lpReserved2 != NULL)
> +  if (si.cbReserved2 != 0 && si.lpReserved2 != NULL && *(unsigned*)si.lpReserved2)

IMO, we should also test that cbReserved2 is >= sizeof(unsigned) 
otherwise we'd be in trouble
> {
>      char*       wxflag_ptr;
>      HANDLE*     handle_ptr;
> diff --git a/dlls/msvcrt/tests/Makefile.in b/dlls/msvcrt/tests/Makefile.in
> index 74199e1..8c71971 100644
> --- a/dlls/msvcrt/tests/Makefile.in
> +++ b/dlls/msvcrt/tests/Makefile.in
> @@ -4,7 +4,7 @@ SRCDIR    = @srcdir@
>  VPATH     = @srcdir@
>  TESTDLL   = msvcrt.dll
>  APPMODE   = -mno-cygwin
> -IMPORTS   = kernel32
> +IMPORTS   = kernel32 user32

you should do it without user32. instead of doing this
> +    while (MsgWaitForMultipleObjects( 1, &proc.hProcess, FALSE, INFINITE, QS_ALLINPUT ) != 0)
> +    {
> +        while (PeekMessage(&msg, 0, 0, 0, PM_REMOVE)) DispatchMessageA(&msg);
> +    }

you could simply wait for the child process to terminate
A+

-- 
Eric Pouech
"The problem with designing something completely foolproof is to underestimate the ingenuity of a complete idiot." (Douglas Adams)

msvcrt: dont overwrite the standard streams (with test, try 5)

[André Hentschel]

Eric Pouech schrieb:
> Hi André
> a couple of comments to your patch
> always welcome!
> IMO, we should also test that cbReserved2 is >= sizeof(unsigned)
> otherwise we'd be in trouble
> maybe you had something like that patch in mind:
> http://www.winehq.org/pipermail/wine-patches/2009-August/077766.html
> it checks if the size makes sense
> you should do it without user32. instead of doing this
> > +    while (MsgWaitForMultipleObjects( 1, &proc.hProcess, FALSE,
> > INFINITE, QS_ALLINPUT ) != 0)
> > +    {
> > +        while (PeekMessage(&msg, 0, 0, 0, PM_REMOVE))
> > DispatchMessageA(&msg);
> > +    }
>
> you could simply wait for the child process to terminate
> A+

of course, your right, fixed that.

-- 

Best Regards, André Hentschel

msvcrt: dont overwrite the standard streams (with test, try 5)

[Eric Pouech]

André Hentschel a écrit :
> Eric Pouech schrieb:
> > Hi André
> > a couple of comments to your patch
>
> always welcome!
> > IMO, we should also test that cbReserved2 is >= sizeof(unsigned)
> > otherwise we'd be in trouble
>
> maybe you had something like that patch in mind:
> http://www.winehq.org/pipermail/wine-patches/2009-August/077766.html
> it checks if the size makes sense

no
I'm just saying that since we do something like *(unsigned*)lpReserved, 
we'd better check that cbReserved2 >= sizeof(unsigned)
I don't think that your patch is correct. If you want a coding of the 
"reserved" block, that you can extend, I think you could well have 
additional information *after* the block of handles+flags we're considering

therefore the valid tests I see would be:
cdReserved2 >= sizeof(unsigned)
numH = *(unsigned*)lpReserved2
error if sizeof(unsigned) + numH*(1+sizeof(HANDLE)) > cbReserved2

A+


-- 
Eric Pouech
"The problem with designing something completely foolproof is to underestimate the ingenuity of a complete idiot." (Douglas Adams)

msvcrt: dont overwrite the standard streams (with test, try 6)

[André Hentschel]

Eric Pouech schrieb:
> André Hentschel a écrit :
> > Eric Pouech schrieb:
> > > Hi André
> > > a couple of comments to your patch
> >
> > always welcome!
> > > IMO, we should also test that cbReserved2 is >= sizeof(unsigned)
> > > otherwise we'd be in trouble
> >
> > maybe you had something like that patch in mind:
> > http://www.winehq.org/pipermail/wine-patches/2009-August/077766.html
> > it checks if the size makes sense
>
> no
> I'm just saying that since we do something like *(unsigned*)lpReserved,
> we'd better check that cbReserved2 >= sizeof(unsigned)
> I don't think that your patch is correct. If you want a coding of the
> "reserved" block, that you can extend, I think you could well have
> additional information *after* the block of handles+flags we're considering
>
> therefore the valid tests I see would be:
> cdReserved2 >= sizeof(unsigned)
> numH = *(unsigned*)lpReserved2
> error if sizeof(unsigned) + numH*(1+sizeof(HANDLE)) > cbReserved2
>
> A+

I already understood the problem you mentioned and sent try 6: http://www.winehq.org/pipermail/wine-patches/2009-September/078158.html
So for now this patch fixes a Problem, if we need it we also could check if the calculated size is bigger than the given one. (You were right with that, too)

-- 

Best Regards, André Hentschel