[alsa-devel] [PATCH 1/3 v3] alsa-lib: fixed coverity reported issues under "FORWARD_NULL" checker.
From: Sudarshan Bisht 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.
--- modules/mixer/simple/sbasedl.c | 2 +- src/conf.c | 1 + src/hwdep/hwdep.c | 1 + src/pcm/pcm_hooks.c | 1 + src/pcm/pcm_simple.c | 2 +- src/rawmidi/rawmidi.c | 1 + src/rawmidi/rawmidi_virt.c | 8 +++++--- 7 files changed, 11 insertions(+), 5 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..ddefff6 100644 --- a/src/conf.c +++ b/src/conf.c @@ -3268,6 +3268,7 @@ static int snd_config_hooks_call(snd_config_t *root, snd_config_t *config, snd_c snd_config_iterator_t i, next; if (snd_config_get_type(func_conf) != SND_CONFIG_TYPE_COMPOUND) { SNDERR("Invalid type for func %s definition", str); + err = -EINVAL; goto _err; } snd_config_for_each(i, next, func_conf) { diff --git a/src/hwdep/hwdep.c b/src/hwdep/hwdep.c index b882b35..5dc791c 100644 --- a/src/hwdep/hwdep.c +++ b/src/hwdep/hwdep.c @@ -78,6 +78,7 @@ static int snd_hwdep_open_conf(snd_hwdep_t **hwdep, if (err >= 0) { if (snd_config_get_type(type_conf) != SND_CONFIG_TYPE_COMPOUND) { SNDERR("Invalid type for HWDEP type %s definition", str); + err = -EINVAL; goto _err; } snd_config_for_each(i, next, type_conf) { diff --git a/src/pcm/pcm_hooks.c b/src/pcm/pcm_hooks.c index 3a99d55..404d51e 100644 --- a/src/pcm/pcm_hooks.c +++ b/src/pcm/pcm_hooks.c @@ -385,6 +385,7 @@ static int snd_pcm_hook_add_conf(snd_pcm_t *pcm, snd_config_t *root, snd_config_ if (err >= 0) { if (snd_config_get_type(type) != SND_CONFIG_TYPE_COMPOUND) { SNDERR("Invalid type for PCM type %s definition", str); + err = -EINVAL; goto _err; } snd_config_for_each(i, next, type) { 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..0bd6b96 100644 --- a/src/rawmidi/rawmidi.c +++ b/src/rawmidi/rawmidi.c @@ -201,6 +201,7 @@ static int snd_rawmidi_open_conf(snd_rawmidi_t **inputp, snd_rawmidi_t **outputp if (err >= 0) { if (snd_config_get_type(type_conf) != SND_CONFIG_TYPE_COMPOUND) { SNDERR("Invalid type for RAWMIDI type %s definition", str); + err = -EINVAL; goto _err; } snd_config_for_each(i, next, type_conf) { 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)
From: Sudarshan Bisht 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 got fixes for many potential memory leaks.
--- modules/mixer/simple/sbase.c | 1 + modules/mixer/simple/sbasedl.c | 2 +- src/conf.c | 4 ++++ src/control/control_hw.c | 4 +++- src/pcm/pcm_file.c | 4 +++- src/pcm/pcm_ladspa.c | 20 ++++++++++++++------ src/pcm/pcm_plug.c | 9 +++++++-- src/pcm/pcm_rate.c | 4 ++++ src/rawmidi/rawmidi.c | 4 +++- 9 files changed, 40 insertions(+), 12 deletions(-)
diff --git a/modules/mixer/simple/sbase.c b/modules/mixer/simple/sbase.c index 97feee8..bb2f59d 100644 --- a/modules/mixer/simple/sbase.c +++ b/modules/mixer/simple/sbase.c @@ -377,6 +377,7 @@ static int simple_event_add1(snd_mixer_class_t *class, if (ctype != SND_CTL_ELEM_TYPE_BOOLEAN) { __invalid_type: snd_mixer_selem_id_free(id); + free(hsimple); return -EINVAL; } break; 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..c4daae7 100644 --- a/src/conf.c +++ b/src/conf.c @@ -638,6 +638,7 @@ static int get_char_skip_comments(input_t *input) fd = malloc(sizeof(*fd)); if (!fd) { free(str); + snd_input_close(in); return -ENOMEM; } fd->name = str; @@ -1847,6 +1848,7 @@ int snd_config_delete(snd_config_t *config) list_del(&config->list); free(config->id); free(config); + config = NULL; return 0; }
@@ -4589,6 +4591,8 @@ static int parse_args(snd_config_t *subs, const char *str, snd_config_t *defs) if (err < 0) { _err: free(val); + if (sub) + snd_config_delete(sub); return err; } free(val); diff --git a/src/control/control_hw.c b/src/control/control_hw.c index cf258b4..29cbbc6 100644 --- a/src/control/control_hw.c +++ b/src/control/control_hw.c @@ -230,8 +230,10 @@ static int snd_ctl_hw_elem_tlv(snd_ctl_t *handle, int op_flag, return -errno; } if (op_flag == 0) { - if (xtlv->tlv[1] + 2 * sizeof(unsigned int) > tlv_size) + if (xtlv->tlv[1] + 2 * sizeof(unsigned int) > tlv_size) { + free(xtlv); return -EFAULT; + } memcpy(tlv, xtlv->tlv, xtlv->tlv[1] + 2 * sizeof(unsigned int)); } free(xtlv); diff --git a/src/pcm/pcm_file.c b/src/pcm/pcm_file.c index bfa1cc8..adba7c0 100644 --- a/src/pcm/pcm_file.c +++ b/src/pcm/pcm_file.c @@ -226,7 +226,9 @@ static int snd_pcm_file_open_output_file(snd_pcm_file_t *file) file->final_fname); return -errno; } - fd = fileno(pipe); + fd = dup(fileno(pipe)); + if (fd >= 0) + fclose(pipe); } else { if (file->trunc) fd = open(file->final_fname, O_WRONLY|O_CREAT|O_TRUNC, diff --git a/src/pcm/pcm_ladspa.c b/src/pcm/pcm_ladspa.c index c413c10..41e1d38 100644 --- a/src/pcm/pcm_ladspa.c +++ b/src/pcm/pcm_ladspa.c @@ -750,8 +750,10 @@ static int snd_pcm_ladspa_allocate_memory(snd_pcm_t *pcm, snd_pcm_ladspa_t *lads if (instance->input.data == NULL || instance->input.m_data == NULL || instance->output.data == NULL || - instance->output.m_data == NULL) + instance->output.m_data == NULL) { + free(pchannels); return -ENOMEM; + } for (idx = 0; idx < instance->input.channels.size; idx++) { chn = instance->output.channels.array[idx]; if (pchannels[chn] == NULL && chn < ichannels) { @@ -761,8 +763,10 @@ static int snd_pcm_ladspa_allocate_memory(snd_pcm_t *pcm, snd_pcm_ladspa_t *lads instance->input.data[idx] = pchannels[chn]; if (instance->input.data[idx] == NULL) { instance->input.data[idx] = snd_pcm_ladspa_allocate_zero(ladspa, 0); - if (instance->input.data[idx] == NULL) - return -ENOMEM; + if (instance->input.data[idx] == NULL) { + free(pchannels); + return -ENOMEM; + } } } for (idx = 0; idx < instance->output.channels.size; idx++) { @@ -770,8 +774,10 @@ static int snd_pcm_ladspa_allocate_memory(snd_pcm_t *pcm, snd_pcm_ladspa_t *lads /* FIXME/OPTIMIZE: check if we can remove double alloc */ /* if LADSPA plugin has no broken inplace */ instance->output.data[idx] = malloc(sizeof(LADSPA_Data) * ladspa->allocated); - if (instance->output.data[idx] == NULL) - return -ENOMEM; + if (instance->output.data[idx] == NULL) { + free(pchannels); + return -ENOMEM; + } pchannels[chn] = instance->output.m_data[idx] = instance->output.data[idx]; } } @@ -793,8 +799,10 @@ static int snd_pcm_ladspa_allocate_memory(snd_pcm_t *pcm, snd_pcm_ladspa_t *lads instance->output.data[idx] = NULL; } else { instance->output.data[idx] = snd_pcm_ladspa_allocate_zero(ladspa, 1); - if (instance->output.data[idx] == NULL) + if (instance->output.data[idx] == NULL) { + free(pchannels); return -ENOMEM; + } } } } diff --git a/src/pcm/pcm_plug.c b/src/pcm/pcm_plug.c index e9d2923..e80bd3e 100644 --- a/src/pcm/pcm_plug.c +++ b/src/pcm/pcm_plug.c @@ -1298,6 +1298,7 @@ int _snd_pcm_plug_open(snd_pcm_t **pcmp, const char *name, err = snd_pcm_route_load_ttable(tt, ttable, csize, ssize, &cused, &sused, -1); if (err < 0) { snd_config_delete(sconf); + free(ttable); return err; } } @@ -1310,12 +1311,16 @@ int _snd_pcm_plug_open(snd_pcm_t **pcmp, const char *name,
err = snd_pcm_open_slave(&spcm, root, sconf, stream, mode, conf); snd_config_delete(sconf); - if (err < 0) + if (err < 0) { + free(ttable); return err; + } err = snd_pcm_plug_open(pcmp, name, sformat, schannels, srate, rate_converter, route_policy, ttable, ssize, cused, sused, spcm, 1); - if (err < 0) + if (err < 0) { + free(ttable); snd_pcm_close(spcm); + } return err; } #ifndef DOC_HIDDEN diff --git a/src/pcm/pcm_rate.c b/src/pcm/pcm_rate.c index 70e30e5..cf218ae 100644 --- a/src/pcm/pcm_rate.c +++ b/src/pcm/pcm_rate.c @@ -1392,11 +1392,13 @@ int snd_pcm_rate_open(snd_pcm_t **pcmp, const char *name, } else { SNDERR("Invalid type for rate converter"); snd_pcm_close(pcm); + free(rate); return -EINVAL; } if (err < 0) { SNDERR("Cannot find rate converter"); snd_pcm_close(pcm); + free(rate); return -ENOENT; } #else @@ -1405,6 +1407,7 @@ int snd_pcm_rate_open(snd_pcm_t **pcmp, const char *name, err = open_func(SND_PCM_RATE_PLUGIN_VERSION, &rate->obj, &rate->ops); if (err < 0) { snd_pcm_close(pcm); + free(rate); return err; } #endif @@ -1413,6 +1416,7 @@ int snd_pcm_rate_open(snd_pcm_t **pcmp, const char *name, ! rate->ops.input_frames || ! rate->ops.output_frames) { SNDERR("Inproper rate plugin %s initialization", type); snd_pcm_close(pcm); + free(rate); return err; }
diff --git a/src/rawmidi/rawmidi.c b/src/rawmidi/rawmidi.c index b28488a..25732d2 100644 --- a/src/rawmidi/rawmidi.c +++ b/src/rawmidi/rawmidi.c @@ -255,8 +255,10 @@ static int snd_rawmidi_open_conf(snd_rawmidi_t **inputp, snd_rawmidi_t **outputp snd_config_delete(type_conf); if (err >= 0) err = open_func(inputp, outputp, name, rawmidi_root, rawmidi_conf, mode); - if (err < 0) + if (err < 0) { + snd_dlclose(h); return err; + } if (inputp) { (*inputp)->dl_handle = h; h = NULL; snd_rawmidi_params_default(*inputp, ¶ms);
On 04/04/11 01:53, sudarshan.bisht@nokia.com wrote:
@@ -1847,6 +1848,7 @@ int snd_config_delete(snd_config_t *config) list_del(&config->list); free(config->id); free(config);
- config = NULL; return 0;
}
I don't see how this could be correct. You free memory, clear the calling param pointing to it and then return? What is the point of nulling the local pointer?
Regards, Steve
On Mon, 2011-04-04 at 10:30 -0700, ext Steve Calfee wrote:
On 04/04/11 01:53, sudarshan.bisht@nokia.com wrote:
@@ -1847,6 +1848,7 @@ int snd_config_delete(snd_config_t *config) list_del(&config->list); free(config->id); free(config);
- config = NULL; return 0;
}
I don't see how this could be correct. You free memory, clear the calling param pointing to it and then return? What is the point of nulling the local pointer?
yes, this is not needed actually.
Regards, Steve
Thanks,
-Sudarshan
From: Sudarshan Bisht 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 a fix for conditions where 1) variable is used evern after getting freed thus causing junk assignment and 2) double free of memory.
--- modules/mixer/simple/python.c | 1 + src/control/control_hw.c | 1 + src/mixer/mixer.c | 1 - src/mixer/simple_abst.c | 2 ++ 4 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/modules/mixer/simple/python.c b/modules/mixer/simple/python.c index c822c52..784d9a0 100644 --- a/modules/mixer/simple/python.c +++ b/modules/mixer/simple/python.c @@ -663,6 +663,7 @@ pymixer_attach_hctl(struct pymixer *pymixer, PyObject *args) return NULL; err = snd_mixer_attach_hctl(pymixer->mixer, hctl); if (err < 0) { + snd_hctl_close(hctl); PyErr_Format(PyExc_RuntimeError, "Cannot attach hctl: %s", snd_strerror(err)); return NULL; } diff --git a/src/control/control_hw.c b/src/control/control_hw.c index cf258b4..16c4987 100644 --- a/src/control/control_hw.c +++ b/src/control/control_hw.c @@ -414,6 +414,7 @@ int snd_ctl_hw_open(snd_ctl_t **handle, const char *name, int card, int mode) if (err < 0) { close(fd); free(hw); + return err; } ctl->ops = &snd_ctl_hw_ops; ctl->private_data = hw; diff --git a/src/mixer/mixer.c b/src/mixer/mixer.c index 85d843f..5e5789a 100644 --- a/src/mixer/mixer.c +++ b/src/mixer/mixer.c @@ -228,7 +228,6 @@ int snd_mixer_attach_hctl(snd_mixer_t *mixer, snd_hctl_t *hctl) return -ENOMEM; err = snd_hctl_nonblock(hctl, 1); if (err < 0) { - snd_hctl_close(hctl); free(slave); return err; } diff --git a/src/mixer/simple_abst.c b/src/mixer/simple_abst.c index 9e9aaf5..dd0dfa3 100644 --- a/src/mixer/simple_abst.c +++ b/src/mixer/simple_abst.c @@ -336,6 +336,8 @@ int snd_mixer_simple_basic_register(snd_mixer_t *mixer, err = find_module(class, top); if (err >= 0) err = snd_mixer_attach_hctl(mixer, priv->hctl); + if (err < 0) + goto __error; if (err >= 0) { priv->attach_flag = 1; err = snd_mixer_class_register(class, mixer);
Dear Sudarshan,
Am Montag, den 04.04.2011, 11:53 +0300 schrieb sudarshan.bisht@nokia.com:
[…]
diff --git a/src/mixer/simple_abst.c b/src/mixer/simple_abst.c index 9e9aaf5..dd0dfa3 100644 --- a/src/mixer/simple_abst.c +++ b/src/mixer/simple_abst.c @@ -336,6 +336,8 @@ int snd_mixer_simple_basic_register(snd_mixer_t *mixer, err = find_module(class, top); if (err >= 0) err = snd_mixer_attach_hctl(mixer, priv->hctl);
if (err < 0)goto __error;
There are still trailing white spaces at the end of both lines. The following command should show these to you with red color.
git log -p --color-words
if (err >= 0) { priv->attach_flag = 1; err = snd_mixer_class_register(class, mixer);
Thanks,
Paul
Hello Paul,
Thanks for pointing this out. I will correct it and send updated patch.
Br, Sudarshan Bisht
On Mon, 2011-04-04 at 11:12 +0200, ext Paul Menzel wrote:
Dear Sudarshan,
Am Montag, den 04.04.2011, 11:53 +0300 schrieb sudarshan.bisht@nokia.com:
[…]
diff --git a/src/mixer/simple_abst.c b/src/mixer/simple_abst.c index 9e9aaf5..dd0dfa3 100644 --- a/src/mixer/simple_abst.c +++ b/src/mixer/simple_abst.c @@ -336,6 +336,8 @@ int snd_mixer_simple_basic_register(snd_mixer_t *mixer, err = find_module(class, top); if (err >= 0) err = snd_mixer_attach_hctl(mixer, priv->hctl);
if (err < 0)goto __error;There are still trailing white spaces at the end of both lines. The following command should show these to you with red color.
git log -p --color-words
if (err >= 0) { priv->attach_flag = 1; err = snd_mixer_class_register(class, mixer);Thanks,
Paul _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Mon, Apr 04, 2011 at 11:53:57AM +0300, sudarshan.bisht@nokia.com wrote:
This patch has fix for situations where variable can be NULL but not been checked beforehand.
This description doesn't seem to match the patch...
- if (h == NULL)
- if (h) snd_dlclose(h);
This is a coding style change, the two conditions are equivalent.
SNDERR("Invalid type for func %s definition", str);
err = -EINVAL; goto _err;
This appears to be fixing an uninitialised return value error.
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)
This check alters the semantics - rather than checking if *period_time is non-zero we now check to see if period_time is non-NULL, paying no attention to the value if the pointer is set.
There was at least one relevant fix in there but lots of the changes are as above.
At Mon, 4 Apr 2011 10:18:23 +0100, Mark Brown wrote:
On Mon, Apr 04, 2011 at 11:53:57AM +0300, sudarshan.bisht@nokia.com wrote:
This patch has fix for situations where variable can be NULL but not been checked beforehand.
This description doesn't seem to match the patch...
- if (h == NULL)
- if (h) snd_dlclose(h);
This is a coding style change, the two conditions are equivalent.
I thought too at the first glance ;) It's the advantage of a systematic check.
Takashi
On Mon, Apr 04, 2011 at 12:35:11PM +0200, Takashi Iwai wrote:
Mark Brown wrote:
- if (h == NULL)
- if (h) snd_dlclose(h);
This is a coding style change, the two conditions are equivalent.
I thought too at the first glance ;) It's the advantage of a systematic check.
Hrm, yes. Actually the issue here was that due to many of the other changes I was reading very quickly - the patch looks a lot like a misfiring automation.
On Mon, 2011-04-04 at 10:18 +0100, ext Mark Brown wrote:
On Mon, Apr 04, 2011 at 11:53:57AM +0300, sudarshan.bisht@nokia.com wrote:
This patch has fix for situations where variable can be NULL but not been checked beforehand.
This description doesn't seem to match the patch...
- if (h == NULL)
- if (h) snd_dlclose(h);
This is a coding style change, the two conditions are equivalent.
SNDERR("Invalid type for func %s definition", str);
err = -EINVAL; goto _err;This appears to be fixing an uninitialised return value error.
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)This check alters the semantics - rather than checking if *period_time is non-zero we now check to see if period_time is non-NULL, paying no attention to the value if the pointer is set.
If "period_time == NULL" ( in preceding if condition) is true then "if (*period_time == 0)" has no meaning ( causes segmentation fault ) 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.
There was at least one relevant fix in there but lots of the changes are as above. _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
participants (6)
-
Mark Brown -
Paul Menzel -
Steve Calfee -
Sudarshan Bisht -
sudarshan.bisht＠nokia.com
-
Takashi Iwai