On 2024/10/20 19:13:06 +0300, Timo Myyrä <timo.myyra@bittivirhe.fi> wrote:
> On Sun, Oct 20, 2024, at 11:53, Omar Polo wrote:
> > Hello,
> >
> > On 2024/10/19 09:07:44 +0300, Timo Myyrä <timo.myyra@bittivirhe.fi> wrote:
> >> Hi,
> >> 
> >> I'm trying to setup gotwebd hosting on 7.6 release.
> >> The gotwebd crashes on startup when I have the malloc options 'CF'
> >> enabled.
> >>
> >> [...]
> >> 
> >> If I remove the malloc flags gotwebd starts as expected.
> >
> > Can you please tell us more about your installation?  I tried to
> > replicate here but it still seems to work for me.  What version are you
> > using?
> >
> > [...]
>
> Seems it occasionally starts ok, I started and stopped the gotwebd for a while and about 3 out of 20 start ups it works and fails for others. This is from recently upgraded 7.6-release on amd64.

Here's the fun thing.  I just re-ran it *once* and hit the crash.

> Here's the backtrace:
> [...]

Thank you for the backtrace.

So, the issue is an out of bounds access in config_getfd().  The current
code is like this

	for (i = 0; i < nfds; i++) {
		if (i < PRIV_FDS__MAX ++ env->priv_fd[i] == -1) {
			/* elided */
		}

		j = i - PRIV_FDS__MAX;
		if (env->ack_fds[j] == -1) <- KABOOM
	}

for the first 13 iterations, we end up reading out of bounds.  I don't
even know why this worked so far.  (i have a suspect I broke this...
don't want to check though.)

While we could just stick a range check and fix it, I prefer this longer
version that splits the loop in two and simplifies the overall logic.
dumb code is easier to follow.

(and it's also a net negative! \o/)

oks?

diff /home/op/w/got
commit - e2308af98f7d01e81f6173b9c264b1c21190a24a
path + /home/op/w/got
blob - 9f05cca64a73b00c1cebb028d16f3bd436fc1b1d
file + gotwebd/config.c
--- gotwebd/config.c
+++ gotwebd/config.c
@@ -191,33 +191,28 @@ config_setfd(struct gotwebd *env)
 int
 config_getfd(struct gotwebd *env, struct imsg *imsg)
 {
-	int match = 0, i, j;
-	const int nfds = GOTWEB_PACK_NUM_TEMPFILES + PRIV_FDS__MAX;
+	int i;
 
 	if (imsg_get_len(imsg) != 0)
 		fatalx("%s: wrong size", __func__);
 
-	for (i = 0; i < nfds; i++) {
-		if (i < PRIV_FDS__MAX && env->priv_fd[i] == -1) {
+	for (i = 0; i < nitems(env->priv_fd); ++i) {
+		if (env->priv_fd[i] == -1) {
 			env->priv_fd[i] = imsg_get_fd(imsg);
 			log_debug("%s: assigning priv_fd %d",
 			    __func__, env->priv_fd[i]);
-			match = 1;
-			break;
+			return 0;
 		}
+	}
 
-		j = i - PRIV_FDS__MAX;
-		if (env->pack_fds[j] == -1) {
-			env->pack_fds[j] = imsg_get_fd(imsg);
+	for (i = 0; i < nitems(env->pack_fds); ++i) {
+		if (env->pack_fds[i] == -1) {
+			env->pack_fds[i] = imsg_get_fd(imsg);
 			log_debug("%s: assigning pack_fd %d",
-			    __func__, env->pack_fds[j]);
-			match = 1;
-			break;
+			    __func__, env->pack_fds[i]);
+			return 0;
 		}
 	}
 
-	if (match)
-		return 0;
-	else
-		return 1;
+	return 1;
 }