From: Stefan Sperling <stsp@stsp.name>
Subject: Re: types {} block for gotwebd
To: Omar Polo <op@omarpolo.com>
Cc: gameoftrees@openbsd.org
Date: Mon, 17 Nov 2025 17:13:39 +0100

On Mon, Nov 17, 2025 at 03:50:33PM +0100, Omar Polo wrote:
> --- gotwebd/config.c
> +++ gotwebd/config.c
> @@ -44,6 +44,7 @@
>  
>  #include "gotwebd.h"
>  #include "log.h"
> +#include "media.h"
>  
>  int
>  config_init(struct gotwebd *env)
> @@ -60,6 +61,11 @@ config_init(struct gotwebd *env)
>  	TAILQ_INIT(&env->addresses);
>  	STAILQ_INIT(&env->access_rules);
>  
> +	env->mediatypes = calloc(1, sizeof(*env->mediatypes));
> +	if (env->mediatypes == NULL)
> +		return (-1);
> +	RB_INIT(env->mediatypes);
> +

The gotwebd tests can complain about memleaks with this if we
forgot to free it before exiting.

Any reason the RB tree root cannot be a struct instead of a pointer?
Then we could do: RB_INIT(&env.mediatypes); and avoid malloc/free.

>  	for (i = 0; i < PRIV_FDS__MAX; i++)
>  		env->priv_fd[i] = -1;
>  
> @@ -79,6 +85,20 @@ config_getcfg(struct gotwebd *env, struct imsg *imsg)
>  }
>  
>  int
> +config_getmediatype(struct gotwebd *env, struct imsg *imsg)
> +{
> +	struct media_type mt;
> +
> +	if (imsg_get_data(imsg, &mt, sizeof(mt)) == -1)
> +		fatal("%s: failed to receive media type", __func__);

We are reading arbitrary data here and blindly expect it to contain
strings. We should check whether types added are printable-ASCII-only,
and whether the strings are NUL-terminated.

This might sound very paranoid since we trust the parent to send a valid
config. But in context of gotsysd the media types list may well be arbitrary
user input which was read from gotsys.conf. It would be best if gotwebd
rejected bad media types at config parse-time, since gotsys.conf will
likely borrow related code from gotwebd. Then we could fail early on bad
input while parsing gotsys.conf. Because media_add is also called from
parse.y, adding such checks to media_add() would be best, I think.

> +	if (media_add(env->mediatypes, &mt) == NULL)
> +		fatal("%s: failed to insert media type", __func__);
> +
> +	return (0);
> +}
> +
> +int
>  config_getserver(struct gotwebd *env, struct imsg *imsg)
>  {
>  	struct server *srv;

> --- gotwebd/gotwebd.c
> +++ gotwebd/gotwebd.c

> @@ -586,7 +588,8 @@ main(int argc, char **argv)
>  	env = calloc(1, sizeof(*env));
>  	if (env == NULL)
>  		fatal("%s: calloc", __func__);
> -	config_init(env);
> +	if (config_init(env) == -1)
> +		fatal("%s: failed to initialize configuration", __func__);

Are we sure config_init() always sets errno? Should this use fatalx instead?

>  	while ((ch = getopt(argc, argv, "A:D:dG:f:F:L:nS:vW:")) != -1) {
>  		switch (ch) {

> --- /dev/null
> +++ gotwebd/media.c

> +struct media_type *
> +media_add(struct mediatypes *types, struct media_type *media)
> +{
> +	struct media_type	*entry;
> +
> +	if ((entry = RB_FIND(mediatypes, types, media)) != NULL) {
> +		log_debug("%s: entry overwritten for \"%s\"", __func__,
> +		    media->media_name);
> +		media_delete(types, entry);
> +	}
> +
> +	if ((entry = malloc(sizeof(*media))) == NULL)
> +		return (NULL);
> +
> +	memcpy(entry, media, sizeof(*entry));

This is where I would like to have proper validation instead of memcpy.

> +	RB_INSERT(mediatypes, types, entry);
> +
> +	return (entry);
> +}

The rest looks good to me.