Re: Patch winuser.h in w32api

On Sat, 31 Jan 2004, Nicholas Wourms wrote:

> pechtcha wrote:
>
> > 3) Patches for w32api should be sent to the mingw-patches mailing list
> > (see <http://mingw.org/>).  FYI, they also have their own rules for
> > submitting patches -- see <http://lists.sf.net/lists/listinfo/mingw-patches>
> > for instructions.
>
> Since when?  I've seen patches for w32api posted to and accepted from
> here on numerous occasions in the past.

Hmm, not in the time that I've been monitoring this list.  I've had an
impression (from reading the MinGW site) that mingw-patches is the place
for w32api patches.  I'd be happy to be proven wrong.
	Igor
--

-- 
				http://cs.nyu.edu/~pechtcha/
      |\      _,,,---,,_		pechtcha <at> cs.nyu.edu
ZZZzz /,`.-'`'    -.  ;-;;,_		igor <at> watson.ibm.com
     |,4-  ) )-,_. ,\ (  `'-'		Igor Pechtchanski, Ph.D.
    '---''(_/--'  `-'\_) fL	a.k.a JaguaR-R-R-r-r-r-.-.-.  Meow!

"I have since come to realize that being between your mentor and his route
to the bathroom is a major career booster."  -- Patrick Naughton

Re: [Patch]: ciresrv.parent

On Sat, Jan 31, 2004 at 02:18:48PM -0500, Pierre A. Humblet wrote:
>Fortunately it is never used in the case of spawn: all handles are
>inherited, or the parent does the work (sockets). 

The one placed the handle is actually used is in
fhandler_socket::fixup_after_exec.  I'd like Corinna's confirmation
before this patch is checked in.

cgf

Re: [Patch]: ciresrv.parent

At 01:39 PM 2/1/2004 -0500, Christopher Faylor wrote:
>On Sat, Jan 31, 2004 at 02:18:48PM -0500, Pierre A. Humblet wrote:
>>Fortunately it is never used in the case of spawn: all handles are
>>inherited, or the parent does the work (sockets). 
>
>The one placed the handle is actually used is in
>fhandler_socket::fixup_after_exec.  I'd like Corinna's confirmation
>before this patch is checked in.

Good idea. FWIW, I checked that one carefully. That's why I found
the secret_event bug a while back. I also tested on Win95 with an
old winsock. 
It looks like the handle might be used, but the tests for close
on exec always block the paths where it is actually used. 

Pierre

Re: [Patch]: ciresrv.parent

On Feb  1 16:57, Pierre A. Humblet wrote:
> At 01:39 PM 2/1/2004 -0500, Christopher Faylor wrote:
> >On Sat, Jan 31, 2004 at 02:18:48PM -0500, Pierre A. Humblet wrote:
> >>Fortunately it is never used in the case of spawn: all handles are
> >>inherited, or the parent does the work (sockets). 
> >
> >The one placed the handle is actually used is in
> >fhandler_socket::fixup_after_exec.  I'd like Corinna's confirmation
> >before this patch is checked in.
> 
> Good idea. FWIW, I checked that one carefully. That's why I found
> the secret_event bug a while back. I also tested on Win95 with an
> old winsock. 
> It looks like the handle might be used, but the tests for close
> on exec always block the paths where it is actually used. 

AFAICS, you're right.  fhandler_socket::fixup_after_exec calls
fhandler_socket::fixup_after_fork only if !close_on_exec.
fhandler_socket::fixup_after_fork in turn calls fork_fixup which only
uses the parent handle if close_on_exec.  So the parent handle is never
used in this scenario.  So I think it's ok to drop the parent handle.

As a side note, it took me a while to understand that it's the same
situation for the secret_event handle.  The problem is the name of the
function set_inheritance().  The second parameter is the *negation*
of the inheritance.  IMHO this is rather confusing.  Either we should
rename the function to set_no_inheritance or we should revert the
meaning of the second parameter.

Corinna

Re: [Patch]: ciresrv.parent

Corinna Vinschen wrote:
> 
> On Feb  1 16:57, Pierre A. Humblet wrote:
> > At 01:39 PM 2/1/2004 -0500, Christopher Faylor wrote:
> > >On Sat, Jan 31, 2004 at 02:18:48PM -0500, Pierre A. Humblet wrote:
> > >>Fortunately it is never used in the case of spawn: all handles are
> > >>inherited, or the parent does the work (sockets).
> > >
> > >The one placed the handle is actually used is in
> > >fhandler_socket::fixup_after_exec.  I'd like Corinna's confirmation
> > >before this patch is checked in.
> >
> > Good idea. FWIW, I checked that one carefully. That's why I found
> > the secret_event bug a while back. I also tested on Win95 with an
> > old winsock.
> > It looks like the handle might be used, but the tests for close
> > on exec always block the paths where it is actually used.
> 
> AFAICS, you're right.  fhandler_socket::fixup_after_exec calls
> fhandler_socket::fixup_after_fork only if !close_on_exec.
> fhandler_socket::fixup_after_fork in turn calls fork_fixup which only
> uses the parent handle if close_on_exec.  So the parent handle is never
> used in this scenario.  So I think it's ok to drop the parent handle.

Do you want to apply the patch now, or I do it this evening?

To simplify the life of future maintainers, I was thinking of
redoing fixup_after_exec as follows
(instead of going through the cascade of calls above)

Re: [Patch]: ciresrv.parent

On Mon, Feb 02, 2004 at 09:47:35AM -0500, Pierre A. Humblet wrote:
>Do you want to apply the patch now, or I do it this evening?

I applied it.  I also got rid of all of the now-obsolete
arguments/parameters to fixup_after_exec.

cgf

Re: [Patch]: ciresrv.parent

On Mon, Feb 02, 2004 at 04:04:05PM -0500, Christopher Faylor wrote:
>On Mon, Feb 02, 2004 at 09:47:35AM -0500, Pierre A. Humblet wrote:
>>Do you want to apply the patch now, or I do it this evening?
>
>I applied it.  I also got rid of all of the now-obsolete
>arguments/parameters to fixup_after_exec.

Oops.  PGA.

Thank you for the patch!

cgf

[Patch]: heap_chunk_size

Here is a no brainer patch that eliminates the use of 
"heap_chunk" in the cygwin shared. That removes a source 
of DOS attack and it's another step towards the demise
of the cygwin shared.
Actually deleting the "heap_chunk" member from the structure
will be done shortly.

Pierre

2004-02-02  Pierre Humblet <pierre.humblet <at> ieee.org>

	* shared.cc (shared_info::heap_chunk_size): Delete.
	* heap.cc (heap_chunk_size): Create.
	(heap_init): Call heap_chunk_size instead of
	cygwin_shared->heap_chunk_size.

Index: heap.cc
===================================================================
RCS file: /cvs/src/src/winsup/cygwin/heap.cc,v
retrieving revision 1.39
diff -u -p -r1.39 heap.cc
--- heap.cc	14 Jan 2004 15:45:36 -0000	1.39
+++ heap.cc	3 Feb 2004 01:18:58 -0000
 <at>  <at>  -30,6 +30,38  <at>  <at>  extern "C" size_t getpagesize ();

 #define MINHEAP_SIZE (4 * 1024 * 1024)

+static unsigned
+heap_chunk_size ()

Re: [Patch]: heap_chunk_size

On Mon, Feb 02, 2004 at 08:22:01PM -0500, Pierre A. Humblet wrote:
>Here is a no brainer patch that eliminates the use of "heap_chunk" in
>the cygwin shared.  That removes a source of DOS attack and it's
>another step towards the demise of the cygwin shared.

This isn't a no-brainer.  This value is stored in the shared memory to
avoid the runtime cost of registry lookups by every cygwin program.

cgf

Re: [Patch]: heap_chunk_size

Christopher Faylor wrote:
> 
> On Mon, Feb 02, 2004 at 08:22:01PM -0500, Pierre A. Humblet wrote:
> >Here is a no brainer patch that eliminates the use of "heap_chunk" in
> >the cygwin shared.  That removes a source of DOS attack and it's
> >another step towards the demise of the cygwin shared.
> 
> This isn't a no-brainer.  This value is stored in the shared memory to
> avoid the runtime cost of registry lookups by every cygwin program.

A process only reads the registry when (!cygheap->user_heap.base),
i.e. when it starts from Windows. Previously it read the registry when it
was the first Cygwin process on the machine. There is a penalty, but it's 
small.
The chunk size could also be stored in the user shared (and read if it's
not in the cygheap). Actually that would be an even simpler patch, 
giving the same security. I'll send it tonight.

Pierre