Re: [Qemu-devel] [5532] Replace uses of strndup (a GNU extension) with Qemu pstrdup

[andrzej zaborowski]

2008/10/26 Blue Swirl <address@hidden>:
> On 10/25/08, andrzej zaborowski <address@hidden> wrote:
>> 2008/10/25 Blue Swirl <address@hidden>:
>>  > On 10/25/08, andrzej zaborowski <address@hidden> wrote:
>>  >> 2008/10/25 Blue Swirl <address@hidden>:
>>  >>  > On 10/25/08, andrzej zaborowski <address@hidden> wrote:
>>  >>  >> 2008/10/25 Blue Swirl <address@hidden>:
>>  >>  >>  > On 10/25/08, andrzej zaborowski <address@hidden> wrote:
>>  >>  >>  >> 2008/10/25 Blue Swirl <address@hidden>:
>>  >>  >>  >>
>>  >>  >>  >> > Revision: 5532
>>  >>  >>  >>  >          
>> http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=5532
>>  >>  >>  >>  > Author:   blueswir1
>>  >>  >>  >>  > Date:     2008-10-25 11:23:27 +0000 (Sat, 25 Oct 2008)
>>  >>  >>  >>  >
>>  >>  >>  >>  > Log Message:
>>  >>  >>  >>  > -----------
>>  >>  >>  >>  > Replace uses of strndup (a GNU extension) with Qemu pstrdup
>>  >>  >>  >>  >
>>  >>  >>  >>  > Modified Paths:
>>  >>  >>  >>  > --------------
>>  >>  >>  >>  >    trunk/cutils.c
>>  >>  >>  >>  >    trunk/hw/bt-hci.c
>>  >>  >>  >>  >    trunk/qemu-common.h
>>  >>  >>  >>  >
>>  >>  >>  >>  > Modified: trunk/cutils.c
>>  >>  >>  >>  > 
>> ===================================================================
>>  >>  >>  >>  > --- trunk/cutils.c      2008-10-25 11:21:28 UTC (rev 5531)
>>  >>  >>  >>  > +++ trunk/cutils.c      2008-10-25 11:23:27 UTC (rev 5532)
>>  >>  >>  >>  > @@ -50,6 +50,18 @@
>>  >>  >>  >>  >     return buf;
>>  >>  >>  >>  >  }
>>  >>  >>  >>  >
>>  >>  >>  >>  > +/* strdup with a limit */
>>  >>  >>  >>  > +char *pstrdup(const char *str, size_t buf_size)
>>  >>  >>  >>  > +{
>>  >>  >>  >>  > +    size_t len;
>>  >>  >>  >>  > +    char *buf;
>>  >>  >>  >>  > +
>>  >>  >>  >>  > +    len = MIN(buf_size, strlen(str));
>>  >>  >>  >>  > +    buf = qemu_malloc(len);
>>  >>  >>  >>  > +    pstrcpy(buf, len, str);
>>  >>  >>  >>  > +    return buf;
>>  >>  >>  >>  > +}
>>  >>  >>  >>
>>  >>  >>  >>
>>  >>  >>  >> I think here also pstrcpy will only copy up to buf_size - 1 
>> characters
>>  >>  >>  >>  while strndup would copy buf_size chars.
>>  >>  >>  >
>>  >>  >>  > That is actually safer if we always want the strings to be NUL 
>> terminated.
>>  >>  >>
>>  >>  >>
>>  >>  >> strndup also always NUL terminates the string so it's just as safe,
>>  >>  >>  the length is just different.
>>  >>  >>
>>  >>  >>
>>  >>  >>  >
>>  >>  >>  > But the allocation length is wrong, it should be MIN(buf_size, 
>> strlen(str) + 1).
>>  >>  >>
>>  >>  >>
>>  >>  >> By my reading of the manual, it should rather be MIN(buf_size, 
>> strlen(str)) + 1.
>>  >>  >
>>  >>  > But then the length could be incorrect: buf_size + 1.
>>  >>
>>  >>
>>  >> That's what it should be in case lmp_name is 248 chars long.  I think
>>  >>  the confusion is because you called the parameter buf_size while
>>  >>  strndup calls it n (the number of characters.. not buffer size).
>>  >
>>  > If strlen(lmp_name) >= 248, only first 247 should be copied and the
>>  > final character should be NUL, because sizeof(read_local_name_rp.name)
>>  > == buf_size == 248.
>>
>>
>> Nope, actually now I notice that I assumed that strndup never executes
>>  strlen() on the parameter... I think it's a sane assumption (?).
>
> Yes, glibc uses strnlen:
> http://sources.redhat.com/cgi-bin/cvsweb.cgi/libc/string/strndup.c?rev=1.11&content-type=text/x-cvsweb-markup&cvsroot=glibc

Ah, cool.

>
>>  read_local_name_rp.name format is specified by the bluetooth "Volume 2
>>  Core system package, part E - Host Controller Interface Functional
>>  Specification" on pg. 394:
>>
>>  6.24 LOCAL NAME
>>  <...>
>>  If the name contained in the parameter is shorter than 248 octets, the
>>  end of the name is indicated by a NULL octet (0x00), and the following
>>  octets (to fill up 248 octets, which is the length of the parameter) do not
>>  have valid values.
>
> So, in this case neither strndup nor pstrdup are correct, because you
> do not want the string to be NUL terminated in the strlen(lmp_name) >=
> 248 case?

PARAM(change_local_name, name) format is defined by the specification,
but hci->device.lmp_name was just a C string, so that it could be used
for example with printf in qemu.  strndup converted it from one format
to the other so it was correct.  strncpy converted it back.

Cheers