public inbox for cygwin-developers@cygwin.com
 help / color / mirror / Atom feed
From: Corinna Vinschen <corinna-cygwin@cygwin.com>
To: cygwin-developers@cygwin.com
Subject: Re: malloc crash
Date: Tue, 26 Oct 2021 18:49:33 +0200	[thread overview]
Message-ID: <YXgxnRzcx9u58yHS@calimero.vinschen.de> (raw)
In-Reply-To: <66ba46c1-afcd-5f31-58ef-06f96209127e@cornell.edu>

On Oct 26 12:36, Ken Brown wrote:
> On 10/26/2021 12:03 PM, Corinna Vinschen wrote:
> > On Oct 26 10:32, Ken Brown wrote:
> > > It might take a while to get this right, and the bug has existed ever since
> > > I overhauled the fifo code.  So I don't think you have to hold up releasing
> > > 3.3.0 while I work on this.
> > 
> > Try the below patch instead, per Takashi's testing and subsequent discussion.
> > [...]
> I agree with Takashi that this fixes the problem.  Thanks to both of you!
> This saved me a lot of testing and the risk of introducing new bugs.

Below's another incarnation of this patch.  I wasn't quite happy with
the first one.  Statically initialized SRWLOCKs seem to be the natural
way to solve the problem...

Ken, Takashi, can you please test it both, just to get as much testing
as possible?


Thanks,
Corinna

From 44a79a6eca3d322020dda0919023d78dda129d4d Mon Sep 17 00:00:00 2001
From: Corinna Vinschen <corinna@vinschen.de>
Date: Tue, 26 Oct 2021 17:53:08 +0200
Subject: [PATCH] Cygwin: convert malloc lock to SRWLOCK

Per https://cygwin.com/pipermail/cygwin-developers/2021-October/012429.html,
we may encounter a crash when starting multiple threads during process
startup (here: fhandler_fifo::fixup_after_{fork,exec}) which in turn
allocate memory via malloc.

The problem is concurrent usage of malloc before the malloc muto has
been initialized.

To fix this issue, convert the muto to a SRWLOCK and make sure it is
statically initalized.  Thus, malloc can be called as early as necessary
and malloc_init is only required to check for user space provided malloc.

Note that this requires to implement a __malloc_trylock macro to be
called from fork.

Signed-off-by: Corinna Vinschen <corinna@vinschen.de>
---
 winsup/cygwin/cygmalloc.h       | 8 +++++---
 winsup/cygwin/dcrt0.cc          | 1 +
 winsup/cygwin/fork.cc           | 2 +-
 winsup/cygwin/heap.cc           | 1 -
 winsup/cygwin/heap.h            | 1 -
 winsup/cygwin/malloc_wrapper.cc | 4 +---
 winsup/cygwin/release/3.3.0     | 4 ++++
 7 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/winsup/cygwin/cygmalloc.h b/winsup/cygwin/cygmalloc.h
index 84bad824c5ae..36f56294fb35 100644
--- a/winsup/cygwin/cygmalloc.h
+++ b/winsup/cygwin/cygmalloc.h
@@ -36,9 +36,11 @@ void *mmap64 (void *, size_t, int, int, int, off_t);
 
 #elif defined (__INSIDE_CYGWIN__)
 
-# define __malloc_lock() mallock.acquire ()
-# define __malloc_unlock() mallock.release ()
-extern muto mallock;
+# define __malloc_lock() AcquireSRWLockExclusive (&mallock)
+# define __malloc_trylock() TryAcquireSRWLockExclusive (&mallock)
+# define __malloc_unlock() ReleaseSRWLockExclusive (&mallock)
+extern SRWLOCK NO_COPY mallock;
+void malloc_init ();
 
 #endif
 
diff --git a/winsup/cygwin/dcrt0.cc b/winsup/cygwin/dcrt0.cc
index 6f4723bb059d..f3d09c1695f8 100644
--- a/winsup/cygwin/dcrt0.cc
+++ b/winsup/cygwin/dcrt0.cc
@@ -29,6 +29,7 @@ details. */
 #include "shared_info.h"
 #include "cygwin_version.h"
 #include "dll_init.h"
+#include "cygmalloc.h"
 #include "heap.h"
 #include "tls_pbuf.h"
 #include "exception.h"
diff --git a/winsup/cygwin/fork.cc b/winsup/cygwin/fork.cc
index 719217856ae3..912f1727441e 100644
--- a/winsup/cygwin/fork.cc
+++ b/winsup/cygwin/fork.cc
@@ -296,7 +296,7 @@ frok::parent (volatile char * volatile stack_here)
   si.lpReserved2 = (LPBYTE) &ch;
   si.cbReserved2 = sizeof (ch);
 
