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 <[email protected]> --- 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 [email protected] http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
