From 1f1be520070165ce402b46d17a44a2941c0dda2f Mon Sep 17 00:00:00 2001
From: Augustin Cavalier <waddlesplash@gmail.com>
Date: Wed, 14 Aug 2024 15:45:50 -0400
Subject: [PATCH] runtime_loader: Properly unload libraries whose dependencies
 fail to load.

If some (but not all) of the dependencies failed to load, they
will be in an inconsistent state (some NEEDED unset/uninitialized, etc.)
In order to neither leak this data nor have it cause problems later,
we should unload actually call unload_library() and thus put_image()
to have the unused/uninitialized dependencies be propery unloaded.

To make this work correctly, adjust unload_library to only call
the exit hooks if the initialization hooks were also called.

Fixes a crash in Firefox startup when some dependencies aren't installed.
---
 src/system/runtime_loader/elf.cpp    | 47 +++++++++++++---------------
 src/system/runtime_loader/images.cpp | 10 ++++--
 2 files changed, 28 insertions(+), 29 deletions(-)

diff --git a/src/system/runtime_loader/elf.cpp b/src/system/runtime_loader/elf.cpp
index 55859b9f20..d2d4ec5a0d 100644
--- a/src/system/runtime_loader/elf.cpp
+++ b/src/system/runtime_loader/elf.cpp
@@ -716,8 +716,7 @@ load_library(char const *path, uint32 flags, bool addOn, void* caller,
 err:
 	KTRACE("rld: load_library(\"%s\") failed: %s", path, strerror(status));
 
-	dequeue_loaded_image(image);
-	delete_image(image);
+	unload_library(image, -1, addOn);
 	return status;
 }
 
@@ -744,28 +743,24 @@ unload_library(void* handle, image_id imageID, bool addOn)
 
 	// we only check images that have been already initialized
 
-	status_t status = B_BAD_IMAGE_ID;
-
 	if (handle != NULL) {
 		image = (image_t*)handle;
 		put_image(image);
-		status = B_OK;
 	} else {
 		image = find_loaded_image_by_id(imageID, true);
-		if (image != NULL) {
-			// unload image
-			if (type == image->type) {
-				put_image(image);
-				status = B_OK;
-			} else
-				status = B_BAD_VALUE;
-		}
+		if (image == NULL)
+			return B_BAD_IMAGE_ID;
+
+		// unload image
+		if (type != image->type)
+			return B_BAD_VALUE;
+		put_image(image);
 	}
 
-	if (status == B_OK) {
-		while ((image = get_disposable_images().head) != NULL) {
-			dequeue_disposable_image(image);
+	while ((image = get_disposable_images().head) != NULL) {
+		dequeue_disposable_image(image);
 
+		if ((image->flags & RFLAG_INITIALIZED) != 0) {
 			// Call the exit hooks that live in this image.
 			// Note: With the Itanium ABI this shouldn't really be done this
 			// way anymore, since global destructors are registered via
@@ -778,7 +773,7 @@ unload_library(void* handle, image_id imageID, bool addOn)
 			// probably more expensive than calling
 			// call_atexit_hooks_for_range() only here, which happens only when
 			// libraries are unloaded dynamically.
-			if (gRuntimeLoader.call_atexit_hooks_for_range) {
+			if (gRuntimeLoader.call_atexit_hooks_for_range != NULL) {
 				gRuntimeLoader.call_atexit_hooks_for_range(
 					image->regions[0].vmstart, image->regions[0].vmsize);
 			}
@@ -786,18 +781,18 @@ unload_library(void* handle, image_id imageID, bool addOn)
 			image_event(image, IMAGE_EVENT_UNINITIALIZING);
 
 			call_term_functions(image);
-
-			TLSBlockTemplates::Get().Unregister(image->dso_tls_id);
-
-			unmap_image(image);
-
-			image_event(image, IMAGE_EVENT_UNLOADING);
-
-			delete_image(image);
 		}
+
+		TLSBlockTemplates::Get().Unregister(image->dso_tls_id);
+
+		unmap_image(image);
+
+		image_event(image, IMAGE_EVENT_UNLOADING);
+
+		delete_image(image);
 	}
 
-	return status;
+	return B_OK;
 }
 
 
diff --git a/src/system/runtime_loader/images.cpp b/src/system/runtime_loader/images.cpp
index 6e0df04bd5..574132389d 100644
--- a/src/system/runtime_loader/images.cpp
+++ b/src/system/runtime_loader/images.cpp
@@ -277,14 +277,18 @@ put_image(image_t* image)
 	// and remove all dependencies
 
 	if (atomic_add(&image->ref_count, -1) == 1) {
-		size_t i;
-
 		dequeue_image(&sLoadedImages, image);
 		enqueue_image(&sDisposableImages, image);
 		sLoadedImageCount--;
 
-		for (i = 0; i < image->num_needed; i++)
+		// If the image wasn't fully loaded, its NEEDED may be incomplete.
+		if (image->needed == NULL)
+			image->num_needed = 0;
+		for (size_t i = 0; i < image->num_needed; i++) {
+			if (image->needed[i] == NULL)
+				continue;
 			put_image(image->needed[i]);
+		}
 	}
 }