-  bool locked = __malloc_lock ();
+  bool locked = __malloc_trylock ();
 
   /* Remove impersonation */
   cygheap->user.deimpersonate ();
diff --git a/winsup/cygwin/heap.cc b/winsup/cygwin/heap.cc
index b839c8cd48ee..f27f81bc4b59 100644
--- a/winsup/cygwin/heap.cc
+++ b/winsup/cygwin/heap.cc
@@ -230,7 +230,6 @@ user_heap_info::init ()
   debug_printf ("heap base %p, heap top %p, heap size %ly (%lu)",
 		base, top, chunk, chunk);
   page_const--;
-  // malloc_init ();
 }
 
 #define pround(n) (((size_t)(n) + page_const) & ~page_const)
diff --git a/winsup/cygwin/heap.h b/winsup/cygwin/heap.h
index 565758e4872c..25200f286f8a 100644
--- a/winsup/cygwin/heap.h
+++ b/winsup/cygwin/heap.h
@@ -10,7 +10,6 @@ details. */
 
 /* Heap management. */
 void heap_init ();
-void malloc_init ();
 
 #define inheap(s) \
   (cygheap->user_heap.ptr && s \
diff --git a/winsup/cygwin/malloc_wrapper.cc b/winsup/cygwin/malloc_wrapper.cc
index 3b245800abec..2842e5398501 100644
--- a/winsup/cygwin/malloc_wrapper.cc
+++ b/winsup/cygwin/malloc_wrapper.cc
@@ -269,13 +269,11 @@ strdup (const char *s)
    newlib will call __malloc_lock and __malloc_unlock at appropriate
    times.  */
 
-muto NO_COPY mallock;
+SRWLOCK NO_COPY mallock = SRWLOCK_INIT;
 
 void
 malloc_init ()
 {
-  mallock.init ("mallock");
-
   /* Check if malloc is provided by application. If so, redirect all
      calls to malloc/free/realloc to application provided. This may
      happen if some other dll calls cygwin's malloc, but main code provides
diff --git a/winsup/cygwin/release/3.3.0 b/winsup/cygwin/release/3.3.0
index 895c273974cd..1595b17539ce 100644
--- a/winsup/cygwin/release/3.3.0
+++ b/winsup/cygwin/release/3.3.0
@@ -78,3 +78,7 @@ Bug Fixes
 - Fix access violation that can sometimes occur when copy/pasting between
   32-bit and 64-bit Cygwin environments.  Align clipboard descriptor layouts.
   Addresses: https://cygwin.com/pipermail/cygwin-patches/2021q4/011517.html
+
+- Fix a synchronization issue when running multiple threads from DLL
+  initialization which in turn call malloc.
+  Addresses: https://cygwin.com/pipermail/cygwin/2021-October/249635.html
-- 
2.31.1


  reply	other threads:[~2021-10-26 16:49 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-24 21:46 Ken Brown
2021-10-25  8:56 ` Takashi Yano
2021-10-25 13:37   ` Ken Brown
2021-10-25  8:59 ` Corinna Vinschen
2021-10-25 12:35   ` Ken Brown
2021-10-25 15:39     ` Corinna Vinschen
2021-10-25 21:29       ` Mark Geisert
2021-10-25 22:02         ` Ken Brown
2021-10-25 23:36           ` Mark Geisert
2021-10-26  0:18             ` Takashi Yano
2021-10-26  0:54               ` Mark Geisert
2021-10-26  8:30                 ` Mark Geisert
2021-10-26  8:52                   ` Takashi Yano
2021-10-26  8:59                     ` Mark Geisert
2021-10-26  9:26                       ` Takashi Yano
2021-10-26  9:31                         ` Corinna Vinschen
2021-10-26  9:28                       ` Corinna Vinschen
2021-10-26  9:27                 ` Corinna Vinschen
2021-10-26  9:24           ` Corinna Vinschen
2021-10-26 14:32             ` Ken Brown
2021-10-26 16:03               ` Corinna Vinschen
2021-10-26 16:36                 ` Ken Brown
2021-10-26 16:49                   ` Corinna Vinschen [this message]
2021-10-26 17:10                     ` Ken Brown
2021-10-27  0:44                     ` Takashi Yano
2021-10-27  9:01                       ` Corinna Vinschen
2021-10-26 16:44                 ` Takashi Yano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YXgxnRzcx9u58yHS@calimero.vinschen.de \
    --to=corinna-cygwin@cygwin.com \
    --cc=cygwin-developers@cygwin.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).