Patch to optionally disable overlapped pipes

Patch to optionally disable overlapped pipes

[James Johnston]

[-- Attachment #1: Type: text/plain, Size: 2297 bytes --]

Hi,

As I have recently mentioned on the main Cygwin mailing list, Cygwin by
default creates FILE_FLAG_OVERLAPPED named pipes for the standard file
handles (stdin/stdout/stderr).  These overlapped pipes require all programs
using ReadFile/WriteFile to use overlapped I/O when using the pipes.  Since
standard runtimes in Win32 programs don't normally use overlapped I/O on the
standard file handles, most Win32 programs will exhibit undefined behavior
when called by Cygwin.  In my case, it has resulted in a problem with
calling .NET Framework 4.0 programs from Cygwin (which, coincidentally,
NETFX 4.0 also probably has a bug resulting in undefined behavior that
definitely clashes with overlapped pipes).

The attached patch creates a new "pipe_nooverlap" flag in the CYGWIN
environment variable (similar to the existing pipe_byte flag).  By default,
the flag would not be set and Cygwin will continue to make overlapped pipes
by default, because I did not know if this will result in any breakages
elsewhere in some Cygwin packages.

If the new "pipe_nooverlap" flag is set, then Cygwin won't make overlapped
pipes by default (i.e. pipes made by the pipe or pipe2 functions).  For me,
this got NETFX 4.0 programs working again.  I've been using it all day today
with no ill effects noted.  It seems safe to use.  But I made it a flag
because I am not 100% certain that some package won't break, and it isn't
needed if you are only running Cygwin programs (which presumably use
Cygwin1.dll which presumably is using overlapped I/O everywhere).

If the maintainers feel that a CYGWIN flag isn't necessary and it is safe to
always remove FILE_FLAG_OVERLAPPED, then I can submit a patch that doesn't
have the "pipe_nooverlap" flag - i.e. just assumes the flag is always set.

Thank you for the consideration of this patch.  Here are the change log
entries:

2013-12-24  James Johnston  <JamesJ@motionview3d.com>

	* environ.cc (struct parse_thing): Add "pipe_nooverlap" option.
	* globals.cc (pipe_nooverlap): Declare.
	* pipe.cc (fhandler_pipe::create): Honor pipe_nooverlap to create
non-
	overlapped pipes if set.


==== Documentation changelog entry ====

2013-12-24  James Johnston  <JamesJ@motionview3d.com>

	* cygwinenv.xml: Add pipe_nooverlap description.

Best regards,

James Johnston


[-- Attachment #2: pipepatch --]
[-- Type: application/octet-stream, Size: 2959 bytes --]

Index: cygwin/environ.cc
===================================================================
RCS file: /cvs/src/src/winsup/cygwin/environ.cc,v
retrieving revision 1.209
diff -u -p -r1.209 environ.cc
--- cygwin/environ.cc	24 Nov 2013 12:13:33 -0000	1.209
+++ cygwin/environ.cc	24 Dec 2013 15:54:09 -0000
@@ -130,6 +130,7 @@ static struct parse_thing
   {"export", {&export_settings}, setbool, NULL, {{false}, {true}}},
   {"glob", {func: glob_init}, isfunc, NULL, {{0}, {s: "normal"}}},
   {"pipe_byte", {&pipe_byte}, setbool, NULL, {{false}, {true}}},
+  {"pipe_nooverlap", {&pipe_nooverlap}, setbool, NULL, {{false}, {true}}},
   {"proc_retry", {func: set_proc_retry}, isfunc, NULL, {{0}, {5}}},
   {"reset_com", {&reset_com}, setbool, NULL, {{false}, {true}}},
   {"wincmdln", {&wincmdln}, setbool, NULL, {{false}, {true}}},
Index: cygwin/globals.cc
===================================================================
RCS file: /cvs/src/src/winsup/cygwin/globals.cc,v
retrieving revision 1.55
diff -u -p -r1.55 globals.cc
--- cygwin/globals.cc	9 Dec 2013 20:32:24 -0000	1.55
+++ cygwin/globals.cc	24 Dec 2013 15:54:09 -0000
@@ -72,6 +72,7 @@ bool detect_bloda;
 bool dos_file_warning = true;
 bool ignore_case_with_glob;
 bool pipe_byte;
+bool pipe_nooverlap;
 bool reset_com;
 bool wincmdln;
 winsym_t allow_winsymlinks = WSYM_sysfile;
Index: cygwin/pipe.cc
===================================================================
RCS file: /cvs/src/src/winsup/cygwin/pipe.cc,v
retrieving revision 1.148
diff -u -p -r1.148 pipe.cc
--- cygwin/pipe.cc	1 May 2013 01:20:37 -0000	1.148
+++ cygwin/pipe.cc	24 Dec 2013 15:54:09 -0000
@@ -342,7 +342,8 @@ fhandler_pipe::create (fhandler_pipe *fh
   SECURITY_ATTRIBUTES *sa = sec_none_cloexec (mode);
   int res = -1;
 
-  int ret = create (sa, &r, &w, psize, NULL, FILE_FLAG_OVERLAPPED);
+  int ret = create (sa, &r, &w, psize, NULL,
+		   pipe_nooverlap ? 0 : FILE_FLAG_OVERLAPPED);
   if (ret)
     __seterrno_from_win_error (ret);
   else if ((fhs[0] = (fhandler_pipe *) build_fh_dev (*piper_dev)) == NULL)
Index: doc/cygwinenv.xml
===================================================================
RCS file: /cvs/src/src/winsup/doc/cygwinenv.xml,v
retrieving revision 1.4
diff -u -p -r1.4 cygwinenv.xml
--- doc/cygwinenv.xml	26 Jul 2013 17:27:59 -0000	1.4
+++ doc/cygwinenv.xml	24 Dec 2013 15:54:09 -0000
@@ -68,6 +68,11 @@ message mode.</para>
 </listitem>
 
 <listitem>
+<para><envar>(no)pipe_nooverlap</envar> - causes Cygwin to open pipes in non-overlapped mode by default, rather
+than overlapped mode.  Useful for when running a Win32 program that doesn't expect an overlapped pipe.</para>
+</listitem>
+
+<listitem>
 <para><envar>proc_retry:n</envar> - causes <function>fork()</function> and
 <function>exec*()</function> to retry n times when a child process fails
 due to certain windows-specific errors.  These errors usually occur when

Re: Patch to optionally disable overlapped pipes

[Christopher Faylor]

On Tue, Dec 24, 2013 at 11:01:21PM -0000, James Johnston wrote:
>Hi,
>
>As I have recently mentioned on the main Cygwin mailing list, Cygwin by
>default creates FILE_FLAG_OVERLAPPED named pipes for the standard file
>handles (stdin/stdout/stderr).  These overlapped pipes require all programs
>using ReadFile/WriteFile to use overlapped I/O when using the pipes.

Thanks for the patch but Cygwin has been using overlapped I/O with pipes
for many years.  They are a requirement for proper operation with
signals.  We try to be very sparing when adding new options and we're
not going to add an option to make things work less reliably.

cgf

RE: Patch to optionally disable overlapped pipes

[James Johnston]

> -----Original Message-----
> From: Christopher Faylor
> Sent: Wednesday, December 25, 2013 04:13
>
> On Tue, Dec 24, 2013 at 11:01:21PM -0000, James Johnston wrote:
> >Hi,
> >
> >As I have recently mentioned on the main Cygwin mailing list, Cygwin by
> >default creates FILE_FLAG_OVERLAPPED named pipes for the standard file
> >handles (stdin/stdout/stderr).  These overlapped pipes require all
> >programs using ReadFile/WriteFile to use overlapped I/O when using the
> pipes.
> 
> Thanks for the patch but Cygwin has been using overlapped I/O with pipes
> for many years.  They are a requirement for proper operation with signals.
> We try to be very sparing when adding new options and we're not going to
> add an option to make things work less reliably.

Alright, so what is the solution to my problems (outlined on the main cygwin
list) if not this patch?  I have been using it for a few weeks with no ill
effects.  It fixed all the problems I was having.  Signals work fine, as far
as I can tell.  I think this is going to be something that will benefit
other users.  Can I modify the patch to make it acceptable?  An analysis of
why I think the patch is harmless is below...

The function I modified is fhandler_pipe::create(fhandler_pipe**, unsigned,
int).  This function is a thin wrapper around a more specific
fhandler_pipe::create(LPSECURITY_ATTRIBUTES, PHANDLE, PHANDLE, DWORD, const
char*, DWORD open_mode) with default values for some of the parameters for
that more specific function, and it passes FILE_FLAG_OVERLAPPED by default.
My change involved optionally removing FILE_FLAG_OVERLAPPED from the
default.

Critically, my change does NOT affect any code that uses the
fhandler_pipe::create overload that takes 6 parameters.  If anyone still
wants an overlapped pipe, they can still pass FILE_FLAG_OVERLAPPED to the
create function that takes 6 parameters.  And as it turns out, Cygwin
already does this.  I searched, and checked each place fhandler_pipe::create
is called:

JamesJ at JamesJ-PC /d/AppData/Local/Temp/cygwin-snapshot-20131219-1
$ grep -r "fhandler_pipe::create" .
<snip changelogs and comments>

./winsup/cygwin/fhandler_fifo.cc:  fhandler_pipe::create (sa_buf, (r), (w),
0, fnpipe (), open_mode)
./winsup/cygwin/fhandler_tty.cc:  res = fhandler_pipe::create (&sec_none,
&get_io_handle (), &to_master,
./winsup/cygwin/miscfuncs.cc:  int ret = fhandler_pipe::create (sa, hr, hw,
0, NULL,
./winsup/cygwin/sigproc.cc:  DWORD err = fhandler_pipe::create (sa,
&my_readsig, &my_sendsig,
./winsup/cygwin/tty.cc:  return fhandler_pipe::create (&sec_none, &r, &w,
^--- all the above use 6 parameter overload: they may or may not specify
overlapped pipe today, and my change does NOT affect this code in ANY way.

./winsup/cygwin/pipe.cc:  int res = fhandler_pipe::create (fhs, psize,
mode);
^--- the pipe_worker function uses 3 parameter overload, so it will be
affected by the new pipe_nooverlap flag and may not get an overlapped pipe.
pipe_worker is a static function and is used by the POSIX _pipe, pipe, and
pipe2 functions.

From the above, we can see that the ONLY code that is affected by my change
is the pipe_worker function, which is used by the POSIX functions for
creating a pipe.  And from further grepping, I couldn't find any places
where Cygwin was using the POSIX pipe functions for purposes other than
implementing other POSIX functions and in libc.

Some counterpoints:

 * From the above search, signal handling code in places like sigproc.cc and
miscfuncs.cc are clearly unaffected, because they use the 6 parameter
overload, which I didn't modify.  If you pass FILE_FLAG_OVERLAPPED to that
function, it will still work and give you an overlapped pipe.

 * Just to test your hypothesis that I broke signals, I copied a few sample
programs off the Internet that use simple signal handling (e.g. custom
Ctrl+C handler, handler for user signal sent from kill).  Everything worked
as expected, nothing looks broken.  If signal handling is broken, I have yet
to find out how!

 * From a practical standpoint, I've been using this for a few weeks now
since I sent the original message.  I haven't had any problems.  And the
.NET programs I was calling work fine now, whereas they did not previously.
It has only benefitted me, with no problems.  I suspect it would benefit
other users as well.

 * Suppose someone does create a non-overlapped pipe, and then tries to do
overlapped I/O on it.  The Win32 API documentation says this should work,
just that the operation will complete synchronously instead of
asynchronously.  Note that this is a situation that has to be considered
anyway when doing asynchronous I/O in Win32: ReadFile/WriteFile says that
the operation may or may not complete synchronously on an asynchronous file
handle.  It is *not* undefined behavior to do overlapped I/O on a
non-overlapped file handle - it is 100% supported - it will just be
synchronous.  In fact, there is an MSKB article about it:
http://support.microsoft.com/kb/156932/en-us

 * I am not a POSIX expert so I don't know every every single API call that
might possibly want to use asynchronous overlapped I/O on a Win32 file
handle, such as the ones returned by pipe/pipe2.  But the obvious would be
POSIX asynchronous I/O.  But Cygwin doesn't even implement these API calls;
all the aio functions in aio.c just return an error code.  Obviously, they
are not widely used in the wild.  If Cygwin did implement POSIX asynchronous
I/O in the future, it would seem that overlapped pipes would then be
required, and the pipe_nooverlap option might be useful for these rare
situations.

 * Why allow the pipe_byte option but not the pipe_nooverlap option?  Both
are required to properly support calling non-Cygwin programs.  If it is
really an issue to add a new CYGWIN flag, I suggest one of the following:
        * Combine the two flags into one, to be used by users who invoke
non-Cygwin programs.
        * Always assume pipe_nooverlap is set, and remove the flag - since
it appears that there won't be any breaking changes anyway.  It can be added
later if POSIX async I/O is implemented.

Best regards,

James Johnston

Re: Patch to optionally disable overlapped pipes

[Christopher Faylor]

On Wed, Jan 08, 2014 at 06:00:54PM -0000, James Johnston wrote:
>The function I modified is fhandler_pipe::create(fhandler_pipe**, unsigned,
>int).  This function is a thin wrapper around a more specific
>fhandler_pipe::create(LPSECURITY_ATTRIBUTES, PHANDLE, PHANDLE, DWORD, const
>char*, DWORD open_mode) with default values for some of the parameters for
>that more specific function, and it passes FILE_FLAG_OVERLAPPED by default.
>My change involved optionally removing FILE_FLAG_OVERLAPPED from the
>default.
>
>Critically, my change does NOT affect any code that uses the
>fhandler_pipe::create overload that takes 6 parameters.

I'm the author of the code and I'm familiar with the implications of what
you did.  You modified the way pipes are commonly created.  I'm not
comfortable supporting code which has that option.

cgf