From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS3215 2.6.0.0/16 X-Spam-Status: No, score=-3.8 required=3.0 tests=AWL,BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,T_SCC_BODY_TEXT_LINE shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from out1.vger.email (out1.vger.email [IPv6:2620:137:e000::1:20]) by dcvr.yhbt.net (Postfix) with ESMTP id 787E91F54E for ; Tue, 30 Aug 2022 20:41:05 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229954AbiH3UlC (ORCPT ); Tue, 30 Aug 2022 16:41:02 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:47676 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230038AbiH3Uk7 (ORCPT ); Tue, 30 Aug 2022 16:40:59 -0400 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 0B81823151 for ; Tue, 30 Aug 2022 13:40:57 -0700 (PDT) Received: (qmail 7720 invoked by uid 109); 30 Aug 2022 20:40:57 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Tue, 30 Aug 2022 20:40:57 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 29437 invoked by uid 111); 30 Aug 2022 20:40:57 -0000 Received: from coredump.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Tue, 30 Aug 2022 16:40:57 -0400 Authentication-Results: peff.net; auth=none Date: Tue, 30 Aug 2022 16:40:56 -0400 From: Jeff King To: "brian m. carlson" Cc: git@vger.kernel.org, Junio C Hamano , Johannes Schindelin , Derrick Stolee , Renato Botelho Subject: [PATCH] test-crontab: minor memory and error handling fixes Message-ID: References: <20220823010120.25388-1-sandals@crustytoothpaste.net> <20220828214143.754759-1-sandals@crustytoothpaste.net> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20220828214143.754759-1-sandals@crustytoothpaste.net> Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org On Sun, Aug 28, 2022 at 09:41:43PM +0000, brian m. carlson wrote: > diff --git a/t/helper/test-crontab.c b/t/helper/test-crontab.c > index e7c0137a47..2942543046 100644 > --- a/t/helper/test-crontab.c > +++ b/t/helper/test-crontab.c > @@ -17,8 +17,8 @@ int cmd__crontab(int argc, const char **argv) > if (!from) > return 0; > to = stdout; > - } else if (argc == 2) { > - from = stdin; > + } else if (argc == 3) { > + from = fopen(argv[2], "r"); > to = fopen(argv[1], "w"); > } else > return error("unknown arguments"); After this commit we know that argc must be 3, so that makes the "else" in the cleanup section dead code: if (argc == 3) fclose(from); else fclose(to); While fixing that, I noticed a ton of other small problems, so I just lumped them all together (which I think is OK given the relative insignificance of this program). I do have to wonder if this really could be replaced by a call to "cp". ;) -- >8 -- Subject: [PATCH] test-crontab: minor memory and error handling fixes Since ee69e7884e (gc: use temporary file for editing crontab, 2022-08-28), we now insist that "argc == 3" (and otherwise return an error). Coverity notes that this causes some dead code: if (argc == 3) fclose(from); else fclose(to); as we will never trigger the else. This also causes a memory leak, since we'll never close "to". Now that all paths require 2 arguments, we can just reorganize the function to check argc up front, and tweak the cleanup to do the right thing for all cases. While we're here, we can also notice some minor problems: - we return a negative int via error() from what is essentially a main() function; we should return a positive non-zero value for error. Or better yet, we can just use usage(), which gives a better message. - while writing the usage message, we can note the one in the comment was made out of date by ee69e7884e. But it also had a typo already, calling the subcommand "cron" and not "crontab" - we didn't check for an error from fopen(), meaning we would segfault if the to-be-read file was missing. We can use xfopen() to catch this. Signed-off-by: Jeff King --- t/helper/test-crontab.c | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/t/helper/test-crontab.c b/t/helper/test-crontab.c index 2942543046..e6c1b1e22b 100644 --- a/t/helper/test-crontab.c +++ b/t/helper/test-crontab.c @@ -2,33 +2,34 @@ #include "cache.h" /* - * Usage: test-tool cron [-l] + * Usage: test-tool crontab -l| * * If -l is specified, then write the contents of to stdout. - * Otherwise, write from stdin into . + * Otherwise, copy the contents of into . */ int cmd__crontab(int argc, const char **argv) { int a; FILE *from, *to; - if (argc == 3 && !strcmp(argv[2], "-l")) { + if (argc != 3) + usage("test-tool crontab -l|"); + + if (!strcmp(argv[2], "-l")) { from = fopen(argv[1], "r"); if (!from) return 0; to = stdout; - } else if (argc == 3) { - from = fopen(argv[2], "r"); - to = fopen(argv[1], "w"); - } else - return error("unknown arguments"); + } else { + from = xfopen(argv[2], "r"); + to = xfopen(argv[1], "w"); + } while ((a = fgetc(from)) != EOF) fputc(a, to); - if (argc == 3) - fclose(from); - else + fclose(from); + if (to != stdout) fclose(to); return 0; -- 2.37.3.1051.g85dc4064ac