From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Lars Ingebrigtsen Newsgroups: gmane.emacs.bugs Subject: bug#23808: Emacs 25 open-network-stream, make-network-process Date: Thu, 11 Aug 2016 12:41:22 +0200 Message-ID: References: <5767C0A3.9010902@cs.ucla.edu> <4dc79eb4-e2b6-9f33-cf61-af1971e10275@cs.ucla.edu> NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: text/plain X-Trace: blaine.gmane.org 1470912301 15810 195.159.176.226 (11 Aug 2016 10:45:01 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Thu, 11 Aug 2016 10:45:01 +0000 (UTC) User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1.50 (gnu/linux) Cc: 23808@debbugs.gnu.org To: Paul Eggert Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Thu Aug 11 12:44:54 2016 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by blaine.gmane.org with esmtp (Exim 4.84_2) (envelope-from ) id 1bXnTq-0003r4-Er for geb-bug-gnu-emacs@m.gmane.org; Thu, 11 Aug 2016 12:44:54 +0200 Original-Received: from localhost ([::1]:47509 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bXnTn-0004iX-Du for geb-bug-gnu-emacs@m.gmane.org; Thu, 11 Aug 2016 06:44:51 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:45241) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bXnS5-0002ge-8g for bug-gnu-emacs@gnu.org; Thu, 11 Aug 2016 06:43:06 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bXnS2-0004jK-1e for bug-gnu-emacs@gnu.org; Thu, 11 Aug 2016 06:43:05 -0400 Original-Received: from debbugs.gnu.org ([208.118.235.43]:55314) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bXnS1-0004jG-Tq for bug-gnu-emacs@gnu.org; Thu, 11 Aug 2016 06:43:01 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1bXnS1-0007x6-PV for bug-gnu-emacs@gnu.org; Thu, 11 Aug 2016 06:43:01 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Lars Ingebrigtsen Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Thu, 11 Aug 2016 10:43:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 23808 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: patch Original-Received: via spool by 23808-submit@debbugs.gnu.org id=B23808.147091213630501 (code B ref 23808); Thu, 11 Aug 2016 10:43:01 +0000 Original-Received: (at 23808) by debbugs.gnu.org; 11 Aug 2016 10:42:16 +0000 Original-Received: from localhost ([127.0.0.1]:53022 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1bXnRH-0007vt-Fv for submit@debbugs.gnu.org; Thu, 11 Aug 2016 06:42:15 -0400 Original-Received: from hermes.netfonds.no ([80.91.224.195]:35658) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1bXnRF-0007vi-2d for 23808@debbugs.gnu.org; Thu, 11 Aug 2016 06:42:13 -0400 Original-Received: from cm-84.215.1.64.getinternet.no ([84.215.1.64] helo=stories) by hermes.netfonds.no with esmtpsa (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.84_2) (envelope-from ) id 1bXnR3-0007p6-O9; Thu, 11 Aug 2016 12:42:06 +0200 Face: iVBORw0KGgoAAAANSUhEUgAAADAAAAAwBAMAAAClLOS0AAAAGFBMVEX9+cIFAgZOhagCAAH+ /+E4YIWqu6QYHi9B9JI1AAACLUlEQVQ4jW2TTY+jMAyGrVGbPUdolWtlkcw54g9wQD2PojVcESLp mV1l8vfXDtDOflhKad4Hv/mwAQBQ0dmAe1hLIY5wRErE0oECvoASgfCMLyA8rSSSO50wWKSa4VIK TqWqWuLAZTNUEG+cLYMBTaYQBasNZsS2VzTDNwFam1IovDNgs3VQYYaIDDqvtS6SkTAtAi5AAryA 3F7zONS44wCZB1uVTef1GnZ9UOOgKsguxZJTbE/g2E3ADaC35OJ6gPs4XAP2rMJluJKz/Pgb8IQc ppnV/jLcPxi4Xs7Oc3KO5fr/BcTCuvbQBUR8AnS3y7n4jxPIbHFuPkvwB3hzr2K0vQ0ODnB9AWzB 1sWr9xXDV4DuWJtdw1Fyi6t0AIN5BxPXJRMVbdqF34EIctu9xcnrxhuh6xIwqALEt6csWu0bQ1Ka dRnj+ChQOEUR2qYzAadSfq7x4zG+FzDh0r+RSNwrUw44x5lu0wY+3+5LBTojbZkSLjQXDZ0Zgfdq N+mWrdRWoiTAZ6CAllNqcOMRKQFdnjmBfnV7yHHCCaKlrYo8vC40CvCdmSNNVZIWk6OMe4ZxfBUN vy9L6IZP/xG3CnhFrZuaU3/9Z9QC/gn/+fg/0N8PoP/U+ZZ34Ddet7rveqf9dlj5UrJsoBPc+Ea2 XYEpXLxJEO+u2U9TgZYrTwsXtkZ3ZnjNZRikx9Skz9ityO39CfDYvgBfwvnZAEzlBZ4JQtQL8Be+ PkF/mv0G91Xqa8i3xrAAAAAASUVORK5CYII= In-Reply-To: <4dc79eb4-e2b6-9f33-cf61-af1971e10275@cs.ucla.edu> (Paul Eggert's message of "Thu, 11 Aug 2016 02:14:18 -0700") X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 208.118.235.43 X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.org gmane.emacs.bugs:122063 Archived-At: Paul Eggert writes: > [CC'ing this to Lars, since this bug seems to have been introduced by > the async DNS changes in February.] > > Attached is a proposed patch for Bug#23808. The patch attempts to fix > the bug reported by my anonymous correspondent, who writes "It appears > that before the refactoring, make-network-process would not create a > process object if the socket connection failed, or if it had been > interrupted by ^G. However after the new refactoring, the process > object is created upfront with status 'run', before the connection is > attempted. The refactoring failed to preserve the semantics." > > From a21dfe515552c7d7f486950e9231b8ca2668f6cc Mon Sep 17 00:00:00 2001 > From: Paul Eggert > Date: Thu, 11 Aug 2016 01:56:16 -0700 > Subject: [PATCH] Fix process leak with make-network-process > > This problem was introduced by the recent async changes (Bug#23808). > * src/process.c (Fmake_process): Move USE_SAFE_ALLOCA later, > so that it follows the start_process_unwind unwind-protect. > Set pid to -1 while the process is being created. > (start_process_unwind): Omit unnecessary emacs_abort test. > (connect_network_socket): Simplify use of counts. Unwind > bind_polling_period a bit earlier, so that a remove_process > unwind-protect can be added when needed; this is the heart of > the fix. Undo the unwind-protect just before returning. > --- > src/process.c | 49 ++++++++++++++++++------------------------------- > src/process.h | 9 +++++---- > 2 files changed, 23 insertions(+), 35 deletions(-) > > diff --git a/src/process.c b/src/process.c > index fb32698..69d1b2a 100644 > --- a/src/process.c > +++ b/src/process.c > @@ -238,6 +238,7 @@ static int process_output_delay_count; > > static bool process_output_skip; > > +static void start_process_unwind (Lisp_Object); > static void create_process (Lisp_Object, char **, Lisp_Object); > #ifdef USABLE_SIGIO > static bool keyboard_bit_set (fd_set *); > @@ -245,11 +246,8 @@ static bool keyboard_bit_set (fd_set *); > static void deactivate_process (Lisp_Object); > static int status_notify (struct Lisp_Process *, struct Lisp_Process *); > static int read_process_output (Lisp_Object, int); > -static void handle_child_signal (int); > static void create_pty (Lisp_Object); > - > -static Lisp_Object get_process (register Lisp_Object name); > -static void exec_sentinel (Lisp_Object proc, Lisp_Object reason); > +static void exec_sentinel (Lisp_Object, Lisp_Object); > > /* Mask of bits indicating the descriptors that we wait for input on. */ > > @@ -1407,8 +1405,6 @@ DEFUN ("process-list", Fprocess_list, Sprocess_list, 0, 0, 0, > > /* Starting asynchronous inferior processes. */ > > -static void start_process_unwind (Lisp_Object proc); > - > DEFUN ("make-process", Fmake_process, Smake_process, 0, MANY, 0, > doc: /* Start a program in a subprocess. Return the process object for it. > > @@ -1459,7 +1455,6 @@ usage: (make-process &rest ARGS) */) > Lisp_Object buffer, name, command, program, proc, contact, current_dir, tem; > Lisp_Object xstderr, stderrproc; > ptrdiff_t count = SPECPDL_INDEX (); > - USE_SAFE_ALLOCA; > > if (nargs == 0) > return Qnil; > @@ -1508,10 +1503,6 @@ usage: (make-process &rest ARGS) */) > } > > proc = make_process (name); > - /* If an error occurs and we can't start the process, we want to > - remove it from the process list. This means that each error > - check in create_process doesn't need to call remove_process > - itself; it's all taken care of here. */ > record_unwind_protect (start_process_unwind, proc); > > pset_childp (XPROCESS (proc), Qt); > @@ -1561,6 +1552,8 @@ usage: (make-process &rest ARGS) */) > BUF_ZV (XBUFFER (buffer)), > BUF_ZV_BYTE (XBUFFER (buffer))); > > + USE_SAFE_ALLOCA; > + > { > /* Decide coding systems for communicating with the process. Here > we don't setup the structure coding_system nor pay attention to > @@ -1719,18 +1712,11 @@ usage: (make-process &rest ARGS) */) > return unbind_to (count, proc); > } > > -/* This function is the unwind_protect form for Fstart_process. If > - PROC doesn't have its pid set, then we know someone has signaled > - an error and the process wasn't started successfully, so we should > - remove it from the process list. */ > +/* If PROC doesn't have its pid set, then an error was signaled and > + the process wasn't started successfully, so remove it. */ > static void > start_process_unwind (Lisp_Object proc) > { > - if (!PROCESSP (proc)) > - emacs_abort (); > - > - /* Was PROC started successfully? > - -2 is used for a pty with no process, eg for gdb. */ > if (XPROCESS (proc)->pid <= 0 && XPROCESS (proc)->pid != -2) > remove_process (proc); > } > @@ -3124,7 +3110,6 @@ connect_network_socket (Lisp_Object proc, Lisp_Object addrinfos, > Lisp_Object use_external_socket_p) > { > ptrdiff_t count = SPECPDL_INDEX (); > - ptrdiff_t count1; > int s = -1, outch, inch; > int xerrno = 0; > int family; > @@ -3145,7 +3130,6 @@ connect_network_socket (Lisp_Object proc, Lisp_Object addrinfos, > } > > /* Do this in case we never enter the while-loop below. */ > - count1 = SPECPDL_INDEX (); > s = -1; > > while (!NILP (addrinfos)) > @@ -3313,7 +3297,7 @@ connect_network_socket (Lisp_Object proc, Lisp_Object addrinfos, > immediate_quit = 0; > > /* Discard the unwind protect closing S. */ > - specpdl_ptr = specpdl + count1; > + specpdl_ptr = specpdl + count; > emacs_close (s); > s = -1; > if (0 <= socket_to_use) > @@ -3398,10 +3382,7 @@ connect_network_socket (Lisp_Object proc, Lisp_Object addrinfos, > p->outfd = outch; > > /* Discard the unwind protect for closing S, if any. */ > - specpdl_ptr = specpdl + count1; > - > - /* Unwind bind_polling_period and request_sigio. */ > - unbind_to (count, Qnil); > + specpdl_ptr = specpdl + count; > > if (p->is_server && p->socktype != SOCK_DGRAM) > pset_status (p, Qlisten); > @@ -3925,7 +3906,12 @@ usage: (make-network-process &rest ARGS) */) > > if (!NILP (buffer)) > buffer = Fget_buffer_create (buffer); > + > + /* Unwind bind_polling_period. */ > + unbind_to (count, Qnil); > + > proc = make_process (name); > + record_unwind_protect (remove_process, proc); > p = XPROCESS (proc); > pset_childp (p, contact); > pset_plist (p, Fcopy_sequence (Fplist_get (contact, QCplist))); > @@ -3956,8 +3942,6 @@ usage: (make-network-process &rest ARGS) */) > > set_network_socket_coding_system (proc, host, service, name); > > - unbind_to (count, Qnil); > - > /* :server BOOL */ > tem = Fplist_get (contact, QCserver); > if (!NILP (tem)) > @@ -3974,6 +3958,7 @@ usage: (make-network-process &rest ARGS) */) > && !NILP (Fplist_get (contact, QCnowait))) > p->is_non_blocking_client = true; > > + bool postpone_connection = false; > #ifdef HAVE_GETADDRINFO_A > /* With async address resolution, the list of addresses is empty, so > postpone connecting to the server. */ > @@ -3981,11 +3966,13 @@ usage: (make-network-process &rest ARGS) */) > { > p->dns_request = dns_request; > p->status = list1 (Qconnect); > - return proc; > + postpone_connection = true; > } > #endif > + if (! postpone_connection) > + connect_network_socket (proc, addrinfos, use_external_socket_p); > > - connect_network_socket (proc, addrinfos, use_external_socket_p); > + specpdl_ptr = specpdl + count; > return proc; > } > > diff --git a/src/process.h b/src/process.h > index 6c227bc..9926050 100644 > --- a/src/process.h > +++ b/src/process.h > @@ -118,10 +118,11 @@ struct Lisp_Process > /* After this point, there are no Lisp_Objects any more. */ > /* alloc.c assumes that `pid' is the first such non-Lisp slot. */ > > - /* Number of this process. > - allocate_process assumes this is the first non-Lisp_Object field. > - A value 0 is used for pseudo-processes such as network or serial > - connections. */ > + /* Process ID. A positive value is a child process ID. > + Zero is for pseudo-processes such as network or serial connections, > + or for processes that have not been fully created yet. > + -1 is for a process that was not created successfully. > + -2 is for a pty with no process, e.g., for GDB. */ > pid_t pid; > /* Descriptor by which we read from this process. */ > int infd; -- (domestic pets only, the antidote for overdose, milk.) bloggy blog: http://lars.ingebrigtsen.no