valgrind error in snd_tlv_get_dB_range

valgrind error in snd_tlv_get_dB_range

[Ivan Sorokin]

One program (qasmixer -- alsa mixer) on my system prints lots of 
valgrind errors.

The first one is this:

==25716== Conditional jump or move depends on uninitialised value(s)
==25716==    at 0x4E72307: snd_tlv_get_dB_range (tlv.c:170)
==25716==    by 0x4E8A746: get_dB_range (simple_none.c:1162)
==25716==    by 0x4E8A7E7: get_dB_range_ops (simple_none.c:1176)
==25716==    by 0x4E85590: snd_mixer_selem_get_playback_dB_range 
(simple.c:298)
==25716==    by 0x472B2B: 
QSnd::Mixer_Simple_Elem::set_snd_mixer_selem_id(_snd_mixer_selem_id*) 
(mixer_simple_elem.cpp:161)
==25716==    by 0x4724B8: 
QSnd::Mixer_Simple_Elem::Mixer_Simple_Elem(QObject*, _snd_mixer*, 
_snd_mixer_selem_id*) (mixer_simple_elem.cpp:31)
==25716==    by 0x479F17: 
QSnd::Mixer_Simple::create_mixer_elem(_snd_mixer_elem*) 
(mixer_simple.cpp:318)
==25716==    by 0x4797B3: QSnd::Mixer_Simple::create_mixer_elems() 
(mixer_simple.cpp:217)
==25716==    by 0x4795DF: QSnd::Mixer_Simple::open(QString const&) 
(mixer_simple.cpp:178)
==25716==    by 0x4D4754: Tray_Mixer::load_mixer() (tray_mixer.cpp:222)
==25716==    by 0x4D4505: 
Tray_Mixer::set_mdev_setup(Tray_Mixer_MDev_Setup*) (tray_mixer.cpp:137)
==25716==    by 0x4DB863: Desktop_Items::tray_mixer_create() 
(desktop_items.cpp:504)
==25716==  Uninitialised value was created by a heap allocation
==25716==    at 0x4C2CD7B: malloc (in 
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==25716==    by 0x4E8A572: init_db_range (simple_none.c:1114)
==25716==    by 0x4E8A70D: get_dB_range (simple_none.c:1159)
==25716==    by 0x4E8A7E7: get_dB_range_ops (simple_none.c:1176)
==25716==    by 0x4E85590: snd_mixer_selem_get_playback_dB_range 
(simple.c:298)
==25716==    by 0x472B2B: 
QSnd::Mixer_Simple_Elem::set_snd_mixer_selem_id(_snd_mixer_selem_id*) 
(mixer_simple_elem.cpp:161)
==25716==    by 0x4724B8: 
QSnd::Mixer_Simple_Elem::Mixer_Simple_Elem(QObject*, _snd_mixer*, 
_snd_mixer_selem_id*) (mixer_simple_elem.cpp:31)
==25716==    by 0x479F17: 
QSnd::Mixer_Simple::create_mixer_elem(_snd_mixer_elem*) 
(mixer_simple.cpp:318)
==25716==    by 0x4797B3: QSnd::Mixer_Simple::create_mixer_elems() 
(mixer_simple.cpp:217)
==25716==    by 0x4795DF: QSnd::Mixer_Simple::open(QString const&) 
(mixer_simple.cpp:178)
==25716==    by 0x4D4754: Tray_Mixer::load_mixer() (tray_mixer.cpp:222)
==25716==    by 0x4D4505: 
Tray_Mixer::set_mdev_setup(Tray_Mixer_MDev_Setup*) (tray_mixer.cpp:137)

I compiled libasound2 from the latest git from 
git://gitorious.org/alsa/alsa-lib.git with -g -O0, so you can see line 
numbers.

As I understand the problem is that init_db_range initialize invalid 
rec->db_info, because snd_hctl_elem_tlv_read failed to fully initialize tlv:

> 	tlv = malloc(tlv_size);                  // line 1114
> 	if (! tlv)
> 		return -ENOMEM;
> 	if (snd_hctl_elem_tlv_read(ctl, tlv, tlv_size) < 0)
> 		goto error;
> 	db_size = snd_tlv_parse_dB_info(tlv, tlv_size, &dbrec);
> 	if (db_size < 0)
> 		goto error;
> 	rec->db_info = malloc(db_size);
> 	if (!rec->db_info)
> 		goto error;
> 	memcpy(rec->db_info, dbrec, db_size);
> 	free(tlv);

I tried to trace problem in snd_hctl_elem_tlv_read, but I failed to do 
so due to lack of experience with alsa-lib.

Could someone more experienced with alsa-lib look at the problem?

Re: valgrind error in snd_tlv_get_dB_range

[Clemens Ladisch]

Ivan Sorokin wrote:
> One program (qasmixer -- alsa mixer) on my system prints lots of valgrind errors.

Yes, these are errors in valgrind.

> As I understand the problem is that init_db_range initialize invalid rec->db_info, because snd_hctl_elem_tlv_read failed to fully initialize tlv:

snd_hctl_elem_tlv_read initializes as many bytes as needed.

I'd guess valgrind doesn't notice this because this is done through
an ioctl, in the kernel.


Regards,
Clemens

Re: valgrind error in snd_tlv_get_dB_range

[Ivan Sorokin]

On 19.01.2014 14:16, Ivan Sorokin wrote:

> I tried to trace problem in snd_hctl_elem_tlv_read, but I failed to do
> so due to lack of experience with alsa-lib.
>
> Could someone more experienced with alsa-lib look at the problem?

After a bit more investigation I've found that tlv is initialized in 
snd_ctl_hw_elem_tlv with memcpy in line 245. This initialization looks 
perfectly correct. So perhaps this is a error in valgrind (unimplemented 
ioctl).

P.S. I believe I found a memory leak:

> 	switch (op_flag) {
> 	case -1: inum = SNDRV_CTL_IOCTL_TLV_COMMAND; break;
>  	case 0:	inum = SNDRV_CTL_IOCTL_TLV_READ; break;
> 	case 1:	inum = SNDRV_CTL_IOCTL_TLV_WRITE; break;
> 	default: return -EINVAL;
> 	}
> 	xtlv = malloc(sizeof(struct snd_ctl_tlv) + tlv_size);
> 	if (xtlv == NULL)
> 		return -ENOMEM;
> 	xtlv->numid = numid;
> 	xtlv->length = tlv_size;
> 	memcpy(xtlv->tlv, tlv, tlv_size);
> 	if (ioctl(hw->fd, inum, xtlv) < 0) {
> 		free(xtlv);
> 		return -errno;
> 	}
> 	if (op_flag == 0) {
> 		if (xtlv->tlv[1] + 2 * sizeof(unsigned int) > tlv_size)

missing free(xtlv) here

> 			return -EFAULT;
> 		memcpy(tlv, xtlv->tlv, xtlv->tlv[1] + 2 * sizeof(unsigned int));
> 	}
> 	free(xtlv);
> 	return 0;

Re: valgrind error in snd_tlv_get_dB_range

[Takashi Iwai]

At Sun, 19 Jan 2014 16:22:47 +0400,
Ivan Sorokin wrote:
> 
> On 19.01.2014 14:16, Ivan Sorokin wrote:
> 
> > I tried to trace problem in snd_hctl_elem_tlv_read, but I failed to do
> > so due to lack of experience with alsa-lib.
> >
> > Could someone more experienced with alsa-lib look at the problem?
> 
> After a bit more investigation I've found that tlv is initialized in 
> snd_ctl_hw_elem_tlv with memcpy in line 245. This initialization looks 
> perfectly correct. So perhaps this is a error in valgrind (unimplemented 
> ioctl).
> 
> P.S. I believe I found a memory leak:
> 
> > 	switch (op_flag) {
> > 	case -1: inum = SNDRV_CTL_IOCTL_TLV_COMMAND; break;
> >  	case 0:	inum = SNDRV_CTL_IOCTL_TLV_READ; break;
> > 	case 1:	inum = SNDRV_CTL_IOCTL_TLV_WRITE; break;
> > 	default: return -EINVAL;
> > 	}
> > 	xtlv = malloc(sizeof(struct snd_ctl_tlv) + tlv_size);
> > 	if (xtlv == NULL)
> > 		return -ENOMEM;
> > 	xtlv->numid = numid;
> > 	xtlv->length = tlv_size;
> > 	memcpy(xtlv->tlv, tlv, tlv_size);
> > 	if (ioctl(hw->fd, inum, xtlv) < 0) {
> > 		free(xtlv);
> > 		return -errno;
> > 	}
> > 	if (op_flag == 0) {
> > 		if (xtlv->tlv[1] + 2 * sizeof(unsigned int) > tlv_size)
> 
> missing free(xtlv) here

Good catch.  Care to send a proper fix patch?


thanks,

Takashi

> 
> > 			return -EFAULT;
> > 		memcpy(tlv, xtlv->tlv, xtlv->tlv[1] + 2 * sizeof(unsigned int));
> > 	}
> > 	free(xtlv);
> > 	return 0;
> 
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>