From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21495 invoked by alias); 17 Jul 2013 20:25:48 -0000 Mailing-List: contact zsh-workers-help@zsh.org; run by ezmlm Precedence: bulk X-No-Archive: yes List-Id: Zsh Workers List List-Post: List-Help: X-Seq: 31528 Received: (qmail 5773 invoked from network); 17 Jul 2013 20:25:33 -0000 X-Spam-Checker-Version: SpamAssassin 3.3.2 (2011-06-06) on f.primenet.com.au X-Spam-Level: X-Spam-Status: No, score=-2.6 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.2 Received-SPF: neutral (ns1.primenet.com.au: 209.85.215.174 is neither permitted nor denied by SPF record at ntlworld.com) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=x-proxyuser-ip:date:from:to:subject:message-id:in-reply-to :references:x-mailer:mime-version:content-type :content-transfer-encoding:x-gm-message-state; bh=a+vK+7s4ss3gP6sFVEcF59ZCEuOGKD56rq6/vE8BlhM=; b=aFAlZslfM+OiKizkxU3mCxwn9PRI0UR8mWkfQSQLYQKhncgt40GWgaGcNQ53KAtVEj q10QpAugPYk2bo/u9TxkaOT44ni2Q1sv2MlSq5yIGfaGm1X/1+cwICfniqKrlfFBqJmU dWGAPnkKTyFUZSRD8pT0Uu1Fq/0IAKrrhQ09zns9z4XJISBMvR5vj9M48XEmny4SgBSO Y5Z7L+oQzzx03CowCQRZ8Z8dENhs5iJRHm2/52PYGwFWG2WJf1p5OtG5zuwCcozP/lC9 uBc5sQm5pVPAWEuzdfYJJZmNt1LFluP++HHprSNTJKEY6Mq6VasV+YTweTgeE7++uA6V uq9w== X-Received: by 10.14.99.71 with SMTP id w47mr7693823eef.140.1374088657202; Wed, 17 Jul 2013 12:17:37 -0700 (PDT) X-ProxyUser-IP: 86.6.30.159 Date: Wed, 17 Jul 2013 20:17:33 +0100 From: Peter Stephenson To: zsh-workers@zsh.org Subject: Re: bug with eval, proc-subst and pipes Message-ID: <20130717201733.2c0b029b@pws-pc.ntlworld.com> In-Reply-To: <130717000027.ZM15643@torch.brasslantern.com> References: <20130715133525.GA7694@chaz.gmail.com> <130715100624.ZM14123@torch.brasslantern.com> <20130716215540.22d88a27@pws-pc.ntlworld.com> <130717000027.ZM15643@torch.brasslantern.com> X-Mailer: Claws Mail 3.8.0 (GTK+ 2.24.7; x86_64-redhat-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-Gm-Message-State: ALoCoQll04q0/KXbBLkY5KB468ZO03hwOnNGk4B15ppdIh0Xe6L6xli0W3xL10Jf9IeesaS6P/ol On Wed, 17 Jul 2013 00:00:27 -0700 Bart Schaefer wrote: > On Jul 16, 9:55pm, Peter Stephenson wrote: > } It may therefore be a different problem from Stephane's. > > I don't think so, fundamentally. The problem in both cases is that the > file descriptor for the process substitution is closed before the data > is read. This has the unintended side-effect of removing the special > device file. The reasons for the premature descriptor close may be > different, but the end result is the same; I believe fixing either one > would fix both. We can see... > } I suppose a smarter way of marking such file descriptors is needed, > } perhaps one associated with the appropriate job in the same way as > } auxiliary processes are remembered. Good luck avoiding leaks... > > Not in the way auxiliary processes are remembered; in the way temporary > files are remembered. With #undef PATH_DEV_FD, zsh seems to do a very > good job of cleaning up those FIFOs. In that case, this patch does the exact equivalent. I've made the entries in the temporary file list larger rather than adding a separate entry to the job since it should use less memory in general. It seems to fix the completely reproducible variant. diff --git a/Src/exec.c b/Src/exec.c index 75805d3..d462d97 100644 --- a/Src/exec.c +++ b/Src/exec.c @@ -2860,9 +2860,6 @@ execcmd(Estate state, int input, int output, int how, int last1) close(synch[1]); read_loop(synch[0], &dummy, 1); close(synch[0]); -#ifdef PATH_DEV_FD - closem(FDT_PROC_SUBST); -#endif if (how & Z_ASYNC) { lastpid = (zlong) pid; /* indicate it's possible to set status for lastpid */ @@ -3247,32 +3244,16 @@ execcmd(Estate state, int input, int output, int how, int last1) if (is_shfunc) { /* It's a shell function */ -#ifdef PATH_DEV_FD - int i; - - for (i = 10; i <= max_zsh_fd; i++) - if (fdtable[i] >= FDT_PROC_SUBST) - fdtable[i]++; -#endif if (subsh_close >= 0) zclose(subsh_close); subsh_close = -1; execshfunc((Shfunc) hn, args); -#ifdef PATH_DEV_FD - for (i = 10; i <= max_zsh_fd; i++) - if (fdtable[i] >= FDT_PROC_SUBST) - if (--(fdtable[i]) <= FDT_PROC_SUBST) - zclose(i); -#endif } else { /* It's a builtin */ if (forked) closem(FDT_INTERNAL); lastval = execbuiltin(args, (Builtin) hn); -#ifdef PATH_DEV_FD - closem(FDT_PROC_SUBST); -#endif fflush(stdout); if (save[1] == -2) { if (ferror(stdout)) { @@ -3887,9 +3868,7 @@ getoutputfile(char *cmd, char **eptr) untokenize(s); } - if (!jobtab[thisjob].filelist) - jobtab[thisjob].filelist = znewlinklist(); - zaddlinknode(jobtab[thisjob].filelist, nam); + addfilelist(nam, 0); if (!s) child_block(); @@ -3975,9 +3954,7 @@ getproc(char *cmd, char **eptr) return NULL; if (!(prog = parsecmd(cmd, eptr))) return NULL; - if (!jobtab[thisjob].filelist) - jobtab[thisjob].filelist = znewlinklist(); - zaddlinknode(jobtab[thisjob].filelist, ztrdup(pnam)); + addfilelist(pnam, 0); if ((pid = zfork(&bgtime))) { if (pid == -1) @@ -3995,7 +3972,7 @@ getproc(char *cmd, char **eptr) entersubsh(ESUB_ASYNC|ESUB_PGRP); redup(fd, out); #else /* PATH_DEV_FD */ - int pipes[2]; + int pipes[2], fd; if (thisjob == -1) return NULL; @@ -4012,7 +3989,9 @@ getproc(char *cmd, char **eptr) zclose(pipes[!out]); return NULL; } - fdtable[pipes[!out]] = FDT_PROC_SUBST; + fd = pipes[!out]; + fdtable[fd] = FDT_PROC_SUBST; + addfilelist(NULL, fd); if (!out) { addproc(pid, NULL, 1, &bgtime); diff --git a/Src/jobs.c b/Src/jobs.c index 0dbb10b..a1955bb 100644 --- a/Src/jobs.c +++ b/Src/jobs.c @@ -1113,16 +1113,48 @@ printjob(Job jn, int lng, int synch) return doneprint; } +/* Add a file to be deleted or fd to be closed to the current job */ + +/**/ +void +addfilelist(const char *name, int fd) +{ + Jobfile jf = (Jobfile)zalloc(sizeof(struct jobfile)); + LinkList ll = jobtab[thisjob].filelist; + + if (!ll) + ll = jobtab[thisjob].filelist = znewlinklist(); + if (name) + { + jf->u.name = ztrdup(name); + jf->is_fd = 0; + } + else + { + jf->u.fd = fd; + jf->is_fd = 1; + } + zaddlinknode(ll, jf); +} + +/* Finished with list of files for a job */ + /**/ void deletefilelist(LinkList file_list, int disowning) { - char *s; + Jobfile jf; if (file_list) { - while ((s = (char *)getlinknode(file_list))) { - if (!disowning) - unlink(s); - zsfree(s); + while ((jf = (Jobfile)getlinknode(file_list))) { + if (jf->is_fd) { + if (!disowning) + zclose(jf->u.fd); + } else { + if (!disowning) + unlink(jf->u.name); + zsfree(jf->u.name); + } + zfree(jf, sizeof(*jf)); } zfree(file_list, sizeof(struct linklist)); } diff --git a/Src/zsh.h b/Src/zsh.h index 299357d..ebd3cb7 100644 --- a/Src/zsh.h +++ b/Src/zsh.h @@ -374,9 +374,8 @@ enum { #ifdef PATH_DEV_FD /* * Entry used by a process substition. - * The value will be incremented on entering a function and - * decremented on exit; we don't close entries greater than - * FDT_PROC_SUBST except when closing everything. + * This marker is not tested internally as we associated the file + * descriptor with a job for closing. */ #define FDT_PROC_SUBST 6 #endif @@ -422,6 +421,7 @@ typedef struct heap *Heap; typedef struct heapstack *Heapstack; typedef struct histent *Histent; typedef struct hookdef *Hookdef; +typedef struct jobfile *Jobfile; typedef struct job *Job; typedef struct linkedmod *Linkedmod; typedef struct linknode *LinkNode; @@ -878,6 +878,18 @@ struct eccstr { /* Definitions for job table and job control */ /********************************************/ +/* Entry in filelist linked list in job table */ + +struct jobfile { + /* Record to be deleted or closed */ + union { + char *name; /* Name of file to delete */ + int fd; /* File descriptor to close */ + } u; + /* Discriminant */ + int is_fd; +}; + /* entry in the job table */ struct job { @@ -889,6 +901,7 @@ struct job { struct process *procs; /* list of processes */ struct process *auxprocs; /* auxiliary processes e.g multios */ LinkList filelist; /* list of files to delete when done */ + /* elements are struct jobfile */ int stty_in_env; /* if STTY=... is present */ struct ttyinfo *ty; /* the modes specified by STTY */ }; -- Peter Stephenson Web page now at http://homepage.ntlworld.com/p.w.stephenson/