Re: [PATCH] Cygwin: console: Fix segfault on shared_console_info access.

[Corinna Vinschen <corinna-cygwin at cygwin dot com>]

On Feb 22 22:35, Takashi Yano wrote:
> On Sat, 22 Feb 2020 17:01:23 +0900
> Takashi Yano wrote:
> > Hi Corinna,
> > 
> > On Fri, 21 Feb 2020 20:43:33 +0100
> > Corinna Vinschen wrote:
> > > On Feb 22 04:10, Takashi Yano wrote:
> > > > - Accessing shared_console_info accidentaly causes segmentation
> > > >   fault when it is a NULL pointer. The cause of the problem reported
> > > >   in https://cygwin.com/ml/cygwin/2020-02/msg00197.html is this NULL
> > > >   pointer access in request_xterm_mode_output(). This patch fixes
> > > >   the issue.
> > > 
> > > When does this occur?  [...]
> > This happens when request_xterm_mode_output() is called from
> > close(). Usually, shared_console_info has been set when it is
> > called from close(), buf this happnes in mintty case. Since I
> > was not sure why shared_console_info is NULL in mintty case,
> > I have investigated deeper.
> > 
> > And I found the following code causes the same situation.
> > 
> > /* fork-setsid.c: */
> > /* gcc -mwindows fork-setsid.c -o fork-setsid */
> > #include <unistd.h>
> > 
> > int main()
> > {
> >     if (!fork()) setsid();
> >     return 0;
> > }
> > 
> > In this case, close() is called via cygheap->close_ctty() from
> > setsid() even though console is not opened.
> > 
> > Therefore, the following patch also fixes the mintty issue.
> > However, I am not sure this is the right thing.
> > [...]
> This does not work as expected. How about this one?
> 
> diff --git a/winsup/cygwin/dtable.cc b/winsup/cygwin/dtable.cc
> index 4652de929..138b7a1eb 100644
> --- a/winsup/cygwin/dtable.cc
> +++ b/winsup/cygwin/dtable.cc
> @@ -937,6 +937,7 @@ dtable::fixup_after_exec ()
>  void
>  dtable::fixup_after_fork (HANDLE parent)
>  {
> +  bool ctty_opened = false;
>    fhandler_base *fh;
>    for (size_t i = 0; i < size; i++)
>      if ((fh = fds[i]) != NULL)
> @@ -957,7 +958,11 @@ dtable::fixup_after_fork (HANDLE parent)
>  	  SetStdHandle (std_consts[i], fh->get_handle ());
>  	else if (i <= 2)
>  	  SetStdHandle (std_consts[i], fh->get_output_handle ());
> +	if (cygheap->ctty == fh->archetype)
> +	  ctty_opened = true;
>        }
> +  if (!ctty_opened)
> +    cygheap->ctty = NULL;
>  }

I'm not sure this is safe, archetype can be NULL.

I debugged this situation as well and what strikes me as weird is
that in fhandler_console::close, there are two calls to
request_xterm_mode_output(false).  The first one is only called if
shared_console_info is != NULL, the other one is always called if
wincap.has_con_24bit_colors().  This looks a bit fishy.  Not only
the shared_console_info test is missing, but also the con_is_legacy
test.

What about something like this:

diff --git a/winsup/cygwin/fhandler_console.cc b/winsup/cygwin/fhandler_console.cc
index 42040a97162e..edb71fffe48f 100644
--- a/winsup/cygwin/fhandler_console.cc
+++ b/winsup/cygwin/fhandler_console.cc
@@ -1159,18 +1159,17 @@ fhandler_console::close ()
 
   acquire_output_mutex (INFINITE);
 
-  if (shared_console_info && myself->pid == con.owner &&
-      wincap.has_con_24bit_colors () && !con_is_legacy)
-    request_xterm_mode_output (false);
-
-  /* Restore console mode if this is the last closure. */
-  OBJECT_BASIC_INFORMATION obi;
-  NTSTATUS status;
-  status = NtQueryObject (get_handle (), ObjectBasicInformation,
-			  &obi, sizeof obi, NULL);
-  if (NT_SUCCESS (status) && obi.HandleCount == 1)
-    if (wincap.has_con_24bit_colors ())
-      request_xterm_mode_output (false);
+  if (shared_console_info && !con_is_legacy && wincap.has_con_24bit_colors ())
+    {
+      /* Restore console mode if this is the last closure. */
+      OBJECT_BASIC_INFORMATION obi;
+      NTSTATUS status;
+      status = NtQueryObject (get_handle (), ObjectBasicInformation,
+			      &obi, sizeof obi, NULL);
+      if ((NT_SUCCESS (status) && obi.HandleCount == 1)
+	  || myself->pid == con.owner)
+	request_xterm_mode_output (false);
+    }
 
   release_output_mutex ();
 
Btw., are you testing the console with black background?  I'm asking
because I'm using the console with grey background and black characters,
and I'm always seeing artifacts when using vim in xterm mode.

E.g., open vim on the fork-setsid.c source in the console in xterm
mode.  Move the cursor to the beginning of the word `setsid'.  Now
press the three chars

  c h <CR>

this moves the setsid call to the next line.  But it also adds
black background after `setsid();'.  Simiar further actions always
create black background artifacts.

Is there anything we can do against that?


Thanks,
Corinna

-- 
Corinna Vinschen
Cygwin Maintainer

Attachment: signature.asc
Description: PGP signature