Re: [PATCH 0/2] Testsuite adjustment and relevant fix
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 , 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);
Re: [PATCH 0/2] Testsuite adjustment and relevant fix
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 , 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
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 , 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
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
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
