Re: [alsa-devel] coverity fix in alsa-libs

16 Sep 2014

      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
...

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 Iwaitiwai@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

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