From: Theo Buehler <tb@theobuehler.org>
Subject: Re: pppd(8): cleanup
To: Denis Fondras <denis@openbsd.org>
Cc: tech@openbsd.org
Date: Sat, 17 Aug 2024 17:18:25 +0200

On Sat, Aug 17, 2024 at 03:51:02PM +0200, Denis Fondras wrote:
> Fixing a case of missing braces...

comments inline.

> Index: fsm.c

Not saying this is wrong, but I couldn't convince myself of the
correctness of this change within 5 minutes...

> ===================================================================
> RCS file: /cvs/src/usr.sbin/pppd/fsm.c,v
> diff -u -p -r1.9 fsm.c
> --- fsm.c	9 Aug 2024 05:16:13 -0000	1.9
> +++ fsm.c	17 Aug 2024 13:47:41 -0000
> @@ -399,6 +399,7 @@ fsm_rconfreq(fsm *f, int id, u_char *inp
>  	if( f->callbacks->down )
>  	    (*f->callbacks->down)(f);	/* Inform upper layers */
>  	fsm_sconfreq(f, 0);		/* Send initial Configure-Request */
> +	f->state = REQSENT;
>  	break;
>  
>      case STOPPED:
> @@ -434,10 +435,10 @@ fsm_rconfreq(fsm *f, int id, u_char *inp
>  	f->nakloops = 0;
>  
>      } else {
> -	/* we sent CONFACK or CONFREJ */
> +	/* we sent CONFNACK or CONFREJ */
>  	if (f->state != ACKRCVD)
>  	    f->state = REQSENT;
> -	if( code == CONFNAK )
> +	if(code == CONFNAK)

missing space after if.

>  	    ++f->nakloops;
>      }
>  }
> @@ -537,8 +538,10 @@ fsm_rconfnakrej(fsm *f, int code, int id
>  	UNTIMEOUT(fsm_timeout, f);	/* Cancel timeout */
>  	if (ret < 0)
>  	    f->state = STOPPED;		/* kludge for stopping CCP */
> -	else
> +	else {
>  	    fsm_sconfreq(f, 0);		/* Send Configure-Request */
> +	    f->state = REQSENT;
> +	}
>  	break;
>  
>      case ACKRCVD:
> @@ -624,6 +627,7 @@ fsm_rtermack(fsm *f)
>  	if (f->callbacks->down)
>  	    (*f->callbacks->down)(f);	/* Inform upper layers */
>  	fsm_sconfreq(f, 0);
> +	f->state = REQSENT;
>  	break;
>      }
>  }
> Index: chat/chat.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/pppd/chat/chat.c,v
> diff -u -p -r1.37 chat.c
> --- chat/chat.c	10 Aug 2024 05:32:28 -0000	1.37
> +++ chat/chat.c	17 Aug 2024 13:47:41 -0000
> @@ -172,7 +172,6 @@ int clear_report_next = 0;
>  
>  int say_next = 0, hup_next = 0;
>  
> -void *dup_mem(void *b, size_t c);
>  void usage(void);
>  void logmsg(const char *fmt, ...);
>  void fatal(int code, const char *fmt, ...);
> @@ -204,22 +203,6 @@ int vfmtmsg(char *, int, const char *, v
>  
>  int main(int, char *[]);
>  
> -void *
> -dup_mem(void *b, size_t c)
> -{
> -    void *ans = malloc (c);
> -    if (!ans)
> -	fatal(2, "memory error!");
> -
> -    memcpy (ans, b, c);
> -    return ans;
> -}
> -
> -void *copy_of (char *s)
> -{
> -    return dup_mem (s, strlen (s) + 1);
> -}
> -
>  /*
>   * chat [ -v ] [-T number] [-U number] [ -t timeout ] [ -f chat-file ] \
>   * [ -r report-file ] \
> @@ -257,7 +240,8 @@ main(int argc, char **argv)
>  	    break;
>  
>  	case 'f':
> -	    chat_file = strdup(optarg);
> +	    if ((chat_file = strdup(optarg)) == NULL)
> +		fatal(2, "chat_file memory error!");

I would not include the variable names. Just 'fatal(2, "memory error!");'
in all those cases seems enough. With that the chat.c change is ok tb

>  	    break;
>  
>  	case 't':
> @@ -267,7 +251,8 @@ main(int argc, char **argv)
>  	case 'r':
>  	    if (report_fp != NULL)
>  		fclose (report_fp);
> -	    report_file = copy_of (optarg);
> +	    if ((report_file = strdup(optarg)) == NULL)
> +		fatal(2, "report_file memory error!");
>  	    report_fp   = fopen (report_file, "a");
>  	    if (report_fp != NULL) {
>  		if (verbose)
> @@ -278,11 +263,14 @@ main(int argc, char **argv)
>  	    break;
>  
>  	case 'T':
> -	    phone_num = strdup(optarg);
> +	    if ((phone_num = strdup(optarg)) == NULL)
> +		fatal(2, "phone_num memory error!");
>  	    break;
>  
>  	case 'U':
>  	    phone_num2 = strdup(optarg);
> +	    if ((phone_num2 = strdup(optarg)) == NULL)
> +		fatal(2, "phone_num2 memory error!");
>  	    break;
>  
>  	case ':':