[PATCH] staging: speakup: more fixes for init-failure handling.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



We still leaked many resources when Speakup failed to initialize.
Examples of leaked resources include:
/dev/synth, keyboard or VT notifiers, and heap-allocated st_spk_t
structs.

* I reworked the initialization and exit code.  Both speakup_init and
speakup_exit now call on a common helper function: resource_cleanup.
resource_cleanup is responsible for releasing resources in the proper
order.  It doesn't try to release unacquired resources, so it is safe
for both failed initialization and successful termination.

* devsynth.c gains a new accessor function, so that we can know whether
/dev/synth is registered.
* I also changed initialization order slightly, so that initializations
which can *never* fail come before those which may fail.

* We now use PTR_ERR to detect kthread_create failure
(thank you Dan Carpenter).

The loop which frees members of the speakup_console array now iterates
over the whole array, not stopping at the first NULL value.  Fixes
a possible memory leak.  Safe because kfree(NULL) is a no-op.

Signed-off-by: Christopher Brannon <chris@xxxxxxxxxxxxxxxx>
---
 drivers/staging/speakup/devsynth.c |    5 +
 drivers/staging/speakup/main.c     |  152 +++++++++++++++++++++++++-----------
 drivers/staging/speakup/speakup.h  |    1 +
 3 files changed, 111 insertions(+), 47 deletions(-)

diff --git a/drivers/staging/speakup/devsynth.c b/drivers/staging/speakup/devsynth.c
index 39dc586..ea1cf0f 100644
--- a/drivers/staging/speakup/devsynth.c
+++ b/drivers/staging/speakup/devsynth.c
@@ -92,3 +92,8 @@ void speakup_unregister_devsynth(void)
 	misc_deregister(&synth_device);
 	misc_registered = 0;
 }
+
+int speakup_devsynth_is_registered(void)
+{
+	return misc_registered;
+}
diff --git a/drivers/staging/speakup/main.c b/drivers/staging/speakup/main.c
index 3cd0039..b70dddc 100644
--- a/drivers/staging/speakup/main.c
+++ b/drivers/staging/speakup/main.c
@@ -1283,7 +1283,7 @@ static int edit_bits(struct vc_data *vc, u_char type, u_char ch, u_short key)
 }
 
 /* Allocation concurrency is protected by the console semaphore */
-void speakup_allocate(struct vc_data *vc)
+int speakup_allocate(struct vc_data *vc)
 {
 	int vc_num;
 
@@ -1292,10 +1292,12 @@ void speakup_allocate(struct vc_data *vc)
 		speakup_console[vc_num] = kzalloc(sizeof(*speakup_console[0]),
 						  GFP_ATOMIC);
 		if (speakup_console[vc_num] == NULL)
-			return;
+			return -ENOMEM;
 		speakup_date(vc);
 	} else if (!spk_parked)
 		speakup_date(vc);
+
+	return 0;
 }
 
 void speakup_deallocate(struct vc_data *vc)
@@ -2212,23 +2214,49 @@ static int vt_notifier_call(struct notifier_block *nb,
 	return NOTIFY_OK;
 }
 
-/* called by: module_exit() */
-static void __exit speakup_exit(void)
+static bool speakup_task_started;
+static bool keyboard_notifier_registered;
+static bool vt_notifier_registered;
+static bool kobjects_initialized;
+static bool virtual_keyboard_registered;
+
+/*
+ * Safely clean up all resources that have been allocated by speakup.
+ * This function is used both in speakup_init and speakup_exit.
+ */
+static void resource_cleanup(void)
 {
 	int i;
 
-	free_user_msgs();
-	unregister_keyboard_notifier(&keyboard_notifier_block);
-	unregister_vt_notifier(&vt_notifier_block);
-	speakup_unregister_devsynth();
-	del_timer(&cursor_timer);
+	if (speakup_task_started) {
+		del_timer(&cursor_timer);
+		kthread_stop(speakup_task);
+		speakup_task = NULL;
+	}
+
+	if (vt_notifier_registered)
+		unregister_vt_notifier(&vt_notifier_block);
+	if (keyboard_notifier_registered)
+		unregister_keyboard_notifier(&keyboard_notifier_block);
+	if (speakup_devsynth_is_registered())
+		speakup_unregister_devsynth();
 
-	kthread_stop(speakup_task);
-	speakup_task = NULL;
 	mutex_lock(&spk_mutex);
 	synth_release();
 	mutex_unlock(&spk_mutex);
 
+	if (kobjects_initialized)
+		speakup_kobj_exit();
+
+	/*
+	 * Free all allocated st_spk_t structures.
+	 */
+	for (i = 0; i < MAX_NR_CONSOLES; i++)
+		kfree(speakup_console[i]);
+
+	if (virtual_keyboard_registered)
+		speakup_remove_virtual_keyboard();
+
 	for (i = 0; i < MAXVARS; i++)
 		speakup_unregister_var(i);
 
@@ -2236,42 +2264,29 @@ static void __exit speakup_exit(void)
 		if (characters[i] != default_chars[i])
 			kfree(characters[i]);
 	}
