[PATCH 1/3 v3] alsa-lib: fixed coverity reported issues under "FORWARD_NULL" checker.

[PATCH 1/3 v3] alsa-lib: fixed coverity reported issues under "FORWARD_NULL" checker.

[sudarshan.bisht]

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)
-- 
1.7.0.4

[PATCH 2/3 v3] alsa-lib: fixed coverity reported issues under "RESOURCE_LEAK" checker.

[sudarshan.bisht]

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, &params);
-- 
1.7.0.4

Re: [PATCH 2/3 v3] alsa-lib: fixed coverity reported issues under "RESOURCE_LEAK" checker.

[Steve Calfee]

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

Re: [PATCH 2/3 v3] alsa-lib: fixed coverity reported issues under "RESOURCE_LEAK" checker.

[Sudarshan Bisht]

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 

[PATCH 3/3 v3] alsa-lib: fixed coverity reported issues under "USE_AFTER_FREE" checker.

[sudarshan.bisht]

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);
-- 
1.7.0.4

Re: [PATCH 3/3 v3] alsa-lib: fixed coverity reported issues under "USE_AFTER_FREE" checker.

[Paul Menzel]

[-- Attachment #1.1: Type: text/plain, Size: 898 bytes --]

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

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 3/3 v3] alsa-lib: fixed coverity reported issues under "USE_AFTER_FREE" checker.

[Sudarshan Bisht]

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


_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 1/3 v3] alsa-lib: fixed coverity reported issues under "FORWARD_NULL" checker.

[Mark Brown]

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.

Re: [PATCH 1/3 v3] alsa-lib: fixed coverity reported issues under "FORWARD_NULL" checker.

[Takashi Iwai]

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

Re: [PATCH 1/3 v3] alsa-lib: fixed coverity reported issues under "FORWARD_NULL" checker.

[Mark Brown]

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.

Re: [PATCH 1/3 v3] alsa-lib: fixed coverity reported issues under "FORWARD_NULL" checker.

[Sudarshan Bisht]

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