[PATCH 0/2] Testsuite adjustment and relevant fix

[PATCH 0/2] Testsuite adjustment and relevant fix

[Jon Turney]

[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).

Future work: It looks like similar problems might generically occur in similar
code througout syscalls.cc.

Jon Turney (2):
  Cygwin: testsuite: Drop setting TDIRECTORY
  Cygwin: Fix Windows file handle leak in stat("file", -1)

 winsup/cygwin/syscalls.cc              | 6 +++---
 winsup/testsuite/Makefile.am           | 8 --------
 winsup/testsuite/cygrun.c              | 5 +----
 winsup/testsuite/winsup.api/winsup.exp | 4 +---
 4 files changed, 5 insertions(+), 18 deletions(-)

-- 
2.39.0

[PATCH 1/2] Cygwin: testsuite: Drop setting TDIRECTORY

[Jon Turney]

Drop setting TDIRECTORY, just use /tmp in the 'test installation' now
that we have it.

This effectively reverts f3ed5f2fe029d74372aca68b18936e164ff47cf7

Signed-off-by: Jon Turney <jon.turney@dronecode.org.uk>
---
 winsup/testsuite/Makefile.am           | 8 --------
 winsup/testsuite/cygrun.c              | 5 +----
 winsup/testsuite/winsup.api/winsup.exp | 4 +---
 3 files changed, 2 insertions(+), 15 deletions(-)

diff --git a/winsup/testsuite/Makefile.am b/winsup/testsuite/Makefile.am
index 60111a0aa..9159a1be8 100644
--- a/winsup/testsuite/Makefile.am
+++ b/winsup/testsuite/Makefile.am
@@ -334,17 +334,9 @@ DEJATOOL = winsup
 RUNTESTFLAGS_1 = -v
 RUNTESTFLAGS = $(RUNTESTFLAGS_$(V))
 
-# a temporary directory, to be used for files created by tests
-tmpdir = $(abspath $(builddir)/tmp/)
-# the same temporary directory, as an absolute, /cygdrive path (so it can be
-# understood by the test DLL, which will have a different mount table)
-testdll_tmpdir = $(shell cygpath -ma $(tmpdir) | sed -e 's#^\([A-Z]\):#/cygdrive/\L\1#')
-
 site-extra.exp: ../config.status Makefile
 	@rm -f ./tmp0
 	@echo "set runtime_root \"`pwd`/testinst/bin\"" >> ./tmp0
-	@echo "set tmpdir $(tmpdir)" >> ./tmp0
-	@echo "set testdll_tmpdir $(testdll_tmpdir)" >> ./tmp0
 	@echo "set cygrun \"`pwd`/mingw/cygrun\"" >> ./tmp0
 	@mv ./tmp0 site-extra.exp
 
diff --git a/winsup/testsuite/cygrun.c b/winsup/testsuite/cygrun.c
index 925b5513f..d8de7d158 100644
--- a/winsup/testsuite/cygrun.c
+++ b/winsup/testsuite/cygrun.c
@@ -26,13 +26,10 @@ main (int argc, char **argv)
 
   if (argc < 2)
     {
-      fprintf (stderr, "Usage: cygrun [program] [tmpdir]\n");
+      fprintf (stderr, "Usage: cygrun [program]\n");
       exit (0);
     }
 
-  if (argc >= 3)
-    SetEnvironmentVariable ("TDIRECTORY", argv[2]);
-
   SetEnvironmentVariable ("CYGWIN_TESTING", "1");
 
   memset (&sa, 0, sizeof (sa));
diff --git a/winsup/testsuite/winsup.api/winsup.exp b/winsup/testsuite/winsup.api/winsup.exp
index 111509511..76455a97c 100644
--- a/winsup/testsuite/winsup.api/winsup.exp
+++ b/winsup/testsuite/winsup.api/winsup.exp
@@ -62,10 +62,8 @@ foreach src [lsort [glob -nocomplain $srcdir/$subdir/*.c $srcdir/$subdir/*/*.{cc
 	    } else {
 	       set redirect_output /dev/null
 	    }
-	    file mkdir $tmpdir/$tmpfile
 	    set env(PATH) "$runtime_root:$env(PATH)"
-	    ws_spawn "cygdrop $cygrun $exec $testdll_tmpdir/$tmpfile > $redirect_output"
-	    file delete -force $tmpdir/$tmpfile
+	    ws_spawn "cygdrop $cygrun $exec > $redirect_output"
 	    set env(PATH) "$orig_path"
 	    if { $rv } {
 		fail "$testcase"
-- 
2.39.0

[PATCH 2/2] Cygwin: Fix Windows file handle leak in stat("file", -1)

[Jon Turney]

Don't leak a Windows file handle if stat() is called with a valid
filename, but invalid stat buffer pointer.

We do not destroy fh if an exception happens in the __try block, which
closes a Windows handle it has opened.

Fixes: 73151c54d581 ("syscalls.cc (stat_worker): Don't call build_fh_pc with invalid pc.")
Signed-off-by: Jon Turney <jon.turney@dronecode.org.uk>
---
 winsup/cygwin/syscalls.cc | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
index 73343ecc1..c6999407e 100644
--- a/winsup/cygwin/syscalls.cc
+++ b/winsup/cygwin/syscalls.cc
@@ -1955,6 +1955,7 @@ int
 stat_worker (path_conv &pc, struct stat *buf)
 {
   int res = -1;
+  fhandler_base *fh = NULL;
 
   __try
     {
@@ -1965,8 +1966,6 @@ stat_worker (path_conv &pc, struct stat *buf)
 	}
       else if (pc.exists ())
 	{
-	  fhandler_base *fh;
-
 	  if (!(fh = build_fh_pc (pc)))
 	    __leave;
 
@@ -1976,13 +1975,14 @@ stat_worker (path_conv &pc, struct stat *buf)
 	  res = fh->fstat (buf);
 	  if (!res)
 	    fh->stat_fixup (buf);
-	  delete fh;
 	}
       else
 	set_errno (ENOENT);
     }
   __except (EFAULT) {}
   __endtry
+
+  delete fh;
   syscall_printf ("%d = (%S,%p)", res, pc.get_nt_native_path (), buf);
   return res;
 }
-- 
2.39.0

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

[Corinna Vinschen]

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.

> Future work: It looks like similar problems might generically occur in similar
> code througout syscalls.cc.

Uh oh...


Corinna

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

[Jon Turney]

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)

>> Future work: It looks like similar problems might generically occur in similar
>> code througout syscalls.cc.
> 
> Uh oh...

I mean, I have no evidence that there are any problems.

If I had infinite time, maybe I'd review the source code to see if there 
are any other instances where we fail to properly dispose of objects 
created in a __try block...

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

[Corinna Vinschen]

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?

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);


> If I had infinite time, maybe I'd review the source code to see if there are
> any other instances where we fail to properly dispose of objects created in
> a __try block...

I like the idea of infinite time...


Corinna

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

[Corinna Vinschen]

On Jul 20 17: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?
> 
> 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));

Maybe adding a MemoryBarrier() call here if all else fails...

> +
> +	  if (!(fh = build_fh_pc (pc)))
> +	    __leave;
> +
>  	  res = fh->fstat (buf);
>  	  if (!res)
>  	    fh->stat_fixup (buf);

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

[Jon Turney]

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);
>