From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mark H Weaver Subject: bug#28211: Grafting code triggers GC/thread-safety issue on Guile 2.2.2 Date: Tue, 08 May 2018 20:32:26 -0400 Message-ID: <87d0y5k6sl.fsf@netris.org> References: <877exuj58y.fsf@gnu.org> <87d0yo1tie.fsf@gnu.org> <87fu3124nt.fsf@gnu.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:34433) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fGD3V-0000z0-WC for bug-guix@gnu.org; Tue, 08 May 2018 20:34:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fGD3S-0003ae-Pw for bug-guix@gnu.org; Tue, 08 May 2018 20:34:05 -0400 Received: from debbugs.gnu.org ([208.118.235.43]:46739) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1fGD3S-0003aS-M8 for bug-guix@gnu.org; Tue, 08 May 2018 20:34:02 -0400 Sender: "Debbugs-submit" Resent-Message-ID: In-Reply-To: <87fu3124nt.fsf@gnu.org> ("Ludovic \=\?utf-8\?Q\?Court\=C3\=A8s\=22'\?\= \=\?utf-8\?Q\?s\?\= message of "Tue, 08 May 2018 23:55:50 +0200") List-Id: Bug reports for GNU Guix List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-guix-bounces+gcggb-bug-guix=m.gmane.org@gnu.org Sender: "bug-Guix" To: Ludovic =?UTF-8?Q?Court=C3=A8s?= Cc: Andy Wingo , 28211@debbugs.gnu.org Hi Ludovic, ludo@gnu.org (Ludovic Court=C3=A8s) writes: [...] > Thread 1 (Thread 0x7f6fe6f5d700 (LWP 2856)): > #0 0x00007f7019db0d79 in scm_is_pair (x=3D0x0) at ../libguile/pairs.h:159 > #1 scm_ilength (sx=3D) at list.c:190 [...] > What this means is that Thread 1 gets NULL instead of a list as its > on-stack argument (vm-engine.c:909 is =E2=80=98tail-apply=E2=80=99). > > How can arguments on the VM stack be zeroed? I doubt that's what happened, because I expect that each VM stack is dedicated to a single hardware thread. In theory, if a single VM stack is used by one thread, and then later used by another thread, thread-safety issues on the VM stack could arise in the absense of proper thread synchronization. However, I think it's _far_ more likely that the NULL argument on the stack was copied from memory shared by multiple threads without proper thread synchronization. On modern weakly-ordered memory architectures, writes by one thread may be seen in a different order by another thread. For example, if one thread allocates a pair, initializes its CAR and CDR to non-NULL values, and then writes the address of the pair somewhere, another thread could first read the address of the pair, and then read NULL from its CAR or CDR, unless proper thread synchronization is used. At best, this requires memory barriers in both the reader and writer which are typically quite expensive. On x86 processors the read barrier could expand into a no-op in typical cases, but the write barrier cannot be avoided, and must be placed after initializing the structure and before writing its pointer. I think it's most likely that something like this is happening, because NULL is not a valid SCM value. The only code that should normally write a NULL to an SCM slot is Boehm GC, which clears memory before handing it to the user. So, if you read a NULL from a memory location that's meant to hold an SCM, then it's most likely because the reading thread does not yet see the writes that initialized it, because of the weakly-ordered memory architecture. > I commented out the MADV_DONTNEED call to be sure, but I can still > reproduce the bug. I strongly doubt that the MADV_DONTNEED is relevant to this issue. > Then I thought vp->sp might be out-of-sync compared to the local > variable =E2=80=98sp=E2=80=99, which in turn could cause =E2=80=98scm_i_v= m_mark_stack=E2=80=99 to not > mark a few items on the tip of the stack. So I did this: > > diff --git a/libguile/vm-engine.c b/libguile/vm-engine.c > index 9509cd643..1136b2271 100644 > --- a/libguile/vm-engine.c > +++ b/libguile/vm-engine.c > @@ -151,7 +151,8 @@ > code, or otherwise push anything on the stack, you will need to > CACHE_SP afterwards to restore the possibly-changed stack pointer. */ >=20=20 > -#define SYNC_IP() vp->ip =3D (ip) > +#define SYNC_IP() \ > + do { vp->ip =3D (ip); vp->sp =3D (sp); } while (0) I don't see how a change like this could be useful for any thread safety problem. Since stores by one thread may be seen in a different order by other threads, the memory write corresponding to "vp->sp =3D (sp)" might be delayed for some arbitrarily long period of time after the writes that follow it, up until the next appropriate memory barrier. For now, I would suggest avoiding multi-threaded code in Guix, or at least to avoid loading any Scheme code from multiple threads. How about using multiple processes instead? Mark