[alsa-devel] coverity fix in alsa-libs
Takashi Iwai
tiwai at suse.de
Tue Sep 16 16:40:06 CEST 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
>
> 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 at 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 at alsa-project.org
> > > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> > >
> >
> >
> > --
> > Alexander E. Patrakov
> >
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel at 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)>]
>
More information about the Alsa-devel
mailing list