[PATCH] test-crontab: minor memory and error handling fixes

From: Jeff King <peff@peff.net>
Date: Aug 30, 2022, 20:40 UTC
Message-ID: <Yw512HXz/SV50ckc@coredump.intra.peff.net>
In-Reply-To: <20220828214143.754759-1-sandals@crustytoothpaste.net>

On Sun, Aug 28, 2022 at 09:41:43PM +0000, brian m. carlson wrote:

Show 15 quoted lines

> 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 <peff@peff.net>
---
 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 <file> [-l]
+ * Usage: test-tool crontab <file> -l|<input>
  *
  * If -l is specified, then write the contents of <file> to stdout.
- * Otherwise, write from stdin into <file>.
+ * Otherwise, copy the contents of <input> into <file>.
  */
 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 <file> -l|<input>");
+
+	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