From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Pip Cet Newsgroups: gmane.emacs.devel Subject: Re: MPS: Win64 testers? Date: Fri, 23 Aug 2024 07:59:13 +0000 Message-ID: <8734mv8yb6.fsf@protonmail.com> References: <87bk1k8mzd.fsf@protonmail.com> <861q2gs8xu.fsf@gnu.org> <877cc88j4p.fsf@protonmail.com> <86seuvre6x.fsf@gnu.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="23945"; mail-complaints-to="usenet@ciao.gmane.io" Cc: sebastian@sebasmonia.com, kien.n.quang@gmail.com, emacs-devel@gnu.org, yantar92@posteo.net To: Eli Zaretskii Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Fri Aug 23 14:27:02 2024 Return-path: Envelope-to: ged-emacs-devel@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1shTNd-0005yY-Rj for ged-emacs-devel@m.gmane-mx.org; Fri, 23 Aug 2024 14:27:01 +0200 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1shTN2-0002XY-8j; Fri, 23 Aug 2024 08:26:26 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1shPCh-0002NT-U4 for emacs-devel@gnu.org; Fri, 23 Aug 2024 03:59:27 -0400 Original-Received: from mail-40133.protonmail.ch ([185.70.40.133]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1shPCf-0002i0-DH for emacs-devel@gnu.org; Fri, 23 Aug 2024 03:59:27 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com; s=protonmail3; t=1724399960; x=1724659160; bh=babxw9kP38ymjCWyfrQ+mPyGZNlRrOCekg/4BCHNsEI=; h=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References: Feedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID: Message-ID:BIMI-Selector; b=NzmHsWTqjr/2zx2t3I8MEqJl+HCPg5y19Uq5sXCgbNoZEP9ZI+XrjJj+N+mRT0va9 sxZmX5VpEDQ02RuuQfkx5qctMrVh84Nv3ATA4uWKH7L918y3N3iMzt6uhb7MOLn04x gaORs4T2Vg5RZCtP/sJZJpfiRsBWvsfClubC2ve9jOZopX15n2JP5saHpmHx5tYVD9 lfnZT5RPklAtcmQh523kO0ldevhXZMI06Y9osT2ggz+CwxK/6SDieToup3PtY+KvYU H5s+ZMOPDCLbEWntYH9ZFF/g/m1Yk7WhVgJ9+drGR3wGvA/OPficZu9EL/zDJbSpQy /JRpwZNyynwcg== In-Reply-To: <86seuvre6x.fsf@gnu.org> Feedback-ID: 112775352:user:proton X-Pm-Message-ID: 703fe4efb4127d6c37c425de3a6ebff5c2d16687 Received-SPF: pass client-ip=185.70.40.133; envelope-from=pipcet@protonmail.com; helo=mail-40133.protonmail.ch X-Spam_score_int: -10 X-Spam_score: -1.1 X-Spam_bar: - X-Spam_report: (-1.1 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, FREEMAIL_FROM=0.001, FREEMAIL_REPLY=1, RCVD_IN_MSPIKE_H4=0.001, RCVD_IN_MSPIKE_WL=0.001, RCVD_IN_VALIDITY_CERTIFIED_BLOCKED=0.001, RCVD_IN_VALIDITY_RPBL_BLOCKED=0.001, SPF_HELO_PASS=-0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=no autolearn_force=no X-Spam_action: no action X-Mailman-Approved-At: Fri, 23 Aug 2024 08:26:10 -0400 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.devel:323075 Archived-At: "Eli Zaretskii" writes: >> Date: Thu, 22 Aug 2024 19:14:51 +0000 >> From: Pip Cet >> Cc: sebastian@sebasmonia.com, kien.n.quang@gmail.com, emacs-devel@gnu.or= g, yantar92@posteo.net >> >> "Eli Zaretskii" writes: >> >> I meant this code in gnutls.c: >> >> if (proc->gnutls_initstage < GNUTLS_STAGE_TRANSPORT_POINTERS_SET) >> { >> # ifdef WINDOWSNT >> /* On W32 we cannot transfer socket handles between different runt= ime >> =09 libraries, so we tell GnuTLS to use our special push/pull >> =09 functions. */ >> gnutls_transport_set_ptr2 (state, >> =09=09=09=09 (gnutls_transport_ptr_t) proc, >> =09=09=09=09 (gnutls_transport_ptr_t) proc); >> gnutls_transport_set_push_function (state, &emacs_gnutls_push); >> gnutls_transport_set_pull_function (state, &emacs_gnutls_pull); >> # else >> /* This is how GnuTLS takes sockets: as file descriptors passed >> =09 in. For an Emacs process socket, infd and outfd are the >> =09 same but we use this two-argument version for clarity. */ >> gnutls_transport_set_ptr2 (state, >> =09=09=09=09 (void *) (intptr_t) proc->infd, >> =09=09=09=09 (void *) (intptr_t) proc->outfd); >> if (proc->is_non_blocking_client) >> =09gnutls_transport_set_errno_function (state, >> =09=09=09=09=09 emacs_gnutls_nonblock_errno); >> # endif >> >> proc->gnutls_initstage =3D GNUTLS_STAGE_TRANSPORT_POINTERS_SET; >> } >> >> >> That hands a pointer to 'proc' to gnutls, and if 'proc' subsequently >> moves, emacs_gnutls_push or emacs_gnutls_pull will try to dereference a >> stale pointer. > > But the pointer to 'proc' is passed to GnuTLS for the sole purpose > that GnuTLS passes it back to our callback, emacs_gnutls_pull. GnuTLS > itself does not (and cannot) use that pointer, for the obvious reasons > that it knows nothing about our objects. That is my understanding as well. I'm not sure why you think that's a "but", though. A pointer to memory managed in a copying pool, stored outside of MPS-managed memory, as 'proc' presumably is (in malloc()ed memory, I would guess, or maybe in a static location somewhere), must be pinned somehow, so MPS does not move it during GC. >> > You are talking about this line in emacs_gnutls_pull: >> > >> > emacs_gnutls_transport_set_errno (process->gnutls_state, >> > errno =3D=3D EWOULDBLOCK ? EAGAIN = : errno); >> > >> > which causes us to call GnuTLS thusly: >> > >> > gnutls_transport_set_errno (state, err); >> > >> > is that right? >> >> > Does this mean we will have to audit all our calls to >> > external libraries and make sure we never pass to them any part of any >> > Lisp object? That's a tough requirement; it's very easy to miss some >> > such use, which then triggers segfaults. Am I missing something? >> >> Synchronous calls do not need to be changed, only asynchronous callbacks >> that currently use a pointer to a Lisp object as their cookie. > > But emacs_gnutls_pull is not an asynchronous call, it is called when > Emacs calls GnuTLS to read from the connection. So how this could be > a problem? You're right, it's called synchronously, but uses the callback+cookie convention usually used for asynchronous callbacks. > And what do you mean by "asynchronous calls"? For example, you can > see in w32font.c that we call certain Windows APIs which work by > enumerating objects of some kind and calling our callback for each > such object (the callback that does with each object what it needs to > do). Is this an "asynchronous call"? If not, what is? It doesn't look asynchronous to me, though I'm not sure we're talking about the same code. The LPARAM arguments referring to stack locations are fine, because the stack is known to MPS. >> > Btw, I tried using EWW in my MinGW MPS build, and it didn't crash. Is >> > it possible to have a reproducible recipe for this? >> >> I'll try coming up with one, though that's probably just going to mean >> adding (igc-collect) after the gnutls handshake somewhere in the Lisp >> code. > > Thanks. diff --git a/lisp/net/gnutls.el b/lisp/net/gnutls.el index b5fb4d47d57..f2031143fc9 100644 --- a/lisp/net/gnutls.el +++ b/lisp/net/gnutls.el @@ -208,6 +208,7 @@ open-gnutls-stream :keylist keylist :hostname (puny-encode-domain host)))) :coding (plist-get parameters :coding)))) + (igc-collect) (if nowait process (gnutls-negotiate :process process triggers the bug for me, with this recipe: * emacs -Q * M-x eww RET "https://www.gnu.org" RET > But a clear understanding of what causes these segfaults is also > needed, because Emacs is full of similar callbacks, especially on > Windows, where many APIs we use work with callbacks. MPS needs to know about any pointers to MPS-managed memory. In particular, if the cookie is a pointer to MPS-managed memory, and it's stored in non-MPS-managed memory (such as by an external library), that will cause problems because MPS believes it is free to move the memory the pointer points to, but it can't update the pointer itself, so it will become invalid and dereferencing it will cause segfaults. On the other hand, pointers that are on the stack are fine, since MPS treats those conservatively, never moving the blocks of memory they refer to. The problem here is that 'proc' might move. One solution is to make the cookie a pointer to memory known to MPS, which contains the actual pointer to the Lisp_Process. Note that it is not strictly necessary for the pointer to be allocated with igc_xzalloc_ambig, we could use the _raw_exact variant. diff --git a/src/alloc.c b/src/alloc.c index f41cef9516a..50086f22b24 100644 --- a/src/alloc.c +++ b/src/alloc.c @@ -3569,12 +3569,17 @@ cleanup_vector (struct Lisp_Vector *vector) =09hash_table_allocated_bytes -=3D bytes; } break; + case PVEC_PROCESS: + { +=09struct Lisp_Process *p =3D PSEUDOVEC_STRUCT (vector, Lisp_Process); +=09xfree (p->gnutls_pproc); + } + break; /* Keep the switch exhaustive. */ case PVEC_NORMAL_VECTOR: case PVEC_FREE: case PVEC_SYMBOL_WITH_POS: case PVEC_MISC_PTR: - case PVEC_PROCESS: case PVEC_FRAME: case PVEC_WINDOW: case PVEC_BOOL_VECTOR: diff --git a/src/gnutls.c b/src/gnutls.c index 334d1d47eb6..7af123b086f 100644 --- a/src/gnutls.c +++ b/src/gnutls.c @@ -27,6 +27,10 @@ #include "buffer.h" #include "pdumper.h" =20 +#ifdef HAVE_MPS +#include "igc.h" +#endif + #ifdef HAVE_GNUTLS =20 # if GNUTLS_VERSION_NUMBER >=3D 0x030014 @@ -702,12 +706,20 @@ emacs_gnutls_handshake (struct Lisp_Process *proc) if (proc->gnutls_initstage < GNUTLS_STAGE_TRANSPORT_POINTERS_SET) { # ifdef WINDOWSNT +#ifdef HAVE_MPS + if (!proc->gnutls_pproc) +=09proc->gnutls_pproc =3D igc_xzalloc_ambig (sizeof *proc->gnutls_pproc); +#else + if (!proc->gnutls_pproc) +=09proc->gnutls_pproc =3D xmalloc (sizeof *proc->gnutls_pproc); +#endif + *proc->gnutls->pproc =3D proc; /* On W32 we cannot transfer socket handles between different runtim= e =09 libraries, so we tell GnuTLS to use our special push/pull =09 functions. */ gnutls_transport_set_ptr2 (state, -=09=09=09=09 (gnutls_transport_ptr_t) proc, -=09=09=09=09 (gnutls_transport_ptr_t) proc); +=09=09=09=09 (gnutls_transport_ptr_t) proc->gnutls_pproc, +=09=09=09=09 (gnutls_transport_ptr_t) proc->gnutls_pproc); gnutls_transport_set_push_function (state, &emacs_gnutls_push); gnutls_transport_set_pull_function (state, &emacs_gnutls_pull); # else diff --git a/src/process.h b/src/process.h index ebc07b23fa7..812059420e1 100644 --- a/src/process.h +++ b/src/process.h @@ -191,6 +191,7 @@ #define EMACS_PROCESS_H #endif =20 #ifdef HAVE_GNUTLS + struct Lisp_Process **gnutls_pproc; gnutls_initstage_t gnutls_initstage; gnutls_session_t gnutls_state; gnutls_certificate_client_credentials gnutls_x509_cred; diff --git a/src/w32.c b/src/w32.c index 31ffa301c2f..6e7425f4099 100644 --- a/src/w32.c +++ b/src/w32.c @@ -11198,7 +11198,7 @@ register_aux_fd (int infd) emacs_gnutls_pull (gnutls_transport_ptr_t p, void* buf, size_t sz) { int n, err; - struct Lisp_Process *process =3D (struct Lisp_Process *)p; + struct Lisp_Process *process =3D *(struct Lisp_Process **)p; int fd =3D process->infd; =20 n =3D sys_read (fd, (char*)buf, sz); @@ -11220,7 +11220,7 @@ emacs_gnutls_pull (gnutls_transport_ptr_t p, void* = buf, size_t sz) ssize_t emacs_gnutls_push (gnutls_transport_ptr_t p, const void* buf, size_t sz) { - struct Lisp_Process *process =3D (struct Lisp_Process *)p; + struct Lisp_Process *process =3D *(struct Lisp_Process **)p; int fd =3D process->outfd; ssize_t n =3D sys_write (fd, buf, sz); =20 There are other solutions which may be better. Pip