Re: krnl386.exe16: Do not reassign default handles after they got closed.

[Alexandre Julliard]

Sebastian Lackner <sebastian(a)fds-team.de> writes:
> @@ -363,7 +363,7 @@ HFILE16 WINAPI _lclose16( HFILE16 hFile )
>      }
>      TRACE("%d (handle32=%p)\n", hFile, dos_handles[hFile] );
>      CloseHandle( dos_handles[hFile] );
> -    dos_handles[hFile] = 0;
> +    dos_handles[hFile] = (hFile < 5) ? INVALID_HANDLE_VALUE : 0;

This will cause functions that take default handles to not return the
correct errors. Is that really how it's supposed to work? What bug is
this for?

-- 
Alexandre Julliard
julliard(a)winehq.org

krnl386.exe16: Do not reassign default handles after they got closed.

[Sebastian Lackner]

On 06.04.2016 05:50, Alexandre Julliard wrote:
> Sebastian Lackner <sebastian(a)fds-team.de> writes:
> > @@ -363,7 +363,7 @@ HFILE16 WINAPI _lclose16( HFILE16 hFile )
> >      }
> >      TRACE("%d (handle32=%p)\n", hFile, dos_handles[hFile] );
> >      CloseHandle( dos_handles[hFile] );
> > -    dos_handles[hFile] = 0;
> > +    dos_handles[hFile] = (hFile < 5) ? INVALID_HANDLE_VALUE : 0;
>
> This will cause functions that take default handles to not return the
> correct errors. Is that really how it's supposed to work? What bug is
> this for?

As far as I remember it basically solves two bugs at once:

* First of all, apps interpret a returned HFILE of 0 from file APIs as failure.
  This can happen when apps call _lclose(0) before, or when FILE_InitProcessDosHandles
  does not properly initialize them. The second case, failure to
  initialize dos_handles[*] during init, will be fixed in a separate patch.

* I also remember that Michael ran various tests to verify the return value
  of _lclose(*), and it showed that Windows returns 0 when trying to close
  such handle multiple times. I am aware that we still call CloseHandle() on
  INVALID_HANDLE_VALUE in Wine, but this is probably well-defined enough to
  avoid additional checks.

@ Michael: Could you please double-check that my statements above are correct?
If you still have your tests, please also attach them for further review.

Regards,
Sebastian

krnl386.exe16: Do not reassign default handles after they got closed.

[Alexandre Julliard]

Sebastian Lackner <sebastian(a)fds-team.de> writes:
> As far as I remember it basically solves two bugs at once:
>
> * First of all, apps interpret a returned HFILE of 0 from file APIs as failure.
>   This can happen when apps call _lclose(0) before, or when FILE_InitProcessDosHandles
>   does not properly initialize them. The second case, failure to
>   initialize dos_handles[*] during init, will be fixed in a separate patch.
>
> * I also remember that Michael ran various tests to verify the return value
>   of _lclose(*), and it showed that Windows returns 0 when trying to close
>   such handle multiple times. I am aware that we still call CloseHandle() on
>   INVALID_HANDLE_VALUE in Wine, but this is probably well-defined enough to
>   avoid additional checks.

I'm concerned that using two different markers for invalid handles is
confusing. There are already more than enough 0 vs. INVALID_HANDLE_VALUE
mismatches ;-)

I think it would be better to set them to 0 on close, and handle that
case explicitly in all functions.

-- 
Alexandre Julliard
julliard(a)winehq.org