Re: [PATCH 0/2] Testsuite adjustment and relevant fix

[Jon Turney <jon.turney@dronecode.org.uk>]

On 20/07/2023 16:14, Corinna Vinschen wrote:
> On Jul 20 13:55, Jon Turney wrote:
>> On 19/07/2023 16:33, Corinna Vinschen wrote:
>>> On Jul 19 13:41, Jon Turney wrote:
>>>> [1/2] has the side effect of flipping test stat06 from working to failing.
>>>> [2/2] fixes that
>>>>
>>>> When run with TDIRECTORY set, libltp just uses that directory and assumes
>>>> something else will clean it up.
>>>>
>>>> When TDIRECTORY is not set, libltp creates a subdirectory under /tmp, and when
>>>> the test is completed, removes the expected files and verifies that the
>>>> directory is empty.
>>>>
>>>> stat06 fails that check, because it creates the a file named "file" there, and
>>>> tries stat("file", -1), testing that it returns the expected value EFAULT.
>>>>
>>>> "file" is removed, but lingers in the STATUS_DELETE_PENDING state until the
>>>> Windows handle which stat_worker() leaks when an exception occurs is closed
>>>> (when the processes exits).
>>>
>>> Great find. Please push.
>>
>> So, it seems this doesn't work in an optimized build, as fh is always NULL
>> when we get around to deleting it after a fault.
>>
>> I'm thinking that I've written this wrong somehow (horses), rather than it
>> being some complex problem with how the optimizer interacts with all the
>> memory and register barriers the exception handling uses (zebras)
> 
> What if you turn around the order instead?

Yes, this works.

This is how I wrote it initially, in fact.  This is perhaps slightly 
less good, because it still has that leak if the fh method calls throw 
an exception (which should never happen :)), but has the advantage of 
actually working.

> diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
> index 73343ecc1f07..32ace4d38943 100644
> --- a/winsup/cygwin/syscalls.cc
> +++ b/winsup/cygwin/syscalls.cc
> @@ -1967,12 +1967,13 @@ stat_worker (path_conv &pc, struct stat *buf)
>   	{
>   	  fhandler_base *fh;
>   
> -	  if (!(fh = build_fh_pc (pc)))
> -	    __leave;
> -
>   	  debug_printf ("(%S, %p, %p), file_attributes %d",
>   			pc.get_nt_native_path (), buf, fh, (DWORD) *fh);
>   	  memset (buf, 0, sizeof (*buf));
> +
> +	  if (!(fh = build_fh_pc (pc)))
> +	    __leave;
> +
>   	  res = fh->fstat (buf);
>   	  if (!res)
>   	    fh->stat_fixup (buf);
>