[alsa-devel] [PATCH 0/1] Fixed issues/defects reported by Coverity tool.
From: Sudarshan sudarshan.bisht@nokia.com
Coverity Static Analysis helps developers find hard-to-spot, yet potentially crash-causing defects early in the development phase, reducing the cost,time, and risk of software errors. This patch has fix for situations where variable can be NULL but not been checked beforehand.
Sudarshan (1): alsa-lib: fixed coverity reported issues under "FORWARD_NULL" checker.
modules/mixer/simple/sbasedl.c | 2 +- src/conf.c | 8 +++++--- src/hwdep/hwdep.c | 2 +- src/pcm/pcm_hooks.c | 5 +++-- src/pcm/pcm_simple.c | 2 +- src/rawmidi/rawmidi.c | 2 +- src/rawmidi/rawmidi_virt.c | 8 +++++--- 7 files changed, 17 insertions(+), 12 deletions(-)
From: Sudarshan sudarshan.bisht@nokia.com
--- modules/mixer/simple/sbasedl.c | 2 +- src/conf.c | 8 +++++--- src/hwdep/hwdep.c | 2 +- src/pcm/pcm_hooks.c | 5 +++-- src/pcm/pcm_simple.c | 2 +- src/rawmidi/rawmidi.c | 2 +- src/rawmidi/rawmidi_virt.c | 8 +++++--- 7 files changed, 17 insertions(+), 12 deletions(-)
diff --git a/modules/mixer/simple/sbasedl.c b/modules/mixer/simple/sbasedl.c index 0137586..494802f 100644 --- a/modules/mixer/simple/sbasedl.c +++ b/modules/mixer/simple/sbasedl.c @@ -99,7 +99,7 @@ int mixer_simple_basic_dlopen(snd_mixer_class_t *class, __error: if (initflag) free(priv); - if (h == NULL) + if (h) snd_dlclose(h); free(xlib); return -ENXIO; diff --git a/src/conf.c b/src/conf.c index 8939d62..9b69e12 100644 --- a/src/conf.c +++ b/src/conf.c @@ -3321,9 +3321,11 @@ static int snd_config_hooks_call(snd_config_t *root, snd_config_t *config, snd_c snd_config_delete(func_conf); if (err >= 0) { snd_config_t *nroot; - err = func(root, config, &nroot, private_data); - if (err < 0) - SNDERR("function %s returned error: %s", func_name, snd_strerror(err)); + if (func) { + err = func(root, config, &nroot, private_data); + if (err < 0) + SNDERR("function %s returned error: %s", func_name, snd_strerror(err)); + } snd_dlclose(h); if (err >= 0 && nroot) err = snd_config_substitute(root, nroot); diff --git a/src/hwdep/hwdep.c b/src/hwdep/hwdep.c index b882b35..690d9f6 100644 --- a/src/hwdep/hwdep.c +++ b/src/hwdep/hwdep.c @@ -130,7 +130,7 @@ static int snd_hwdep_open_conf(snd_hwdep_t **hwdep, _err: if (type_conf) snd_config_delete(type_conf); - if (err >= 0) { + if (err >= 0 && open_func) { err = open_func(hwdep, name, hwdep_root, hwdep_conf, mode); if (err >= 0) { (*hwdep)->dl_handle = h; diff --git a/src/pcm/pcm_hooks.c b/src/pcm/pcm_hooks.c index 3a99d55..a0ed0b9 100644 --- a/src/pcm/pcm_hooks.c +++ b/src/pcm/pcm_hooks.c @@ -445,14 +445,15 @@ static int snd_pcm_hook_add_conf(snd_pcm_t *pcm, snd_config_t *root, snd_config_ else err = install_func(pcm, args); snd_config_delete(args); - } else + } else if (install_func) err = install_func(pcm, args);
if (err >= 0) err = hook_add_dlobj(pcm, h);
if (err < 0) { - snd_dlclose(h); + if (h) + snd_dlclose(h); return err; } return 0; diff --git a/src/pcm/pcm_simple.c b/src/pcm/pcm_simple.c index 975f699..f943ec0 100644 --- a/src/pcm/pcm_simple.c +++ b/src/pcm/pcm_simple.c @@ -89,7 +89,7 @@ static int set_hw_params(snd_pcm_t *pcm, return err; if (periods == 1) return -EINVAL; - if (*period_time == 0) { + if (period_time) { err = INTERNAL(snd_pcm_hw_params_get_period_time)(hw_params, period_time, NULL); if (err < 0) return err; diff --git a/src/rawmidi/rawmidi.c b/src/rawmidi/rawmidi.c index b28488a..a8d7231 100644 --- a/src/rawmidi/rawmidi.c +++ b/src/rawmidi/rawmidi.c @@ -253,7 +253,7 @@ static int snd_rawmidi_open_conf(snd_rawmidi_t **inputp, snd_rawmidi_t **outputp _err: if (type_conf) snd_config_delete(type_conf); - if (err >= 0) + if (err >= 0 && open_func) err = open_func(inputp, outputp, name, rawmidi_root, rawmidi_conf, mode); if (err < 0) return err; diff --git a/src/rawmidi/rawmidi_virt.c b/src/rawmidi/rawmidi_virt.c index 52b8984..e5b17e4 100644 --- a/src/rawmidi/rawmidi_virt.c +++ b/src/rawmidi/rawmidi_virt.c @@ -383,9 +383,11 @@ int snd_rawmidi_virtual_open(snd_rawmidi_t **inputp, snd_rawmidi_t **outputp, _err: if (seq_handle) snd_seq_close(seq_handle); - if (virt->midi_event) - snd_midi_event_free(virt->midi_event); - free(virt); + if (virt) { + if (virt->midi_event) + snd_midi_event_free(virt->midi_event); + free(virt); + } if (inputp) free(*inputp); if (outputp)
sudarshan.bisht@nokia.com wrote:
--- a/modules/mixer/simple/sbasedl.c +++ b/modules/mixer/simple/sbasedl.c @@ -99,7 +99,7 @@ int mixer_simple_basic_dlopen(snd_mixer_class_t *class, __error: if (initflag) free(priv);
- if (h == NULL)
- if (h) snd_dlclose(h); free(xlib); return -ENXIO;
We already had this one.
--- a/src/conf.c +++ b/src/conf.c @@ -3321,9 +3321,11 @@ static int snd_config_hooks_call(snd_config_t *root, snd_config_t *config, snd_c snd_config_delete(func_conf); if (err >= 0) { snd_config_t *nroot;
err = func(root, config, &nroot, private_data);if (err < 0)SNDERR("function %s returned error: %s", func_name, snd_strerror(err));
if (func) {err = func(root, config, &nroot, private_data);if (err < 0)SNDERR("function %s returned error: %s", func_name, snd_strerror(err));}
The preceding "!func" and "err >= 0" checks already guarantee that func is valid.
--- a/src/hwdep/hwdep.c +++ b/src/hwdep/hwdep.c @@ -130,7 +130,7 @@ static int snd_hwdep_open_conf(snd_hwdep_t **hwdep, _err: if (type_conf) snd_config_delete(type_conf);
- if (err >= 0) {
- if (err >= 0 && open_func) { err = open_func(hwdep, name, hwdep_root, hwdep_conf, mode); if (err >= 0) { (*hwdep)->dl_handle = h;
Same here.
--- a/src/pcm/pcm_hooks.c +++ b/src/pcm/pcm_hooks.c @@ -445,14 +445,15 @@ static int snd_pcm_hook_add_conf(snd_pcm_t *pcm, snd_config_t *root, snd_config_ else err = install_func(pcm, args); snd_config_delete(args);
- } else
} else if (install_func) err = install_func(pcm, args);
if (err >= 0) err = hook_add_dlobj(pcm, h);
if (err < 0) {
snd_dlclose(h);
if (h)snd_dlclose(h);
Same here.
--- a/src/pcm/pcm_simple.c +++ b/src/pcm/pcm_simple.c @@ -89,7 +89,7 @@ static int set_hw_params(snd_pcm_t *pcm, return err; if (periods == 1) return -EINVAL;
if (*period_time == 0) {
if (period_time) { err = INTERNAL(snd_pcm_hw_params_get_period_time)(hw_params, period_time, NULL); if (err < 0) return err;
OK, I think, but this change deserves an explanation.
--- a/src/rawmidi/rawmidi.c +++ b/src/rawmidi/rawmidi.c @@ -253,7 +253,7 @@ static int snd_rawmidi_open_conf(snd_rawmidi_t **inputp, snd_rawmidi_t **outputp _err: if (type_conf) snd_config_delete(type_conf);
- if (err >= 0)
- if (err >= 0 && open_func) err = open_func(inputp, outputp, name, rawmidi_root, rawmidi_conf, mode); if (err < 0) return err;
Same as with the function pointers above.
--- a/src/rawmidi/rawmidi_virt.c +++ b/src/rawmidi/rawmidi_virt.c @@ -383,9 +383,11 @@ int snd_rawmidi_virtual_open(snd_rawmidi_t **inputp, snd_rawmidi_t **outputp, _err: if (seq_handle) snd_seq_close(seq_handle);
- if (virt->midi_event)
snd_midi_event_free(virt->midi_event);- free(virt);
- if (virt) {
if (virt->midi_event)snd_midi_event_free(virt->midi_event);free(virt);- } if (inputp) free(*inputp); if (outputp)
OK, but not needed for the free().
Regards, Clemens
On Thu, 2011-03-17 at 13:08 +0100, ext Clemens Ladisch wrote:
sudarshan.bisht@nokia.com wrote:
--- a/modules/mixer/simple/sbasedl.c +++ b/modules/mixer/simple/sbasedl.c @@ -99,7 +99,7 @@ int mixer_simple_basic_dlopen(snd_mixer_class_t *class, __error: if (initflag) free(priv);
- if (h == NULL)
- if (h) snd_dlclose(h); free(xlib); return -ENXIO;
We already had this one.
Ok.
--- a/src/conf.c +++ b/src/conf.c @@ -3321,9 +3321,11 @@ static int snd_config_hooks_call(snd_config_t *root, snd_config_t *config, snd_c snd_config_delete(func_conf); if (err >= 0) { snd_config_t *nroot;
err = func(root, config, &nroot, private_data);if (err < 0)SNDERR("function %s returned error: %s", func_name, snd_strerror(err));
if (func) {err = func(root, config, &nroot, private_data);if (err < 0)SNDERR("function %s returned error: %s", func_name, snd_strerror(err));}The preceding "!func" and "err >= 0" checks already guarantee that func is valid.
But in case of "goto _err" , "!func" and "err >= 0" are not going to be checked.
--- a/src/hwdep/hwdep.c +++ b/src/hwdep/hwdep.c @@ -130,7 +130,7 @@ static int snd_hwdep_open_conf(snd_hwdep_t **hwdep, _err: if (type_conf) snd_config_delete(type_conf);
- if (err >= 0) {
- if (err >= 0 && open_func) { err = open_func(hwdep, name, hwdep_root, hwdep_conf, mode); if (err >= 0) { (*hwdep)->dl_handle = h;
Same here.
Same explanation as above.
--- a/src/pcm/pcm_hooks.c +++ b/src/pcm/pcm_hooks.c @@ -445,14 +445,15 @@ static int snd_pcm_hook_add_conf(snd_pcm_t *pcm, snd_config_t *root, snd_config_ else err = install_func(pcm, args); snd_config_delete(args);
- } else
} else if (install_func) err = install_func(pcm, args);
if (err >= 0) err = hook_add_dlobj(pcm, h);
if (err < 0) {
snd_dlclose(h);
if (h)snd_dlclose(h);Same here.
h will be NULL when encounters any preceding "goto _err".
--- a/src/pcm/pcm_simple.c +++ b/src/pcm/pcm_simple.c @@ -89,7 +89,7 @@ static int set_hw_params(snd_pcm_t *pcm, return err; if (periods == 1) return -EINVAL;
if (*period_time == 0) {
if (period_time) { err = INTERNAL(snd_pcm_hw_params_get_period_time)(hw_params, period_time, NULL); if (err < 0) return err;OK, I think, but this change deserves an explanation.
If "period_time == NULL" ( in preceding if condition) is true then "if (*period_time == 0)" has no meaning, and if "*period_time == 0" ( in preceding if condition) is true then obviously period_time will be a valid pointer and satisfy the condition. Thats what original source codes demands.
--- a/src/rawmidi/rawmidi.c +++ b/src/rawmidi/rawmidi.c @@ -253,7 +253,7 @@ static int snd_rawmidi_open_conf(snd_rawmidi_t **inputp, snd_rawmidi_t **outputp _err: if (type_conf) snd_config_delete(type_conf);
- if (err >= 0)
- if (err >= 0 && open_func) err = open_func(inputp, outputp, name, rawmidi_root, rawmidi_conf, mode); if (err < 0) return err;
Same as with the function pointers above.
Same reason what I gave for function pointers.
--- a/src/rawmidi/rawmidi_virt.c +++ b/src/rawmidi/rawmidi_virt.c @@ -383,9 +383,11 @@ int snd_rawmidi_virtual_open(snd_rawmidi_t **inputp, snd_rawmidi_t **outputp, _err: if (seq_handle) snd_seq_close(seq_handle);
- if (virt->midi_event)
snd_midi_event_free(virt->midi_event);- free(virt);
- if (virt) {
if (virt->midi_event)snd_midi_event_free(virt->midi_event);free(virt);- } if (inputp) free(*inputp); if (outputp)
OK, but not needed for the free().
Sorry, but I did not get this.
Regards, Clemens
Br, Sudarshan Bisht.
Sudarshan Bisht wrote:
On Thu, 2011-03-17 at 13:08 +0100, ext Clemens Ladisch wrote:
sudarshan.bisht@nokia.com wrote:
--- a/src/conf.c +++ b/src/conf.c @@ -3321,9 +3321,11 @@ static int snd_config_hooks_call(snd_config_t *root, snd_config_t *config, snd_c snd_config_delete(func_conf); if (err >= 0) { snd_config_t *nroot;
err = func(root, config, &nroot, private_data);if (err < 0)SNDERR("function %s returned error: %s", func_name, snd_strerror(err));
if (func) {err = func(root, config, &nroot, private_data);if (err < 0)SNDERR("function %s returned error: %s", func_name, snd_strerror(err));}The preceding "!func" and "err >= 0" checks already guarantee that func is valid.
But in case of "goto _err" , "!func" and "err >= 0" are not going to be checked.
"err >= 0" is checked, and in all error cases, err must be negative; if not, this is bug at that place and should be fixed before that goto. I now see that the TYPE_COMPOUND check forgets to set "err = -EINVAL;".
Regards, Clemens
On Fri, 2011-03-18 at 15:05 +0100, ext Clemens Ladisch wrote:
Sudarshan Bisht wrote:
On Thu, 2011-03-17 at 13:08 +0100, ext Clemens Ladisch wrote:
sudarshan.bisht@nokia.com wrote:
--- a/src/conf.c +++ b/src/conf.c @@ -3321,9 +3321,11 @@ static int snd_config_hooks_call(snd_config_t *root, snd_config_t *config, snd_c snd_config_delete(func_conf); if (err >= 0) { snd_config_t *nroot;
err = func(root, config, &nroot, private_data);if (err < 0)SNDERR("function %s returned error: %s", func_name, snd_strerror(err));
if (func) {err = func(root, config, &nroot, private_data);if (err < 0)SNDERR("function %s returned error: %s", func_name, snd_strerror(err));}The preceding "!func" and "err >= 0" checks already guarantee that func is valid.
But in case of "goto _err" , "!func" and "err >= 0" are not going to be checked.
"err >= 0" is checked, and in all error cases, err must be negative; if not, this is bug at that place and should be fixed before that goto. I now see that the TYPE_COMPOUND check forgets to set "err = -EINVAL;".
Yes, now I got your point, and I will take care of this in all cases.
Regards, Clemens
participants (3)
-
Clemens Ladisch -
Sudarshan Bisht -
sudarshan.bisht＠nokia.com