[PATCH 0/1] Don't allow getpgrp() to fail

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

[PATCH 0/1] Don't allow getpgrp() to fail

Ken Brown-6
This patch makes getpgrp() conform to POSIX.  I have some qualms about
it because getpgrp() will now return 0 in cases where it would
previously return -1.  I don't know if this is likely to break any
applications.

The patch fixes the problem reported here:

  https://cygwin.com/ml/cygwin/2019-07/msg00166.html.

See especially the comments in

  https://cygwin.com/ml/cygwin/2019-07/msg00208.html

about bash.  Although my patch does fix the specific problem I
reported about debugging bash, I'm not sure what other side-effects it
might have.

Ken

Ken Brown (1):
  Cygwin: don't allow getpgrp() to fail

 winsup/cygwin/syscalls.cc | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

--
2.21.0

Reply | Threaded
Open this post in threaded view
|

[PATCH 1/1] Cygwin: don't allow getpgrp() to fail

Ken Brown-6
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.
Change getpgrp() so that it doesn't fail even if getpgid() fails.
---
 winsup/cygwin/syscalls.cc | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
index a914ae8a9..3fd62c286 100644
--- a/winsup/cygwin/syscalls.cc
+++ b/winsup/cygwin/syscalls.cc
@@ -3339,7 +3339,19 @@ setpgrp (void)
 extern "C" pid_t
 getpgrp (void)
 {
-  return getpgid (0);
+  pid_t pid = getpgid (0);
+  if (pid != -1)
+    return pid;
+  else
+    {
+      /* According to POSIX, "The getpgrp() function shall always be
+ successful and no return value is reserved to indicate an
+ error."  We return 0 instead of -1 for the sake of
+ applications that mistakenly check whether the return value
+ is -1. */
+      set_errno (0);
+      return 0;
+    }
 }
 
 extern "C" char *
--
2.21.0

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/1] Cygwin: don't allow getpgrp() to fail

Corinna Vinschen-2
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.

> Change getpgrp() so that it doesn't fail even if getpgid() fails.

Instead of calling getpgid(0), we could just as well return
myself->pgid.  This never fails for sure.


Thanks,
Corinna

--
Corinna Vinschen
Cygwin Maintainer

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

Re: [PATCH 1/1] Cygwin: don't allow getpgrp() to fail

Corinna Vinschen-2
On Jul 23 18: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.
>
> > Change getpgrp() so that it doesn't fail even if getpgid() fails.
>
> Instead of calling getpgid(0), we could just as well return
> myself->pgid.  This never fails for sure.
Just a hunch: The only reason for pinfo::init failing in GDB I can think
of is if the call is too early in the lifetime of the process.  Myself
might not be set up yet.


Corinna

--
Corinna Vinschen
Cygwin Maintainer

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

Re: [PATCH 1/1] Cygwin: don't allow getpgrp() to fail

Corinna Vinschen-2
On Jul 23 18:59, Corinna Vinschen wrote:

> On Jul 23 18: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.
> >
> > > Change getpgrp() so that it doesn't fail even if getpgid() fails.
> >
> > Instead of calling getpgid(0), we could just as well return
> > myself->pgid.  This never fails for sure.
>
> Just a hunch: The only reason for pinfo::init failing in GDB I can think
> of is if the call is too early in the lifetime of the process.  Myself
> might not be set up yet.
No, wrong.  myself is set to &myself_initial at process startup so
it's never NULL.  The pid is probably incorrect at this time, as long
as pinfo::thisproc hasn't been called.


Corinna

--
Corinna Vinschen
Cygwin Maintainer

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

Re: [PATCH 1/1] Cygwin: don't allow getpgrp() to fail

Jon TURNEY
In reply to this post by Corinna Vinschen-2
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).
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/1] Cygwin: don't allow getpgrp() to fail

Corinna Vinschen-2
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?


Corinna

--
Corinna Vinschen
Cygwin Maintainer

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

Re: [PATCH 1/1] Cygwin: don't allow getpgrp() to fail

Ken Brown-6
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
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/1] Cygwin: don't allow getpgrp() to fail

Corinna Vinschen-2
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 (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/1] Cygwin: don't allow getpgrp() to fail

Ken Brown-6
In reply to this post by Ken Brown-6
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