On Sat, Nov 25, 2023 at 02:22:49PM +0100, Stefan Sperling wrote:
> gotwebd on got.g.o crashed and left me with a core file.
> 
> I don't have time to look into this right now. I'm preserving the
> backtrace on this list.  Does anyone else have ideas?
> 
> Program terminated with signal SIGSEGV, Segmentation fault.
> #0  0x000002d5b858e16c in match_packed_object (unique_id=0x766592357050, repo=0x2d7c419e450, id_str_prefix=0x2d7ffe83fc0 "a3a0e34e98c4f91a4b8fe53e24f82833a72e40a1", 
>     obj_type=1) at /home/stsp/src/got/gotwebd/../lib/repository.c:1780
> 1780                    const char *path_packidx = pe->path;
> (gdb) p pe
> $1 = (struct got_pathlist_entry *) 0xdfdfdfdfdfdfdfdf
> (gdb) bt
> #0  0x000002d5b858e16c in match_packed_object (unique_id=0x766592357050, repo=0x2d7c419e450, id_str_prefix=0x2d7ffe83fc0 "a3a0e34e98c4f91a4b8fe53e24f82833a72e40a1", 
>     obj_type=1) at /home/stsp/src/got/gotwebd/../lib/repository.c:1780
> #1  0x000002d5b858de2f in got_repo_match_object_id_prefix (id=0x766592357050, id_str_prefix=0x2d7ffe83fc0 "a3a0e34e98c4f91a4b8fe53e24f82833a72e40a1", obj_type=1, 
>     repo=0x2d7c419e450) at /home/stsp/src/got/gotwebd/../lib/repository.c:1945
> #2  0x000002d5b8558e78 in got_get_repo_commits (c=0x2d807b54000, limit=1) at /home/stsp/src/got/gotwebd/got_operations.c:354
> #3  0x000002d5b8554e2a in gotweb_process_request (c=0x2d807b54000) at /home/stsp/src/got/gotwebd/gotweb.c:220
> #4  0x000002d5b8553ea3 in fcgi_parse_params (buf=0x2d807b54164 "\001\005", n=0, c=0x2d807b54000, id=1) at /home/stsp/src/got/gotwebd/fcgi.c:191
> #5  0x000002d5b8553af6 in fcgi_parse_record (buf=0x2d807b5415c "\001\004", n=16, c=0x2d807b54000) at /home/stsp/src/got/gotwebd/fcgi.c:137
> #6  0x000002d5b85537fb in fcgi_request (fd=87, events=2, arg=0x2d807b54000) at /home/stsp/src/got/gotwebd/fcgi.c:93
> #7  0x000002d7efd1b32f in event_process_active (base=0x2d8aa359800) at /usr/src/lib/libevent/event.c:333
> #8  event_base_loop (base=0x2d8aa359800, flags=<optimized out>) at /usr/src/lib/libevent/event.c:483
> #9  0x000002d5b854bf3b in sockets (env=0x2d8aa3807a0, fd=3) at /home/stsp/src/got/gotwebd/sockets.c:126
> #10 0x000002d5b854f1e3 in main (argc=0, argv=0x766592357878) at /home/stsp/src/got/gotwebd/gotwebd.c:355
> (gdb) 
> 
> 

It looks like the problem is a call to refresh_packidx_paths() within
functions called while looping over repo->pathidx_paths:

 got_object_get_type() -> got_object_open() -> got_object_open_packed() ->
 got_repo_search_packidx() -> refresh_packidx_paths()

The patch below should fix it. This is a quick fix just for this code path.
Other places looping over this list don't seem to be doing anything dangerous.
This could be improved upon later but I think it is good enough for now.

ok?

diff /home/stsp/src/got
commit - 2bde3e78a5bd6619af838df19eec530e23783c0b
path + /home/stsp/src/got
blob - f44259d099d9b70c547d238971a7cc147c4e209f
file + lib/got_lib_repository.h
--- lib/got_lib_repository.h
+++ lib/got_lib_repository.h
@@ -66,6 +66,7 @@ struct got_repository {
 	enum got_hash_algorithm algo;
 
 	struct got_pathlist_head packidx_paths;
+	int no_packidx_refresh;
 	struct timespec pack_path_mtime;
 
 	/* The pack index cache speeds up search for packed objects. */
blob - b3b8dfbf8803c64c39fe81209b84b6fa8fd4d79a
file + lib/repository.c
--- lib/repository.c
+++ lib/repository.c
@@ -713,6 +713,7 @@ got_repo_open(struct got_repository **repop, const cha
 
 	RB_INIT(&repo->packidx_bloom_filters);
 	TAILQ_INIT(&repo->packidx_paths);
+	repo->no_packidx_refresh = 0;
 
 	for (i = 0; i < nitems(repo->privsep_children); i++) {
 		memset(&repo->privsep_children[i], 0,
@@ -1299,6 +1300,9 @@ refresh_packidx_paths(struct got_repository *repo)
 	char *objects_pack_dir = NULL;
 	struct stat sb;
 
+	if (repo->no_packidx_refresh)
+		return NULL;
+
 	objects_pack_dir = got_repo_get_path_objects_pack(repo);
 	if (objects_pack_dir == NULL)
 		return got_error_from_errno("got_repo_get_path_objects_pack");
@@ -1776,6 +1780,14 @@ match_packed_object(struct got_object_id **unique_id,
 	if (err)
 		return err;
 
+	/*
+	 * XXX Disable pack index refresh while we are looping and opening
+	 * packed objects. Otherwise the packidx list we are using could be
+	 * freed via got_object_get_type() -> ... -> got_object_open_packed()
+	 * -> got_repo_search_packidx() -> refresh_packidx_paths().
+	 */
+	repo->no_packidx_refresh = 1;
+
 	TAILQ_FOREACH(pe, &repo->packidx_paths, entry) {
 		const char *path_packidx = pe->path;
 		struct got_packidx *packidx;
@@ -1822,6 +1834,7 @@ match_packed_object(struct got_object_id **unique_id,
 		}
 	}
 done:
+	repo->no_packidx_refresh = 0;
 	got_object_id_queue_free(&matched_ids);
 	if (err) {
 		free(*unique_id);