From: Nir Lichtman <nir@lichtman.org>
Subject: Re: Minor fixes in Fortune game
To: Theo Buehler <tb@theobuehler.org>
Cc: tech@openbsd.org
Date: Mon, 21 Oct 2024 04:03:26 +0000

On Sun, Oct 20, 2024 at 11:08:32PM +0200, Theo Buehler wrote:
> 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.
> 

Sorry about that, noted for next times, I should have split to separate patches
instead of putting all of these over here.

Adding comments below to explain.

> 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?

The idea behind this is to be more indicative in the case a custom fortune list
is chosen in which all the fortunes are too short/too long, in the current
situtation it would just exit with a generic failure, which can be confusing for
the end user, there are more exits like this in the code, and optimally 
all would print something, but started with this.
The code already contains some paths which also print an error, besides exitting.

> 
> >  		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).

The code here already contains a couple of booleans so thought this would fit in
to be more consistent

> 
> >  	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;
> >  }
> >