[alsa-devel] [PATCH v2 6/7] topology: Add support for parsing vendor tuples

[Lin, Mengdong]

> -----Original Message-----
> From: alsa-devel-bounces at alsa-project.org
> [mailto:alsa-devel-bounces at alsa-project.org] On Behalf Of Takashi Iwai
> Sent: Tuesday, April 05, 2016 5:38 PM
> To: Mengdong Lin
> Cc: alsa-devel at alsa-project.org; Koul, Vinod; Lin, Mengdong;
> broonie at kernel.org; Ughreja, Rakesh A; Girdwood, Liam R; Shah, Hardik T;
> Prusty, Subhransu S
> Subject: Re: [alsa-devel] [PATCH v2 6/7] topology: Add support for parsing
> vendor tuples
> 
> On Tue, 05 Apr 2016 10:53:24 +0200,
> Mengdong Lin wrote:
> >
> >
> > On 04/05/2016 02:14 PM, Takashi Iwai wrote:
> > > On Tue, 05 Apr 2016 07:47:08 +0200,
> > > Mengdong Lin wrote:
> > >>
> > >>
> > >>
> > >> On 03/30/2016 03:35 PM, Takashi Iwai wrote:
> > >>> On Wed, 30 Mar 2016 09:11:17 +0200,
> mengdong.lin at linux.intel.com
> > >>> wrote:
> > >>>>
> > >>>> +		switch (type) {
> > >>>> +		case SND_SOC_TPLG_TUPLE_TYPE_UUID:
> > >>>> +			len = strlen(value);
> > >>>> +			if (len > 16 || len == 0) {
> > >>>> +				SNDERR("error: tuple %s: invalid uuid\n", id);
> > >>>> +				goto err;
> > >>>> +			}
> > >>>> +
> > >>>> +			memcpy(tuple->uuid, value, 16);
> > >>>
> > >>> This may still overflow :)
> > >>> How about simply using elem_copy_text()?
> > >>
> > >> Sorry for the late reply.
> > >>
> > >> Would you mind me using uuid_parse() here?
> > >> It can convert an input UUID string into the binary representation.
> > >>
> > >> An UUID string link "1b4e28ba-2fa1-11d2-883f-b9a761bde3fb" is user
> > >> friendly for the text conf file. But this will add dependency on libuuid.
> > >
> > > Additional dependency is no-go, especially when the required change
> > > is so trivial.  It's just a string copy, after all.
> > >
> >
> > Maybe we can just use strncpy(dest, src, 16), assuming the strncpy
> > will not try to write a "\0" at dest[16] that may cause overflow?
> 
> You seem to think of things more complicated than needed.
> 
> Just reread your code.  What if a shorter string value is passed there to
> memcpy() call?  That's what I suggested as an overflow.

I misunderstood your point and overlooked this bug. 
I should have checked the string length at first and then use the length in the memcpy.

Thanks again
Mengdong