From: Theo Buehler <tb@theobuehler.org>
Subject: Re: Minor fixes in Fortune game
To: Nir Lichtman <nir@lichtman.org>
Cc: tech@openbsd.org
Date: Sun, 20 Oct 2024 23:08:32 +0200

It would be nice to have some explanations for the fixes. Maybe it's
just grumpy old me, but I find it unpleasant to be faced with a naked
diff, especially if it mixes a few not entirely obvious things that
have nothing to do with each other.

I took the leak fix. Comments inline

On Sat, Oct 05, 2024 at 09:46:10AM +0000, Nir Lichtman wrote:
> diff --git games/fortune/fortune/fortune.c games/fortune/fortune/fortune.c
> index 652422839..9a84b23cd 100644
> --- games/fortune/fortune/fortune.c
> +++ games/fortune/fortune/fortune.c
> @@ -163,8 +163,11 @@ main(int ac, char *av[])
>  
>  	init_prob();
>  	if ((Short_only && minlen_in_list(File_list) > SLEN) ||
> -	    (Long_only && maxlen_in_list(File_list) <= SLEN))
> +	    (Long_only && maxlen_in_list(File_list) <= SLEN)) {
> +		(void)fprintf(stderr,
> +			"no fortunes matching length constraint found\n");

Not sure why it should print something. Care to explain?

>  		return 1;
> +	}
>  
>  	do {
>  		get_fort();
> @@ -251,12 +254,10 @@ fortlen(void)
>  void
>  getargs(int argc, char *argv[])
>  {
> -	int	ignore_case;
> +	bool	ignore_case = false;

This is purely stylistic. Many people dislike bool (not sure why but
they do).

>  	char	*pat = NULL;
>  	int ch;
>  
> -	ignore_case = 0;
> -
>  #ifdef DEBUG
>  	while ((ch = getopt(argc, argv, "aDefhilm:osw")) != -1)
>  #else
> @@ -296,7 +297,7 @@ getargs(int argc, char *argv[])
>  			pat = optarg;
>  			break;
>  		case 'i':			/* case-insensitive match */
> -			ignore_case = 1;
> +			ignore_case = true;
>  			break;
>  		case 'h':
>  		default:
> @@ -1116,6 +1117,7 @@ find_matches(void)
>  
>  	Found_one = false;
>  	matches_in_list(File_list);
> +	free(Fortbuf);

Committed this with an additional Fortbuf = NULL. The only caller will
exit immediately so freeing and NULLing this is inconsequential. If we
choose to fix it let's ensure we crash instead of use-after-freeing or
double freeing.

>  	return Found_one;
>  }
>