[PATCH] Cygwin: Fix the address of myself

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Cygwin: Fix the address of myself

Corinna Vinschen-2
From: Corinna Vinschen <[hidden email]>

Introducing an independent Cygwin PID introduced a regression:

The expectation is that the myself pinfo pointer always points to a
specific address right in front of the loaded Cygwin DLL.

However, commit b5e1003722cb14235c4f166be72c09acdffc62ea "Cygwin:
processes: use dedicated Cygwin PID rather than Windows PID" and commit
88605243a19bbc2b6b9be36b99f513140b972e38 "Cygwin: fix child getting
another pid after spawnve" broke this.  To get myself at the right
address requires to call init with h0 set to INVALID_HANDLE_VALUE or an
existing address:

void
pinfo::init (pid_t n, DWORD flag, HANDLE h0)
{
  [...]
  if (!h0 || myself.h)
    [...]
  else
    {
      shloc = SH_MYSELF;
      if (h0 == INVALID_HANDLE_VALUE)       <-- !!!
        h0 = NULL;
    }

That was not the case anymore after the above commits.  This patch makes
sure to set the handle to INVALID_HANDLE_VALUE again when creating a new
process, so init knows that myself has to be created in the right spot.

While at it, fix a potential uninitialized handle value in
child_info_spawn::handle_spawn.

Signed-off-by: Corinna Vinschen <[hidden email]>
---
 winsup/cygwin/dcrt0.cc | 2 +-
 winsup/cygwin/pinfo.cc | 3 +--
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/winsup/cygwin/dcrt0.cc b/winsup/cygwin/dcrt0.cc
index fb726a739ccf..86ab7256484c 100644
--- a/winsup/cygwin/dcrt0.cc
+++ b/winsup/cygwin/dcrt0.cc
@@ -652,7 +652,7 @@ void
 child_info_spawn::handle_spawn ()
 {
   extern void fixup_lockf_after_exec (bool);
-  HANDLE h;
+  HANDLE h = INVALID_HANDLE_VALUE;
   if (!dynamically_loaded || get_parent_handle ())
       {
  cygheap_fixup_in_child (true);
diff --git a/winsup/cygwin/pinfo.cc b/winsup/cygwin/pinfo.cc
index cdbd8bd7eaf3..b67d660ae04d 100644
--- a/winsup/cygwin/pinfo.cc
+++ b/winsup/cygwin/pinfo.cc
@@ -62,11 +62,10 @@ pinfo::thisproc (HANDLE h)
     {
       cygheap->pid = create_cygwin_pid ();
       flags |= PID_NEW;
+      h = INVALID_HANDLE_VALUE;
     }
   /* spawnve'd process got pid in parent, cygheap->pid has been set in
      child_info_spawn::handle_spawn. */
-  else if (h == INVALID_HANDLE_VALUE)
-    h = NULL;
 
   init (cygheap->pid, flags, h);
   procinfo->process_state |= PID_IN_USE;
--
2.20.1

Reply | Threaded
Open this post in threaded view
|

[PATCH v2] Cygwin: Fix the address of myself

Corinna Vinschen-2
From: Corinna Vinschen <[hidden email]>

v2: rephrase commit message

Introducing an independent Cygwin PID introduced a regression:

The expectation is that the myself pinfo pointer always points to a
specific address right in front of the loaded Cygwin DLL.

However, the independent Cygwin PID changes broke this.  To create
myself at the right address requires to call init with h0 set to
INVALID_HANDLE_VALUE or an existing address:

void
pinfo::init (pid_t n, DWORD flag, HANDLE h0)
{
  [...]
  if (!h0 || myself.h)
    [...]
  else
    {
      shloc = SH_MYSELF;
      if (h0 == INVALID_HANDLE_VALUE)       <-- !!!
        h0 = NULL;
    }

The aforementioned commits changed that so h0 was always NULL, this way
creating myself at an arbitrary address.

This patch makes sure to set the handle to INVALID_HANDLE_VALUE again
when creating a new process, so init knows that myself has to be created
in the right spot.  While at it, fix a potential uninitialized handle
value in child_info_spawn::handle_spawn.

Fixes: b5e1003722cb ("Cygwin: processes: use dedicated Cygwin PID rather than Windows PID")
Fixes: 88605243a19b ("Cygwin: fix child getting another pid after spawnve")
Signed-off-by: Corinna Vinschen <[hidden email]>
---
 winsup/cygwin/dcrt0.cc | 2 +-
 winsup/cygwin/pinfo.cc | 3 +--
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/winsup/cygwin/dcrt0.cc b/winsup/cygwin/dcrt0.cc
index fb726a739ccf..86ab7256484c 100644
--- a/winsup/cygwin/dcrt0.cc
+++ b/winsup/cygwin/dcrt0.cc
@@ -652,7 +652,7 @@ void
 child_info_spawn::handle_spawn ()
 {
   extern void fixup_lockf_after_exec (bool);
-  HANDLE h;
+  HANDLE h = INVALID_HANDLE_VALUE;
   if (!dynamically_loaded || get_parent_handle ())
       {
  cygheap_fixup_in_child (true);
diff --git a/winsup/cygwin/pinfo.cc b/winsup/cygwin/pinfo.cc
index cdbd8bd7eaf3..b67d660ae04d 100644
--- a/winsup/cygwin/pinfo.cc
+++ b/winsup/cygwin/pinfo.cc
@@ -62,11 +62,10 @@ pinfo::thisproc (HANDLE h)
     {
       cygheap->pid = create_cygwin_pid ();
       flags |= PID_NEW;
+      h = INVALID_HANDLE_VALUE;
     }
   /* spawnve'd process got pid in parent, cygheap->pid has been set in
      child_info_spawn::handle_spawn. */
-  else if (h == INVALID_HANDLE_VALUE)
-    h = NULL;
 
   init (cygheap->pid, flags, h);
   procinfo->process_state |= PID_IN_USE;
--
2.20.1

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] Cygwin: Fix the address of myself

Ken Brown-6
On 7/24/2019 12:54 PM, Corinna Vinschen wrote:

> From: Corinna Vinschen <[hidden email]>
>
> v2: rephrase commit message
>
> Introducing an independent Cygwin PID introduced a regression:
>
> The expectation is that the myself pinfo pointer always points to a
> specific address right in front of the loaded Cygwin DLL.
>
> However, the independent Cygwin PID changes broke this.  To create
> myself at the right address requires to call init with h0 set to
> INVALID_HANDLE_VALUE or an existing address:
>
> void
> pinfo::init (pid_t n, DWORD flag, HANDLE h0)
> {
>    [...]
>    if (!h0 || myself.h)
>      [...]
>    else
>      {
>        shloc = SH_MYSELF;
>        if (h0 == INVALID_HANDLE_VALUE)       <-- !!!
>          h0 = NULL;
>      }
>
> The aforementioned commits changed that so h0 was always NULL, this way
> creating myself at an arbitrary address.
>
> This patch makes sure to set the handle to INVALID_HANDLE_VALUE again
> when creating a new process, so init knows that myself has to be created
> in the right spot.  While at it, fix a potential uninitialized handle
> value in child_info_spawn::handle_spawn.
>
> Fixes: b5e1003722cb ("Cygwin: processes: use dedicated Cygwin PID rather than Windows PID")
> Fixes: 88605243a19b ("Cygwin: fix child getting another pid after spawnve")
> Signed-off-by: Corinna Vinschen <[hidden email]>
> ---
>   winsup/cygwin/dcrt0.cc | 2 +-
>   winsup/cygwin/pinfo.cc | 3 +--
>   2 files changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/winsup/cygwin/dcrt0.cc b/winsup/cygwin/dcrt0.cc
> index fb726a739ccf..86ab7256484c 100644
> --- a/winsup/cygwin/dcrt0.cc
> +++ b/winsup/cygwin/dcrt0.cc
> @@ -652,7 +652,7 @@ void
>   child_info_spawn::handle_spawn ()
>   {
>     extern void fixup_lockf_after_exec (bool);
> -  HANDLE h;
> +  HANDLE h = INVALID_HANDLE_VALUE;
>     if (!dynamically_loaded || get_parent_handle ())
>         {
>   cygheap_fixup_in_child (true);
> diff --git a/winsup/cygwin/pinfo.cc b/winsup/cygwin/pinfo.cc
> index cdbd8bd7eaf3..b67d660ae04d 100644
> --- a/winsup/cygwin/pinfo.cc
> +++ b/winsup/cygwin/pinfo.cc
> @@ -62,11 +62,10 @@ pinfo::thisproc (HANDLE h)
>       {
>         cygheap->pid = create_cygwin_pid ();
>         flags |= PID_NEW;
> +      h = INVALID_HANDLE_VALUE;
>       }
>     /* spawnve'd process got pid in parent, cygheap->pid has been set in
>        child_info_spawn::handle_spawn. */
> -  else if (h == INVALID_HANDLE_VALUE)
> -    h = NULL;
>  
>     init (cygheap->pid, flags, h);
>     procinfo->process_state |= PID_IN_USE;
>

I'll be glad to take a close look at this as you asked.  But I'm not familiar
with this part of the code, so it will take me a little time.

Ken
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] Cygwin: Fix the address of myself

Corinna Vinschen-2
On Jul 24 19:11, Ken Brown wrote:

> On 7/24/2019 12:54 PM, Corinna Vinschen wrote:
> > From: Corinna Vinschen <[hidden email]>
> >
> > v2: rephrase commit message
> >
> > Introducing an independent Cygwin PID introduced a regression:
> >
> > The expectation is that the myself pinfo pointer always points to a
> > specific address right in front of the loaded Cygwin DLL.
> >
> > However, the independent Cygwin PID changes broke this.  To create
> > myself at the right address requires to call init with h0 set to
> > INVALID_HANDLE_VALUE or an existing address:
> >
> > void
> > pinfo::init (pid_t n, DWORD flag, HANDLE h0)
> > {
> >    [...]
> >    if (!h0 || myself.h)
> >      [...]
> >    else
> >      {
> >        shloc = SH_MYSELF;
> >        if (h0 == INVALID_HANDLE_VALUE)       <-- !!!
> >          h0 = NULL;
> >      }
> >
> > The aforementioned commits changed that so h0 was always NULL, this way
> > creating myself at an arbitrary address.
> >
> > This patch makes sure to set the handle to INVALID_HANDLE_VALUE again
> > when creating a new process, so init knows that myself has to be created
> > in the right spot.  While at it, fix a potential uninitialized handle
> > value in child_info_spawn::handle_spawn.
> >
> > Fixes: b5e1003722cb ("Cygwin: processes: use dedicated Cygwin PID rather than Windows PID")
> > Fixes: 88605243a19b ("Cygwin: fix child getting another pid after spawnve")
> > Signed-off-by: Corinna Vinschen <[hidden email]>
> > ---
> >   winsup/cygwin/dcrt0.cc | 2 +-
> >   winsup/cygwin/pinfo.cc | 3 +--
> >   2 files changed, 2 insertions(+), 3 deletions(-)
> > [...]
>
> I'll be glad to take a close look at this as you asked.  But I'm not familiar
> with this part of the code, so it will take me a little time.
>
> Ken
Thanks!  I accidentally pushed the patch a few minutes ago when I
was actually just planning to push the ndbm.h patch.  Anyway, I
took the opportunity to create new snapshots with all patches from
yesterday and today, so the getpgrp problems in GDB 8.1.1 and 8.2.1
should both be fixed there as well.

I'd still be glad if the two of you could check if my patch makes
sense as is.


Thanks,
Corinna

--
Corinna Vinschen
Cygwin Maintainer

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] Cygwin: Fix the address of myself

Ken Brown-6
On 7/25/2019 9:37 AM, Corinna Vinschen wrote:

> On Jul 24 19:11, Ken Brown wrote:
>> On 7/24/2019 12:54 PM, Corinna Vinschen wrote:
>>> From: Corinna Vinschen <[hidden email]>
>>>
>>> v2: rephrase commit message
>>>
>>> Introducing an independent Cygwin PID introduced a regression:
>>>
>>> The expectation is that the myself pinfo pointer always points to a
>>> specific address right in front of the loaded Cygwin DLL.
>>>
>>> However, the independent Cygwin PID changes broke this.  To create
>>> myself at the right address requires to call init with h0 set to
>>> INVALID_HANDLE_VALUE or an existing address:
>>>
>>> void
>>> pinfo::init (pid_t n, DWORD flag, HANDLE h0)
>>> {
>>>     [...]
>>>     if (!h0 || myself.h)
>>>       [...]
>>>     else
>>>       {
>>>         shloc = SH_MYSELF;
>>>         if (h0 == INVALID_HANDLE_VALUE)       <-- !!!
>>>           h0 = NULL;
>>>       }
>>>
>>> The aforementioned commits changed that so h0 was always NULL, this way
>>> creating myself at an arbitrary address.
>>>
>>> This patch makes sure to set the handle to INVALID_HANDLE_VALUE again
>>> when creating a new process, so init knows that myself has to be created
>>> in the right spot.  While at it, fix a potential uninitialized handle
>>> value in child_info_spawn::handle_spawn.
>>>
>>> Fixes: b5e1003722cb ("Cygwin: processes: use dedicated Cygwin PID rather than Windows PID")
>>> Fixes: 88605243a19b ("Cygwin: fix child getting another pid after spawnve")
>>> Signed-off-by: Corinna Vinschen <[hidden email]>
>>> ---
>>>    winsup/cygwin/dcrt0.cc | 2 +-
>>>    winsup/cygwin/pinfo.cc | 3 +--
>>>    2 files changed, 2 insertions(+), 3 deletions(-)
>>> [...]
>>
>> I'll be glad to take a close look at this as you asked.  But I'm not familiar
>> with this part of the code, so it will take me a little time.
>>
>> Ken
>
> Thanks!  I accidentally pushed the patch a few minutes ago when I
> was actually just planning to push the ndbm.h patch.  Anyway, I
> took the opportunity to create new snapshots with all patches from
> yesterday and today, so the getpgrp problems in GDB 8.1.1 and 8.2.1
> should both be fixed there as well.
>
> I'd still be glad if the two of you could check if my patch makes
> sense as is.

It looks fine to me, though I can't claim to have grasped all its implications.
In any case, I've installed it and have done a few things that often catch bugs
(e.g., building emacs and running its test suite), and there are no problems so far.

My next step will be to install the experimental pipe code that I posted in
https://cygwin.com/ml/cygwin-patches/2019-q2/msg00144.html to see if that shakes
anything loose.

Ken
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] Cygwin: Fix the address of myself

Corinna Vinschen-2
On Jul 25 19:59, Ken Brown wrote:

> On 7/25/2019 9:37 AM, Corinna Vinschen wrote:
> > On Jul 24 19:11, Ken Brown wrote:
> >> On 7/24/2019 12:54 PM, Corinna Vinschen wrote:
> >>> From: Corinna Vinschen <[hidden email]>
> >>>
> >>> v2: rephrase commit message
> >>>
> >>> Introducing an independent Cygwin PID introduced a regression:
> >>> [...]
> >>> This patch makes sure to set the handle to INVALID_HANDLE_VALUE again
> >>> when creating a new process, so init knows that myself has to be created
> >>> in the right spot.  While at it, fix a potential uninitialized handle
> >>> value in child_info_spawn::handle_spawn.
> >>>
> >>> Fixes: b5e1003722cb ("Cygwin: processes: use dedicated Cygwin PID rather than Windows PID")
> >>> Fixes: 88605243a19b ("Cygwin: fix child getting another pid after spawnve")
> >>> Signed-off-by: Corinna Vinschen <[hidden email]>
> >>> [...]
> >>
> >> I'll be glad to take a close look at this as you asked.  But I'm not familiar
> >> with this part of the code, so it will take me a little time.
> >>
> >> Ken
> >
> > Thanks!  I accidentally pushed the patch a few minutes ago when I
> > was actually just planning to push the ndbm.h patch.  Anyway, I
> > took the opportunity to create new snapshots with all patches from
> > yesterday and today, so the getpgrp problems in GDB 8.1.1 and 8.2.1
> > should both be fixed there as well.
> >
> > I'd still be glad if the two of you could check if my patch makes
> > sense as is.
>
> It looks fine to me, though I can't claim to have grasped all its implications.
> In any case, I've installed it and have done a few things that often catch bugs
> (e.g., building emacs and running its test suite), and there are no problems so far.
>
> My next step will be to install the experimental pipe code that I posted in
> https://cygwin.com/ml/cygwin-patches/2019-q2/msg00144.html to see if that shakes
> anything loose.
>
> Ken
Great, thank you!


Corinna

--
Corinna Vinschen
Cygwin Maintainer

signature.asc (849 bytes) Download Attachment