Re: coverity fix in alsa-libs

[Takashi Iwai <tiwai@suse.de>]

At Tue, 16 Sep 2014 04:08:48 +0000 (GMT),
Renu Tyagi wrote:
> 
> Hi Takashi,
> 
> Here are some more bugs and patches

Could you post one patch per mail?  This would make it much easier to
review.


thanks,

Takashi

> 
> 1. File in which changes are being made  :  conf.c
> Value of err to be Negative for correct error handling  
> Patch : 22657.patch
> 
> Community Code:
> if (err >= 0) {
> 	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);
> 		goto _err;
> }
> 
> Recommended Code :
> if (err >= 0) {
> 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;
> }
> 
> ----------------------------------------------------------------------------------------
> 
> 2.File in which changes are being made  :   control.c
> Value of err to be Negative for correct error handling  
> Patch : 22658.patch
> 
> Community Code:
> if (err >= 0) {
> 	if (snd_config_get_type(type_conf) != SND_CONFIG_TYPE_COMPOUND) {
> 		SNDERR("Invalid type for CTL type %s definition", str);
> 		goto _err;
> }
> 
> Recommended Code :
> if (err >= 0) {
> 		if (snd_config_get_type(type_conf) != SND_CONFIG_TYPE_COMPOUND) {
> 			SNDERR("Invalid type for CTL type %s definition", str);
> 			err = -EINVAL;
> 			goto _err;
> }	
> ----------------------------------------------------------------------------------------
> 
> 3.File in which changes are being made : mixer.c
> Double free by calling snd_hctl_close(hctl) twice  it is called inside snd_mixer_attach_hctl  also.
> Patch :  22638.patch
> 
> Community Code:
> int snd_mixer_attach(snd_mixer_t *mixer, const char *name)
> {
> 	snd_hctl_t *hctl;
> 	int err;
> 
> 	err = snd_hctl_open(&hctl, name, 0);
> 	if (err < 0)
> 		return err;
> 	err = snd_mixer_attach_hctl(mixer, hctl);
> 	if (err < 0) {
> 		snd_hctl_close(hctl);
> 		return err;
> 	}
> 	return 0;
> }
> 
> Recommended Code :
> int snd_mixer_attach(snd_mixer_t *mixer, const char *name)
> {
> 	snd_hctl_t *hctl;
> 	int err;
> 
> 	err = snd_hctl_open(&hctl, name, 0);
> 	if (err < 0)
> 		return err;
> 	err = snd_mixer_attach_hctl(mixer, hctl);
> 	if (err < 0) {
> /* Removed snd_hctl_close(hctl) to avoid double free, already called in snd_mixer_attach_hctl. */
> 		return err;
> 	}
> 	return 0;
> }
> ----------------------------------------------------------------------------------------
> 
> 4.File in which changes are being made : pcm.c
> Value of err to be Negative for correct error handling
> Patch : 22659.patch
> 
> Community Code:
> if (err >= 0) {
> 		if (snd_config_get_type(type_conf) != SND_CONFIG_TYPE_COMPOUND) {
> 			SNDERR("Invalid type for PCM type %s definition", str);
> 			goto _err;
> 		}
> 		snd_config_for_each(i, next, type_conf) {
> 			snd_config_t *n = snd_config_iterator_entry(i);
> Recommended Code :			
> if (err >= 0) {
> 		if (snd_config_get_type(type_conf) != SND_CONFIG_TYPE_COMPOUND) {
> 			SNDERR("Invalid type for PCM type %s definition", str);
> 			err = -EINVAL;
> 			goto _err;
> 		}
> 		snd_config_for_each(i, next, type_conf) {
> 			snd_config_t *n = snd_config_iterator_entry(i);
> 
> ----------------------------------------------------------------------------------------
> 		
> 5.File in which changes are being made : pcm_file.c
> Free resources before returning error.
> Patch : 22583.patch 
> Patch : 22584.patch 
> 
> Community Code:
> if (ifname && (stream == SND_PCM_STREAM_CAPTURE)) {
> 		ifd = open(ifname, O_RDONLY);	/* TODO: mind blocking mode */
> 		if (ifd < 0) {
> 			SYSERR("open %s for reading failed", ifname);
> 			free(file);
> 			return -errno;
> 		}
> 		file->ifname = strdup(ifname);
> 	}
> 	file->fd = fd;
> 	file->ifd = ifd;
> 	file->format = format;
> 	file->gen.slave = slave;
> 	file->gen.close_slave = close_slave;
> 
> 	err = snd_pcm_new(&pcm, SND_PCM_TYPE_FILE, name, slave->stream, slave->mode);
> 	if (err < 0) {
> 		free(file->fname);
> 		free(file);
> 		return err;
> 	}
> 	
> Recommended Code :
> if (ifname && (stream == SND_PCM_STREAM_CAPTURE)) {
> 		ifd = open(ifname, O_RDONLY);	/* TODO: mind blocking mode */
> 		if (ifd < 0) {
> 			SYSERR("open %s for reading failed", ifname);
> 			free(file->fname);
> 			free(file);
> 			return -errno;
> 		}
> 		file->ifname = strdup(ifname);
> 	}
> 	file->fd = fd;
> 	file->ifd = ifd;
> 	file->format = format;
> 	file->gen.slave = slave;
> 	file->gen.close_slave = close_slave;
> 
> 	err = snd_pcm_new(&pcm, SND_PCM_TYPE_FILE, name, slave->stream, slave->mode);
> 	if (err < 0) {
> 		free(file->fname);
> 		free(file->ifname);
> 		free(file);
> 		return err;
> 	}
> 	
> ----------------------------------------------------------------------------------------
> 
> 6.File in which changes are being made : pcm_hooks.c
> Null check before closing h
> Patch :22665.patch
> 
> Community Code:
> if (err >= 0)
> 		err = hook_add_dlobj(pcm, h);
> 
> 	if (err < 0) {
> 		snd_dlclose(h);
> 		return err;
> 	}
> 	return 0;
> }
> Recommended Code :
> if (err >= 0)
> 		err = hook_add_dlobj(pcm, h);
> 
> 	if (err < 0) {
> 		if(h)
> 		snd_dlclose(h);
> 		return err;
> 	}
> 	return 0;
> }
> ----------------------------------------------------------------------------------------
> 
> 7.File in which changes are being made : pcm_share.c
> Mutex unlock before returning
> Patch : 22670.patch
> 
> Community Code:
> Pthread_mutex_lock(&slave->mutex);
> 	err = pipe(slave->poll);
> 	if (err < 0) {
> 		SYSERR("can't create a pipe");
> 		return NULL;
> 	}
> 	while (slave->open_count > 0) {
> 		snd_pcm_uframes_t missing;
> 		// printf("begin min_missing\n");
> 		missing = _snd_pcm_share_slave_missing(slave);
> 		// printf("min_missing=%ld\n", missing);
> 		if (missing < INT_MAX) {
> 			snd_pcm_uframes_t hw_ptr;
> 			snd_pcm_sframes_t avail_min;
> 			hw_ptr = slave->hw_ptr + missing;
> 			hw_ptr += spcm->period_size - 1;
> 			if (hw_ptr >= spcm->boundary)
> 				hw_ptr -= spcm->boundary;
> 			hw_ptr -= hw_ptr % spcm->period_size;
> 			avail_min = hw_ptr - *spcm->appl.ptr;
> 			if (spcm->stream == SND_PCM_STREAM_PLAYBACK)
> 				avail_min += spcm->buffer_size;
> 			if (avail_min < 0)
> 				avail_min += spcm->boundary;
> 			// printf("avail_min=%d\n", avail_min);
> 			if ((snd_pcm_uframes_t)avail_min != spcm->avail_min) {
> 				snd_pcm_sw_params_set_avail_min(spcm, &slave->sw_params, avail_min);
> 				err = snd_pcm_sw_params(spcm, &slave->sw_params);
> 				if (err < 0) {
> 					SYSERR("snd_pcm_sw_params error");
> 					return NULL;
> 				}
> 			}
> 
> Recommended Code :
> 	Pthread_mutex_lock(&slave->mutex);
> 	err = pipe(slave->poll);
> 	if (err < 0) {
> 		SYSERR("can't create a pipe");
> Pthread_mutex_unlock(&slave->mutex);
> 		return NULL;
> 	}
> 	while (slave->open_count > 0) {
> 		snd_pcm_uframes_t missing;
> 		// printf("begin min_missing\n");
> 		missing = _snd_pcm_share_slave_missing(slave);
> 		// printf("min_missing=%ld\n", missing);
> 		if (missing < INT_MAX) {
> 			snd_pcm_uframes_t hw_ptr;
> 			snd_pcm_sframes_t avail_min;
> 			hw_ptr = slave->hw_ptr + missing;
> 			hw_ptr += spcm->period_size - 1;
> 			if (hw_ptr >= spcm->boundary)
> 				hw_ptr -= spcm->boundary;
> 			hw_ptr -= hw_ptr % spcm->period_size;
> 			avail_min = hw_ptr - *spcm->appl.ptr;
> 			if (spcm->stream == SND_PCM_STREAM_PLAYBACK)
> 				avail_min += spcm->buffer_size;
> 			if (avail_min < 0)
> 				avail_min += spcm->boundary;
> 			// printf("avail_min=%d\n", avail_min);
> 			if ((snd_pcm_uframes_t)avail_min != spcm->avail_min) {
> 				snd_pcm_sw_params_set_avail_min(spcm, &slave->sw_params, avail_min);
> 				err = snd_pcm_sw_params(spcm, &slave->sw_params);
> 				if (err < 0) {
> 					SYSERR("snd_pcm_sw_params error");
> 					Pthread_mutex_unlock(&slave->mutex);
> 					return NULL;
> 				}
> 			}
> ----------------------------------------------------------------------------------------
> 8.File in which changes are being made :  rawmidi.c
> Handle h to be closed in case of error before returning
> Patch :22578.patch
> 
> Community Code:
> 	if (err >= 0)
> 		err = open_func(inputp, outputp, name, rawmidi_root, rawmidi_conf, mode);
> 	if (err < 0)
> 		return err;
> 	if (inputp) {
> 		(*inputp)->dl_handle = h; h = NULL;
> 
> Recommended Code :
> 	if (err >= 0)
> 		err = open_func(inputp, outputp, name, rawmidi_root, rawmidi_conf, mode);
> 	if (err < 0){
> 		if (h)
> 		snd_dlclose(h);
> 		return err;
> 	}
> 	if (inputp) {
> 		(*inputp)->dl_handle = h; h = NULL;
> ----------------------------------------------------------------------------------------
> 
> 9.File in which changes are being made : sbase.c
> Freeing the resources before returning in case of error
> Patch : 22579.patch 
> 
> Community Code:
> hsimple = calloc(1, sizeof(*hsimple));
> 	if (hsimple == NULL) {
> 		snd_mixer_selem_id_free(id);
> 		return -ENOMEM;
> 	}
> 	switch (sel->purpose) {
> 	case PURPOSE_SWITCH:
> 		if (ctype != SND_CTL_ELEM_TYPE_BOOLEAN) {
> 		      __invalid_type:
> 		      	snd_mixer_selem_id_free(id);
> 			return -EINVAL;
> 		}
> 		break;
> 
> Recommended Code :
> hsimple = calloc(1, sizeof(*hsimple));
> 	if (hsimple == NULL) {
> 		snd_mixer_selem_id_free(id);
> 		return -ENOMEM;
> 	}
> 	switch (sel->purpose) {
> 	case PURPOSE_SWITCH:
> 		if (ctype != SND_CTL_ELEM_TYPE_BOOLEAN) {
> 		      __invalid_type:
> 		      	snd_mixer_selem_id_free(id);
> 			free(hsimple);
> 			return -EINVAL;
> 		}
> 		break;
> ---------------------------------------------------------------------------
> 
> 10.File in which changes are being made : socket.c
> Socket not closed before returning when error occurs
> Patch :22587.patch
> 
> Community Code:
> s = socket(PF_INET, SOCK_STREAM, 0);
> 	if (s < 0) {
> 		SYSERR("socket failed");
> 		return -errno;
> 	}
> 	
> 	conf.ifc_len = numreqs * sizeof(struct ifreq);
> 	conf.ifc_buf = malloc((unsigned int) conf.ifc_len);
> 	if (! conf.ifc_buf)
> 		return -ENOMEM;
> 	while (1) {
> 		err = ioctl(s, SIOCGIFCONF, &conf);
> 		if (err < 0) {
> 			SYSERR("SIOCGIFCONF failed");
> 			return -errno;
> 		}
> 		if ((size_t)conf.ifc_len < numreqs * sizeof(struct ifreq))
> 			break;
> 		numreqs *= 2;
> 		conf.ifc_len = numreqs * sizeof(struct ifreq);
> 		conf.ifc_buf = realloc(conf.ifc_buf, (unsigned int) conf.ifc_len);
> 		if (! conf.ifc_buf)
> 			return -ENOMEM;
> 	}
> 
> Recommended Code :
> 	s = socket(PF_INET, SOCK_STREAM, 0);
> 	if (s < 0) {
> 		SYSERR("socket failed");
> 		return -errno;
> 	}
> 	
> 	conf.ifc_len = numreqs * sizeof(struct ifreq);
> 	conf.ifc_buf = malloc((unsigned int) conf.ifc_len);
> 	if (! conf.ifc_buf){
> 		close(s);
> 		return -ENOMEM;
> 	}
> 	while (1) {
> 		err = ioctl(s, SIOCGIFCONF, &conf);
> 		if (err < 0) {
> 			SYSERR("SIOCGIFCONF failed");
> 			close(s);
> 			return -errno;
> 		}
> 		if ((size_t)conf.ifc_len < numreqs * sizeof(struct ifreq))
> 			break;
> 		numreqs *= 2;
> 		conf.ifc_len = numreqs * sizeof(struct ifreq);
> 		conf.ifc_buf = realloc(conf.ifc_buf, (unsigned int) conf.ifc_len);
> 		if (! conf.ifc_buf){
> 			close(s);
> 			return -ENOMEM;
> 		}
> 	}
> ----------------------------------------------------------------------
> 
> 11.File in which changes are being made : simple_abst.c
> If lib is NULL return error to avoid strlen of NULL string.
> Patch :22667.patch
> 
> Community Code:
> 	path = getenv("ALSA_MIXER_SIMPLE_MODULES");
> 	if (!path)
> 		path = SO_PATH;
> 	xlib = malloc(strlen(lib) + strlen(path) + 1 + 1);
> 	if (xlib == NULL)
> 		return -ENOMEM;
> 
> Recommended Code :
> 	path = getenv("ALSA_MIXER_SIMPLE_MODULES");
> 	if (!lib)
> 		return -ENXIO;
> 	if (!path)
> 		path = SO_PATH;
> 	xlib = malloc(strlen(lib) + strlen(path) + 1 + 1);
> 	if (xlib == NULL)
> 		return -ENOMEM;
> ------------------------------------------------------------------------------
> 
> ------- Original Message -------
> Sender : Takashi Iwai<tiwai@suse.de>
> Date : Sep 15, 2014 17:18 (GMT+05:30)
> Title : Re: [alsa-devel] coverity fix in alsa-libs
> 
> At Mon, 15 Sep 2014 17:36:02 +0600,
> Alexander E. Patrakov wrote:
> > 
> > 15.09.2014 15:25, Renu Tyagi wrote:
> > > Hi,
> > >
> > > I ran Coverity analysis tool on alsa and found some bugs.
> > 
> > May I suggest that we remove aserver and the shm plugin instead of 
> > applying the patch? Three days ago I tried to use it for testing my fix 
> > to the share plugin, but failed. In other words: if even speaker-test 
> > cannot be made to work on it without crashing and/or hanging or valgrind 
> > errors, then I'd rather be aggressive here.
> > 
> > And next time please CC: Takashi Iwai on all alsa-lib patches :)
> 
> I'm for removing aserver & co, too, but let's put it as post-1.0.29
> item, since Jaroslav seems preparing 1.0.29 release now.
> 
> 
> thanks,
> 
> Takashi
> 
> > 
> > > Bug and Patch description
> > >
> > > 1. Changed file  :  aserver.c
> > > Socket not closed before returning when bind fails
> > > Community Code:
> > >
> > > if (bind(sock, (struct sockaddr *) addr, size) < 0) {
> > > int result = -errno;
> > > SYSERROR("bind failed");
> > > return result;
> > > }
> > > return sock;
> > > }
> > >
> > > Recommended Code :
> > >
> > > if (bind(sock, (struct sockaddr *) addr, size) < 0) {
> > > int result = -errno;
> > > SYSERROR("bind failed");
> > > close(sock);
> > > return result;
> > > }
> > > return sock;
> > > }
> > >
> > > 2.Changed file : control_shm.c
> > > Socket not closed before returning when connect fails
> > >
> > > Community Code:
> > > if (connect(sock, (struct sockaddr *) addr, size) < 0)
> > >      return -errno;
> > > return sock;
> > > }
> > >
> > > Recommended Code :
> > > if (connect(sock, (struct sockaddr *) addr, size) < 0){
> > >      SYSERR("connect failed");
> > >      close(sock);
> > >      return -errno;
> > > }
> > > return sock;
> > > }
> > >
> > > 3.Changed file : pcm_shm.c
> > > Socket not closed before returning when connect fails
> > >
> > > Community Code:
> > > if (connect(sock, (struct sockaddr *) addr, size) < 0) {
> > >     SYSERR("connect failed");
> > > return -errno;
> > >     }
> > >   return sock;
> > > }
> > > Recommended Code :
> > > if (connect(sock, (struct sockaddr *) addr, size) < 0) {
> > >   SYSERR("connect failed");
> > >   close(sock);
> > >   return -errno;
> > >   }
> > > return sock;
> > > }
> > >
> > > PFA patch.
> > >
> > >
> > >
> > >
> > >
> > > Thanks & Regards,
> > >
> > > Renu Tyagi
> > >
> > >
> > >
> > > _______________________________________________
> > > Alsa-devel mailing list
> > > Alsa-devel@alsa-project.org
> > > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> > >
> > 
> > 
> > -- 
> > Alexander E. Patrakov
> > 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 
> 
> Thanks & Regards,
>  
> Renu Tyagi
> SRI-Delhi (SW Platform) | Seat Number 3A-157
> Samsung India Electronics Pvt. Ltd.,
> 2A, Sector 126, Noida -201303
> [2 22657.patch <application/octet-stream (base64)>]
> 
> [3 22658.patch <application/octet-stream (base64)>]
> 
> [4 22638.patch <application/octet-stream (base64)>]
> 
> [5 22659.patch <application/octet-stream (base64)>]
> 
> [6 22583.patch <application/octet-stream (base64)>]
> 
> [7 22584.patch <application/octet-stream (base64)>]
> 
> [8 22665.patch <application/octet-stream (base64)>]
> 
> [9 22670.patch <application/octet-stream (base64)>]
> 
> [10 22578.patch <application/octet-stream (base64)>]
> 
> [11 22579.patch <application/octet-stream (base64)>]
> 
> [12 22587.patch <application/octet-stream (base64)>]
> 
> [13 22667.patch <application/octet-stream (base64)>]
>