svn commit: r320761 - head/sbin/init

[Konstantin Belousov]

On Fri, Jul 07, 2017 at 02:48:55AM +0000, Xin LI wrote:
> Author: delphij
> Date: Fri Jul  7 02:48:55 2017
> New Revision: 320761
> URL: https://svnweb.freebsd.org/changeset/base/320761
> 
> Log:
>    - Use strlcat() instead of strncat().
>    - Use asprintf() and handle allocation errors.
>   
>   Reviewed by:	kevlo
>   MFC after:	2 weeks
>   Differential Revision:	https://reviews.freebsd.org/D11486
> 
> Modified:
>   head/sbin/init/init.c
> 
> Modified: head/sbin/init/init.c
> ==============================================================================
> --- head/sbin/init/init.c	Fri Jul  7 00:34:51 2017	(r320760)
> +++ head/sbin/init/init.c	Fri Jul  7 02:48:55 2017	(r320761)
> @@ -1271,8 +1271,8 @@ new_session(session_t *sprev, struct ttyent *typ)
>  
>  	sp->se_flags |= SE_PRESENT;
>  
> -	sp->se_device = malloc(sizeof(_PATH_DEV) + strlen(typ->ty_name));
> -	sprintf(sp->se_device, "%s%s", _PATH_DEV, typ->ty_name);
> +	if (asprintf(&sp->se_device, "%s%s", _PATH_DEV, typ->ty_name) < 0)
> +		err(1, "asprintf");
>  
IMO this is wrong.  init(8) too important for the system operations,
and panicing the machine due to error from attempt creating getty
session is not worth it.

Either session should be disabled, or retried after some time, or
some other measures taken, but please do not kill init just due to a
local error.

I would even argue that using snprintf() there and ignoring truncation
is much better than err(), not least because the problem probably can
only practically appear due to a misconfiguration.

>  	/*
>  	 * Attempt to open the device, if we get "device not configured"
> @@ -1315,8 +1315,8 @@ setupargv(session_t *sp, struct ttyent *typ)
>  		free(sp->se_getty_argv_space);
>  		free(sp->se_getty_argv);
>  	}
> -	sp->se_getty = malloc(strlen(typ->ty_getty) + strlen(typ->ty_name) + 2);
> -	sprintf(sp->se_getty, "%s %s", typ->ty_getty, typ->ty_name);
> +	if (asprintf(&sp->se_getty, "%s %s", typ->ty_getty, typ->ty_name) < 0)
> +		err(1, "asprintf");
Same there.

>  	sp->se_getty_argv_space = strdup(sp->se_getty);
>  	sp->se_getty_argv = construct_argv(sp->se_getty_argv_space);
>  	if (sp->se_getty_argv == NULL) {
> @@ -1429,7 +1429,7 @@ start_window_system(session_t *sp)
>  	if (sp->se_type) {
>  		/* Don't use malloc after fork */
>  		strcpy(term, "TERM=");
> -		strncat(term, sp->se_type, sizeof(term) - 6);
> +		strlcat(term, sp->se_type, sizeof(term));
>  		env[0] = term;
>  		env[1] = 0;
>  	}
> @@ -1493,7 +1493,7 @@ start_getty(session_t *sp)
>  	if (sp->se_type) {
>  		/* Don't use malloc after fork */
>  		strcpy(term, "TERM=");
> -		strncat(term, sp->se_type, sizeof(term) - 6);
> +		strlcat(term, sp->se_type, sizeof(term));
>  		env[0] = term;
>  		env[1] = 0;
>  	} else