-	for (i = 0; speakup_console[i]; i++)
-		kfree(speakup_console[i]);
-	speakup_kobj_exit();
-	speakup_remove_virtual_keyboard();
+
+	free_user_msgs();
+}
+
+/* called by: module_exit() */
+static void __exit speakup_exit(void)
+{
+	resource_cleanup();
 }
 
 /* call by: module_init() */
 static int __init speakup_init(void)
 {
 	int i;
-	int err;
+	long err = 0;
 	struct st_spk_t *first_console;
 	struct vc_data *vc = vc_cons[fg_console].d;
 	struct var_t *var;
 
-	err = speakup_add_virtual_keyboard();
-	if (err)
-		goto out;
-
+/* These first few initializations cannot fail. */
 	initialize_msgs();	/* Initialize arrays for i18n. */
-	first_console = kzalloc(sizeof(*first_console), GFP_KERNEL);
-	if (!first_console) {
-		err = -ENOMEM;
-		goto err_cons;
-	}
-	err = speakup_kobj_init();
-	if (err)
-		goto err_kobject;
-
 	reset_default_chars();
 	reset_default_chartab();
-
-	speakup_console[vc->vc_num] = first_console;
-	speakup_date(vc);
-	pr_info("speakup %s: initialized\n", SPEAKUP_VERSION);
-
 	strlwr(synth_name);
 	spk_vars[0].u.n.high = vc->vc_cols;
 	for (var = spk_vars; var->var_id != MAXVARS; var++)
@@ -2286,31 +2301,74 @@ static int __init speakup_init(void)
 	if (quiet_boot)
 		spk_shut_up |= 0x01;
 
+	/* From here on out, initializations can fail. */
+	err = speakup_add_virtual_keyboard();
+	if (err)
+		goto error_exit;
+	virtual_keyboard_registered = true;
+
+	first_console = kzalloc(sizeof(*first_console), GFP_KERNEL);
+	if (!first_console) {
+		err = -ENOMEM;
+		goto error_exit;
+	}
+
+	speakup_console[vc->vc_num] = first_console;
+	speakup_date(vc);
+
+	/*
+	 * We don't need to worry about first_console from here onward.
+	 * The for-loop in resource_cleanup will free it, if something fails.
+	 */
+
 	for (i = 0; i < MAX_NR_CONSOLES; i++)
-		if (vc_cons[i].d)
-			speakup_allocate(vc_cons[i].d);
+		if (vc_cons[i].d) {
+			err = speakup_allocate(vc_cons[i].d);
+			if (err)
+				goto error_exit;
+		}
+
+	err = speakup_kobj_init();
+	if (err)
+		goto error_exit;
+	kobjects_initialized = true;
 
-	pr_warn("synth name on entry is: %s\n", synth_name);
 	synth_init(synth_name);
+
 	speakup_register_devsynth();
+	/*
+	 * register_devsynth might fail, but this error is not fatal.
+	 * /dev/synth is an extra feature; the rest of Speakup
+	 * will work fine without it.
+	 */
 
-	register_keyboard_notifier(&keyboard_notifier_block);
-	register_vt_notifier(&vt_notifier_block);
+	err = register_keyboard_notifier(&keyboard_notifier_block);
+	if (err)
+		goto error_exit;
+	keyboard_notifier_registered = true;
+	err = register_vt_notifier(&vt_notifier_block);
+	if (err)
+		goto error_exit;
+	vt_notifier_registered = true;
 
 	speakup_task = kthread_create(speakup_thread, NULL, "speakup");
-	set_user_nice(speakup_task, 10);
+
 	if (IS_ERR(speakup_task)) {
-		err = -ENOMEM;
-		goto err_kobject;
+		err = PTR_ERR(speakup_task);
+		goto error_exit;
 	}
+
+	set_user_nice(speakup_task, 10);
 	wake_up_process(speakup_task);
+	speakup_task_started = true;
+
+	pr_info("speakup %s: initialized\n", SPEAKUP_VERSION);
+	pr_warn("synth name on entry is: %s\n", synth_name);
 	goto out;
 
-err_kobject:
-speakup_kobj_exit();
-	kfree(first_console);
-err_cons:
-	speakup_remove_virtual_keyboard();
+error_exit:
+	resource_cleanup();
+
 out:
 	return err;
 }
diff --git a/drivers/staging/speakup/speakup.h b/drivers/staging/speakup/speakup.h
index 46edabe..5b0fb6e 100644
--- a/drivers/staging/speakup/speakup.h
+++ b/drivers/staging/speakup/speakup.h
@@ -87,6 +87,7 @@ extern int speakup_set_selection(struct tty_struct *tty);
 extern int speakup_paste_selection(struct tty_struct *tty);
 extern void speakup_register_devsynth(void);
 extern void speakup_unregister_devsynth(void);
+extern int speakup_devsynth_is_registered(void);
 extern void synth_write(const char *buf, size_t count);
 extern int synth_supports_indexing(void);
 
-- 
1.7.2.2

_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel


[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux