From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 7334 invoked by alias); 7 Jan 2015 08:04:32 -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: 34156 Received: (qmail 22578 invoked from network); 7 Jan 2015 08:04:30 -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.7 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.2 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=HzPkXNWcbPJ9ViJH4LgGaGP4D6vPufm9KVFRZgD7mN0=; b=Q3nJVtLttvgxVjanN7jN/MZJ/JHj41BydglgX4MPu5lb7F0rj87yjaYjMkfwI5sngQ vFujpqp5+zj3WqvZPUmZ7jvMunRGXhFAgPFaiJse/aurJzITuBTiEgTPirOdnVE/kfKv +wxM8J1hxXE3Xs1zHNpY5PSbI5Rh+rvhogpENoSRFkH5Y1xR7RhyzHEhMRsIa3lgPRu7 UcD9Htw8BrBqkxOBfE3wrSD5zJ/iqlb/UWe1xjvL+QBq6AtKBqeWA5kD8PY5ocH8Ygks toAqPBs+S6s4IbDCfxwzxgpaDBBoqoNNy8YAbYHdEpVNO13+LFhC1XmT6+y61AAcM/4g i5iA== MIME-Version: 1.0 X-Received: by 10.107.5.7 with SMTP id 7mr1577302iof.1.1420617867368; Wed, 07 Jan 2015 00:04:27 -0800 (PST) In-Reply-To: <150106223532.ZM1050@torch.brasslantern.com> References: <1420590318-17047-1-git-send-email-mikachu@gmail.com> <150106223532.ZM1050@torch.brasslantern.com> Date: Wed, 7 Jan 2015 09:04:27 +0100 Message-ID: Subject: Re: PATCH: Plug some fd leaks in bin_print From: Mikael Magnusson To: Bart Schaefer Cc: zsh workers Content-Type: text/plain; charset=UTF-8 On Wed, Jan 7, 2015 at 7:35 AM, Bart Schaefer wrote: > I have some doubts about some of this. In fact I suspect that these are > mostly false-positives because the option letters tested in the if() > conditions preceding each of these goto's are (or should be) mutually > exclusive. > > On Jan 7, 1:25am, Mikael Magnusson wrote: > } > } if (*eptr) { > } - zwarnnam(name, "number expcted after -%c: %s", 'C', argptr); > } - return 1; > } + zwarnnam(name, "number expected after -%c: %s", 'C', argptr); > } + ret = 1; > } + goto out_print; > } } > } if (nc <= 0) { > } zwarnnam(name, "invalid number of columns: %s", argptr); > } - return 1; > } + ret = 1; > } + goto out_print; > } } > > I'm not sure what coverity had to say here, but these can't be right. The > descriptor should NOT be fflush()d on this error. Coverity only complains about the missing close, I figured fflushing stdout was harmless and unifying the error cleanup paths was neater. > } queue_signals(); > } zpushnode(bufstack, sepjoin(args, NULL, 0)); > } unqueue_signals(); > } - return 0; > } + goto out_print; > > This can't be right either; there shouldn't be any output when pushing on > the buffer stack, so again we shouldn't be fflush()ing. > > > } zwarnnam(name, "option -S takes a single argument"); > } - return 1; > } + ret = 1; > } + goto out_print; > > Same complaint. > > } if (OPT_ISSET(ops,'z') || OPT_ISSET(ops,'s')) { > } + if (fout != stdout) > } + fclose(fout); > } + fout = stdout; > > Why is that needed when the very next couple of lines are going to over- > write fout with a different open? Because if they fail to open their thing, fout will continue to point to something that was closed? > I'm just going to quit complaining about the rest of these; I think > they're all incorrect. If nothing else, they shouldn't introduce the > possibility of printing BOTH an argument-parsing error AND a write- > failed error. > > This needs to be looked at more closely. Okay, I'll resend a version that just does the fclose when things are actually leaking, and leave the others alone. I didn't realize fflush()ing stdout was somehow dangerous sometimes. -- Mikael Magnusson