Re: [PATCH 1/1] Cygwin: don't allow getpgrp() to fail
On 7/23/2019 3:16 PM, Corinna Vinschen wrote:
> On Jul 23 19:07, Jon Turney wrote:
>> On 23/07/2019 17:54, Corinna Vinschen wrote:
>>> Hi Ken,
>>>
>>> On Jul 23 16:12, Ken Brown wrote:
According to POSIX, "The getpgrp() function shall always be successful
and no return value is reserved to indicate an error." Cygwin's
getpgrp() is defined in terms of getpgid(), which is allowed to fail.
>>>
>>> But it shouldn't fail for the current process. Why should pinfo::init
>>> fail for myself if it begins like this?
>>>
>>> if (myself && n == myself->pid)
>>> {
>>> procinfo = myself;
>>> destroy = 0;
>>> return;
>>> }
>>>
>>> I fear this patch would only cover up the problem still persisting
>>> under the hood.
>>
>> I agree.
>>
>> There is presumably a class of programs which require getpgrp() to return
>> the correct value for correct operation, which cannot be 0 (since that
>> cannot be a pid).
>
> However, did we ever see this problem outside of GDB?
I think I've found the problem, as I just reported on the main cygwin list.
And
I agree that my patch was misguided.
But I still think getpgrp() should be changed, perhaps by having it just return
myself->pgid as you suggested earlier. There's no point in having getpgrp()
call getpgid(), which does error checking, when POSIX specifically says "no
return value [of getpgrp()] is reserved to indicate an error".
POSIX-compatible
applications should call getpgid(0) instead of getpgrp() if they want to do
error checking.
I'll send a couple of patches, one for this issue and one for the tcsetpgrp()
problem, so that we can discuss it further.
Ken
Re: [PATCH 1/1] Cygwin: don't allow getpgrp() to fail
On Jul 24 15:04, Ken Brown wrote:
> On 7/23/2019 3:16 PM, Corinna Vinschen wrote:
> > On Jul 23 19:07, Jon Turney wrote:
> >> On 23/07/2019 17:54, Corinna Vinschen wrote:
> >>> Hi Ken,
> >>>
> >>> On Jul 23 16:12, Ken Brown wrote:
> According to POSIX, "The getpgrp() function shall always be successful
> and no return value is reserved to indicate an error." Cygwin's
> getpgrp() is defined in terms of getpgid(), which is allowed to fail.
> >>>
> >>> But it shouldn't fail for the current process. Why should pinfo::init
> >>> fail for myself if it begins like this?
> >>>
> >>> if (myself && n == myself->pid)
> >>> {
> >>> procinfo = myself;
> >>> destroy = 0;
> >>> return;
> >>> }
> >>>
> >>> I fear this patch would only cover up the problem still persisting
> >>> under the hood.
> >>
> >> I agree.
> >>
> >> There is presumably a class of programs which require getpgrp() to return
> >> the correct value for correct operation, which cannot be 0 (since that
> >> cannot be a pid).
> >
> > However, did we ever see this problem outside of GDB?
>
> I think I've found the problem, as I just reported on the main cygwin list.
> And
> I agree that my patch was misguided.
>
> But I still think getpgrp() should be changed, perhaps by having it just
> return
> myself->pgid as you suggested earlier. There's no point in having getpgrp()
> call getpgid(), which does error checking, when POSIX specifically says "no
> return value [of getpgrp()] is reserved to indicate an error".
> POSIX-compatible
> applications should call getpgid(0) instead of getpgrp() if they want to do
> error checking.
>
> I'll send a couple of patches, one for this issue and one for the tcsetpgrp()
> problem, so that we can discuss it further.
>
> Ken
I have a very puzzeling result debugging this. I just outlined this to
Jon on the #cygwin-developers Freenode IRC channel. Hopefully your
patch to tcsetpgrp clears this up.
I also found a problem in pinfo::this_proc / pinfo_init while debugging
the above. I'll post the patch to this list since I would very much
like that you and/or Jon take a close look.
Thanks,
Corinna
--
Corinna Vinschen
Cygwin Maintainer
signature.asc
Description: PGP signature
[PATCH] Cygwin: fhandler_termios::tcsetpgrp: check that argument is non-negative
Return -1 with EINVAL if pgid < 0. This fixes the gdb problem
reported here:
https://cygwin.com/ml/cygwin/2019-07/msg00166.html
---
winsup/cygwin/fhandler_termios.cc | 5 +
1 file changed, 5 insertions(+)
diff --git a/winsup/cygwin/fhandler_termios.cc
b/winsup/cygwin/fhandler_termios.cc
index 4ce53433a..5b0ba5603 100644
--- a/winsup/cygwin/fhandler_termios.cc
+++ b/winsup/cygwin/fhandler_termios.cc
@@ -69,6 +69,11 @@ fhandler_termios::tcsetpgrp (const pid_t pgid)
set_errno (EPERM);
return -1;
}
+ else if (pgid < 0)
+{
+ set_errno (EINVAL);
+ return -1;
+}
int res;
while (1)
{
--
2.21.0
Re: [PATCH] Cygwin: fhandler_termios::tcsetpgrp: check that argument is non-negative
On Jul 24 15:34, Ken Brown wrote:
> Return -1 with EINVAL if pgid < 0. This fixes the gdb problem
> reported here:
Why does it fix the issue?
> https://cygwin.com/ml/cygwin/2019-07/msg00166.html
> ---
> winsup/cygwin/fhandler_termios.cc | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/winsup/cygwin/fhandler_termios.cc
> b/winsup/cygwin/fhandler_termios.cc
> index 4ce53433a..5b0ba5603 100644
> --- a/winsup/cygwin/fhandler_termios.cc
> +++ b/winsup/cygwin/fhandler_termios.cc
> @@ -69,6 +69,11 @@ fhandler_termios::tcsetpgrp (const pid_t pgid)
>set_errno (EPERM);
>return -1;
> }
> + else if (pgid < 0)
> +{
> + set_errno (EINVAL);
> + return -1;
> +}
>int res;
>while (1)
> {
> --
> 2.21.0
Looks good with GDB 8.2.1. A bit of description why this fixes the
problem and it's GTG.
Unfortunately it doesn't fix what I'm seeing under GDB 8.1.1, but I'm
more and more convinced this is GDB's fault.
Thanks,
Corinna
--
Corinna Vinschen
Cygwin Maintainer
signature.asc
Description: PGP signature
[PATCH] Cygwin: Fix the address of myself
From: Corinna Vinschen
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
---
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
[PATCH v2] Cygwin: Fix the address of myself
From: Corinna Vinschen
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
---
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
Re: [PATCH 1/1] Cygwin: don't allow getpgrp() to fail
On 7/24/2019 11:04 AM, Ken Brown wrote: > But I still think getpgrp() should be changed I've decided not to pursue this. I don't think it's very important, and I don't know how it might affect applications. Ken
Re: [PATCH v2] Cygwin: Fix the address of myself
On 7/24/2019 12:54 PM, Corinna Vinschen wrote:
> From: Corinna Vinschen
>
> 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
> ---
> 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
