Index | Thread | Search

From:
"Jonathan Armani" <jonathan@armani.tech>
Subject:
Re: sndiod: simplify unix socket binding code
To:
"Alexandre Ratchov" <alex@caoua.org>, tech@openbsd.org
Date:
Sat, 14 Mar 2026 15:48:15 +0100

Download raw body.

Thread
ok

On Fri, Mar 13, 2026, at 12:35, Alexandre Ratchov wrote:
> - Use sizeof(sockaddr_un->sun_path) instead of the complicated
>   calculation of the max socket path length.
>
> - Use the socket's address family to determine if this is an Unix
>   socket: no need to store the path, allocate memory for it, then
>   remember to free it.
>
> OK?
>
> Index: lib/libsndio/amsg.h
> ===================================================================
> RCS file: /cvs/src/lib/libsndio/amsg.h,v
> diff -u -p -u -p -r1.17 amsg.h
> --- lib/libsndio/amsg.h	22 Jan 2026 09:24:26 -0000	1.17
> +++ lib/libsndio/amsg.h	13 Mar 2026 11:31:29 -0000
> @@ -20,22 +20,10 @@
>  #include <stdint.h>
> 
>  /*
> - * unix-domain socket name is:
> - *
> - * DIR [ '-' UID ] '/' FILE UNIT
> - *
> - * example: "/tmp/sndio-1000/sock0"
> - *
> + * unix-domain socket (example: "/tmp/sndio-1000/sock0")
>   */
>  #define SOCKPATH_DIR	"/tmp/sndio"
>  #define SOCKPATH_FILE	"sock"
> -#define SOCKPATH_MAX	(1 +		\
> -	sizeof(SOCKPATH_DIR) - 1 +	\
> -	sizeof(char) +			\
> -	sizeof(int) * 3 +		\
> -	sizeof(char) +			\
> -	sizeof(SOCKPATH_FILE) - 1 +	\
> -	sizeof(int) * 3)
> 
>  /*
>   * server TCP base port number
> Index: usr.bin/sndiod/listen.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/sndiod/listen.c,v
> diff -u -p -u -p -r1.16 listen.c
> --- usr.bin/sndiod/listen.c	15 Sep 2025 08:39:22 -0000	1.16
> +++ usr.bin/sndiod/listen.c	13 Mar 2026 11:31:29 -0000
> @@ -67,40 +67,81 @@ listen_close(struct listen *f)
>  	}
>  	*pf = f->next;
> 
> -	if (f->path != NULL) {
> -		xfree(f->path);
> -	}
>  	file_del(f->file);
>  	close(f->fd);
>  	xfree(f);
>  }
> 
>  int
> -listen_new_un(char *path)
> +listen_new_un(unsigned int unit)
>  {
> -	int sock, oldumask;
> +	int len, sock, oldumask;
>  	struct sockaddr_un sockname;
>  	struct listen *f;
> +	struct stat sb;
> +	uid_t uid;
> +	mode_t mask, omask;
> +	char dir[sizeof(sockname.sun_path)];
> +
> +	uid = geteuid();
> +	if (uid == 0) {
> +		mask = 022;
> +		len = snprintf(sockname.sun_path, sizeof(sockname.sun_path),
> +		    SOCKPATH_DIR "/" SOCKPATH_FILE "%u", unit);
> +	} else {
> +		mask = 077;
> +		len = snprintf(sockname.sun_path, sizeof(sockname.sun_path),
> +		    SOCKPATH_DIR "-%u/" SOCKPATH_FILE "%u", uid, unit);
> +	}
> +	if (len >= sizeof(sockname.sun_path)) {
> +		logx(0, "unix socket name too long");
> +		return 0;
> +	}
> +
> +	while (sockname.sun_path[len] != '/')
> +		len--;
> +	memcpy(dir, sockname.sun_path, len);
> +	dir[len] = 0;
> +
> +	omask = umask(mask);
> +	if (mkdir(dir, 0777) == -1) {
> +		if (errno != EEXIST) {
> +			logx(0, "mkdir(\"%s\")", dir);
> +			return 0;
> +		}
> +	}
> +	umask(omask);
> +	if (stat(dir, &sb) == -1) {
> +		logx(0, "stat(\"%s\")", dir);
> +		return 0;
> +	}
> +	if (!S_ISDIR(sb.st_mode)) {
> +		logx(0, "%s is not a directory", dir);
> +		return 0;
> +	}
> +	if (sb.st_uid != uid || (sb.st_mode & mask) != 0) {
> +		logx(0, "%s has wrong permissions", dir);
> +		return 0;
> +	}
> 
>  	sock = socket(AF_UNIX, SOCK_STREAM, 0);
>  	if (sock == -1) {
> -		logx(0, "%s: failed to create socket", path);
> +		logx(0, "%s: failed to create socket", sockname.sun_path);
>  		return 0;
>  	}
> -	if (unlink(path) == -1 && errno != ENOENT) {
> -		logx(0, "%s: failed to unlink socket", path);
> +	if (unlink(sockname.sun_path) == -1 && errno != ENOENT) {
> +		logx(0, "%s: failed to unlink socket", sockname.sun_path);
>  		goto bad_close;
>  	}
>  	sockname.sun_family = AF_UNIX;
> -	strlcpy(sockname.sun_path, path, sizeof(sockname.sun_path));
>  	oldumask = umask(0111);
>  	if (bind(sock, (struct sockaddr *)&sockname,
>  		sizeof(struct sockaddr_un)) == -1) {
> -		logx(0, "%s: failed to bind socket", path);
> +		logx(0, "%s: failed to bind socket", sockname.sun_path);
>  		goto bad_close;
>  	}
>  	if (listen(sock, 1) == -1) {
> -		logx(0, "%s: failed to listen", path);
> +		logx(0, "%s: failed to listen", sockname.sun_path);
>  		goto bad_close;
>  	}
>  	umask(oldumask);
> @@ -108,7 +149,6 @@ listen_new_un(char *path)
>  	f->file = file_new(&listen_fileops, f, "unix", 1);
>  	if (f->file == NULL)
>  		goto bad_close;
> -	f->path = xstrdup(path);
>  	f->fd = sock;
>  	f->next = listen_list;
>  	listen_list = f;
> @@ -172,7 +212,6 @@ listen_new_tcp(char *addr, unsigned int 
>  			close(s);
>  			continue;
>  		}
> -		f->path = NULL;
>  		f->fd = s;
>  		f->next = listen_list;
>  		listen_list = f;
> @@ -231,7 +270,7 @@ listen_in(void *arg)
>  		logx(0, "%s: failed to set non-blocking mode", f->file->name);
>  		goto bad_close;
>  	}
> -	if (f->path == NULL) {
> +	if (caddr.sa_family == AF_INET || caddr.sa_family == AF_INET6) {
>  		opt = 1;
>  		if (setsockopt(sock, IPPROTO_TCP, TCP_NODELAY,
>  		    &opt, sizeof(int)) == -1) {
> Index: usr.bin/sndiod/listen.h
> ===================================================================
> RCS file: /cvs/src/usr.bin/sndiod/listen.h,v
> diff -u -p -u -p -r1.3 listen.h
> --- usr.bin/sndiod/listen.h	8 Jan 2016 13:14:11 -0000	1.3
> +++ usr.bin/sndiod/listen.h	13 Mar 2026 11:31:29 -0000
> @@ -22,14 +22,13 @@ struct file;
>  struct listen {
>  	struct listen *next;
>  	struct file *file;
> -	char *path;
>  	int fd;
>  	int slowaccept;
>  };
> 
>  extern struct listen *listen_list;
> 
> -int listen_new_un(char *);
> +int listen_new_un(unsigned int);
>  int listen_new_tcp(char *, unsigned int);
>  int listen_init(struct listen *);
>  void listen_close(struct listen *);
> Index: usr.bin/sndiod/sndiod.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/sndiod/sndiod.c,v
> diff -u -p -u -p -r1.52 sndiod.c
> --- usr.bin/sndiod/sndiod.c	26 Nov 2025 08:40:16 -0000	1.52
> +++ usr.bin/sndiod/sndiod.c	13 Mar 2026 11:31:29 -0000
> @@ -364,35 +364,6 @@ unsetsig(void)
>  		err(1, "unsetsig(int): sigaction failed");
>  }
> 
> -void
> -getbasepath(char *base)
> -{
> -	uid_t uid;
> -	struct stat sb;
> -	mode_t mask, omask;
> -
> -	uid = geteuid();
> -	if (uid == 0) {
> -		mask = 022;
> -		snprintf(base, SOCKPATH_MAX, SOCKPATH_DIR);
> -	} else {
> -		mask = 077;
> -		snprintf(base, SOCKPATH_MAX, SOCKPATH_DIR "-%u", uid);
> -	}
> -	omask = umask(mask);
> -	if (mkdir(base, 0777) == -1) {
> -		if (errno != EEXIST)
> -			err(1, "mkdir(\"%s\")", base);
> -	}
> -	umask(omask);
> -	if (stat(base, &sb) == -1)
> -		err(1, "stat(\"%s\")", base);
> -	if (!S_ISDIR(sb.st_mode))
> -		errx(1, "%s is not a directory", base);
> -	if (sb.st_uid != uid || (sb.st_mode & mask) != 0)
> -		errx(1, "%s has wrong permissions", base);
> -}
> -
>  struct dev *
>  mkdev(char *path, struct aparams *par,
>      int mode, int bufsz, int round, int rate, int hold, int autovol)
> @@ -536,7 +507,6 @@ main(int argc, char **argv)
>  {
>  	int c, i, background, unit;
>  	int pmin, pmax, rmin, rmax;
> -	char base[SOCKPATH_MAX], path[SOCKPATH_MAX];
>  	unsigned int mode, dup, mmc, vol;
>  	unsigned int hold, autovol, bufsz, round, rate;
>  	const char *str;
> @@ -750,9 +720,7 @@ main(int argc, char **argv)
>  			errx(1, "unknown user %s", SNDIO_USER);
>  	} else
>  		pw = NULL;
> -	getbasepath(base);
> -	snprintf(path, SOCKPATH_MAX, "%s/" SOCKPATH_FILE "%u", base, unit);
> -	if (!listen_new_un(path))
> +	if (!listen_new_un(unit))
>  		return 1;
>  	for (ta = tcpaddr_list; ta != NULL; ta = ta->next) {
>  		if (!listen_new_tcp(ta->host, AUCAT_PORT + unit))