coverity fix in alsa-libs

coverity fix in alsa-libs

[Renu Tyagi]

[-- Attachment #1: Type: text/plain, Size: 1577 bytes --]

Hi,

I ran Coverity analysis tool on alsa and found some bugs.
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


[-- Attachment #2: 22581.patch --]
[-- Type: application/octet-stream, Size: 1196 bytes --]

diff --git a/aserver/aserver.c b/aserver/aserver.c
index 73ea4e9..ca9aaf3 100644
--- a/aserver/aserver.c
+++ b/aserver/aserver.c
@@ -75,6 +75,7 @@ static int make_local_socket(const char *filename)
 	if (bind(sock, (struct sockaddr *) addr, size) < 0) {
 		int result = -errno;
 		SYSERROR("bind failed");
+		close(sock);
 		return result;
 	}
 
diff --git a/src/control/control_shm.c b/src/control/control_shm.c
index abab398..4083ec5 100644
--- a/src/control/control_shm.c
+++ b/src/control/control_shm.c
@@ -436,8 +436,11 @@ static int make_local_socket(const char *filename)
 	addr->sun_family = AF_LOCAL;
 	memcpy(addr->sun_path, filename, l);
 
-	if (connect(sock, (struct sockaddr *) addr, size) < 0)
+	if (connect(sock, (struct sockaddr *) addr, size) < 0){
+		SYSERR("connect failed");
+		close(sock);
 		return -errno;
+	}
 	return sock;
 }
 
diff --git a/src/pcm/pcm_shm.c b/src/pcm/pcm_shm.c
index 69d0524..3fbecca 100644
--- a/src/pcm/pcm_shm.c
+++ b/src/pcm/pcm_shm.c
@@ -649,6 +649,7 @@ static int make_local_socket(const char *filename)
 
 	if (connect(sock, (struct sockaddr *) addr, size) < 0) {
 		SYSERR("connect failed");
+		close(sock);
 		return -errno;
 	}
 	return sock;

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: coverity fix in alsa-libs

[Alexander E. Patrakov]

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

> 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

Re: coverity fix in alsa-libs

[Takashi Iwai]

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
> 

Re: coverity fix in alsa-libs

[Takashi Iwai]

At Mon, 15 Sep 2014 13:48:11 +0200,
Takashi Iwai wrote:
> 
> 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.

BTW, I haven't received the original mail from ML server.
(The delivery seems slowed down by some reason, so it might be that,
 though...)


Takashi

Re: coverity fix in alsa-libs

[Takashi Iwai]

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

Re: coverity fix in alsa-libs

[Takashi Iwai]

At Wed, 17 Sep 2014 03:17:08 +0000 (GMT),
Renu Tyagi wrote:
> 
> Hi Takashi,
> File in which changes are being made :  rawmidi.c
> Bug type - Handle h to be closed in case of error before returning
> PFA patch.

By some reason, your post doesn't appear on alsa-devel ML.
Did you subscribe the ML?  If not, please subscribe and repost.
Also, what about other patches you made?

Last but not least: please prepare a patch to be applicable via
git-am, including subject and from tags and the changelog content, and
don't forget your sign-off to the patch, just like a patch to Linux
kernel.


thanks,

Takashi

> 
> 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;
> 
> 
> Thanks & Regards, 
> Renu Tyagi
> [2 22578.patch <application/octet-stream (base64)>]
> 

Re: coverity fix in alsa-libs

[Renu Tyagi]

[-- Attachment #1: Type: text/plain, Size: 305 bytes --]

Hi, 

Sorry for the spam mail. I am sending the mail in plain text now.
PFA patch. 
File in which changes are being made : rawmidi.c
Bug type - Handle h to be closed in case of error before returning 
Meanwhile I am looking into the matter why my mails are not visible in ML.

Thanks 
Renu Tyagi 

[-- Attachment #2: patch_22578.patch --]
[-- Type: application/octet-stream, Size: 934 bytes --]

From ea6ea1452b0368c25ce41ea4434c29d553912fe3 Mon Sep 17 00:00:00 2001
From: renu555 <renu.tyagi@samsung.com>
Date: Wed, 17 Sep 2014 16:45:27 +0530
Subject: [PATCH 1/8] Handle h to be closed in case of error before returning


Signed-off-by: renu555 <renu.tyagi@samsung.com>
---
 src/rawmidi/rawmidi.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/src/rawmidi/rawmidi.c b/src/rawmidi/rawmidi.c
index b835b47..0ca5d1f 100644
--- a/src/rawmidi/rawmidi.c
+++ b/src/rawmidi/rawmidi.c
@@ -256,8 +256,11 @@ 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){
+		if (h)
+			snd_dlclose(h);
 		return err;
+	}
 	if (inputp) {
 		(*inputp)->dl_handle = h; h = NULL;
 		snd_rawmidi_params_default(*inputp, &params);
-- 
1.7.1


[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: coverity fix in alsa-libs

[Takashi Iwai]

At Thu, 18 Sep 2014 03:57:36 +0000 (GMT),
Renu Tyagi wrote:
> 
> Hi, 
> 
> Sorry for the spam mail. I am sending the mail in plain text now.
> PFA patch. 
> File in which changes are being made : rawmidi.c
> Bug type - Handle h to be closed in case of error before returning 
> Meanwhile I am looking into the matter why my mails are not visible in ML.

I see now on ML, so the problem was your post in HTML, indeed.

Regarding the patch: I see only a few cosmetic issue.

- Use embedded in the mail as much as possible rather than an
  attachment.  This makes review much easier.

- Put your real name in sign-off-by (and also From: line).  Refer to
  Documentation/SubmittingPatches in Linux kernel tree about the
  meaning of this line. 

- Put a space before the open brace.

- Add some prefix in the subject line to identify the area; in this
  case, add like "rawmidi: Handle d to be ...."
 

thanks,

Takashi