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

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

[Blue Swirl]

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;
+}
+
 int strstart(const char *str, const char *val, const char **ptr)
 {
     const char *p, *q;

Modified: trunk/hw/bt-hci.c
===================================================================
--- trunk/hw/bt-hci.c	2008-10-25 11:21:28 UTC (rev 5531)
+++ trunk/hw/bt-hci.c	2008-10-25 11:23:27 UTC (rev 5532)
@@ -1814,7 +1814,7 @@
 
         if (hci->device.lmp_name)
             free((void *) hci->device.lmp_name);
-        hci->device.lmp_name = strndup(PARAM(change_local_name, name),
+        hci->device.lmp_name = pstrdup(PARAM(change_local_name, name),
                         sizeof(PARAM(change_local_name, name)));
         bt_hci_event_complete_status(hci, HCI_SUCCESS);
         break;

Modified: trunk/qemu-common.h
===================================================================
--- trunk/qemu-common.h	2008-10-25 11:21:28 UTC (rev 5531)
+++ trunk/qemu-common.h	2008-10-25 11:23:27 UTC (rev 5532)
@@ -82,6 +82,7 @@
 /* cutils.c */
 void pstrcpy(char *buf, int buf_size, const char *str);
 char *pstrcat(char *buf, int buf_size, const char *s);
+char *pstrdup(const char *str, size_t buf_size);
 int strstart(const char *str, const char *val, const char **ptr);
 int stristart(const char *str, const char *val, const char **ptr);
 time_t mktimegm(struct tm *tm);

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

[andrzej zaborowski]

2008/10/25 Blue Swirl <blauwirbel@gmail.com>:
> 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.

Cheers

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

[Blue Swirl]

On 10/25/08, andrzej zaborowski <balrogg@gmail.com> wrote:
> 2008/10/25 Blue Swirl <blauwirbel@gmail.com>:
>
> > 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.

But the allocation length is wrong, it should be MIN(buf_size, strlen(str) + 1).

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

[andrzej zaborowski]

2008/10/25 Blue Swirl <blauwirbel@gmail.com>:
> On 10/25/08, andrzej zaborowski <balrogg@gmail.com> wrote:
>> 2008/10/25 Blue Swirl <blauwirbel@gmail.com>:
>>
>> > 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.

Now that I think of it, note that this changed malloc to qemu_malloc
so you need to change the respective free()s to qemu_free()s.
Cheers

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

[Blue Swirl]

On 10/25/08, andrzej zaborowski <balrogg@gmail.com> wrote:
> 2008/10/25 Blue Swirl <blauwirbel@gmail.com>:
>  > On 10/25/08, andrzej zaborowski <balrogg@gmail.com> wrote:
>  >> 2008/10/25 Blue Swirl <blauwirbel@gmail.com>:
>  >>
>  >> > 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.

>  Now that I think of it, note that this changed malloc to qemu_malloc
>  so you need to change the respective free()s to qemu_free()s.

True.

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

[andrzej zaborowski]

2008/10/25 Blue Swirl <blauwirbel@gmail.com>:
> On 10/25/08, andrzej zaborowski <balrogg@gmail.com> wrote:
>> 2008/10/25 Blue Swirl <blauwirbel@gmail.com>:
>>  > On 10/25/08, andrzej zaborowski <balrogg@gmail.com> wrote:
>>  >> 2008/10/25 Blue Swirl <blauwirbel@gmail.com>:
>>  >>
>>  >> > 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).

Note that you can use memcpy instead of pstrcpy because the length is
known so there's no need for the overhead.

Cheers

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

[Blue Swirl]

On 10/25/08, andrzej zaborowski <balrogg@gmail.com> wrote:
> 2008/10/25 Blue Swirl <blauwirbel@gmail.com>:
>  > On 10/25/08, andrzej zaborowski <balrogg@gmail.com> wrote:
>  >> 2008/10/25 Blue Swirl <blauwirbel@gmail.com>:
>  >>  > On 10/25/08, andrzej zaborowski <balrogg@gmail.com> wrote:
>  >>  >> 2008/10/25 Blue Swirl <blauwirbel@gmail.com>:
>  >>  >>
>  >>  >> > 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.

>  Note that you can use memcpy instead of pstrcpy because the length is
>  known so there's no need for the overhead.

Right.

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

[andrzej zaborowski]

2008/10/25 Blue Swirl <blauwirbel@gmail.com>:
> On 10/25/08, andrzej zaborowski <balrogg@gmail.com> wrote:
>> 2008/10/25 Blue Swirl <blauwirbel@gmail.com>:
>>  > On 10/25/08, andrzej zaborowski <balrogg@gmail.com> wrote:
>>  >> 2008/10/25 Blue Swirl <blauwirbel@gmail.com>:
>>  >>  > On 10/25/08, andrzej zaborowski <balrogg@gmail.com> wrote:
>>  >>  >> 2008/10/25 Blue Swirl <blauwirbel@gmail.com>:
>>  >>  >>
>>  >>  >> > 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 (?).

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.

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

[Blue Swirl]

On 10/25/08, andrzej zaborowski <balrogg@gmail.com> wrote:
> 2008/10/25 Blue Swirl <blauwirbel@gmail.com>:
>  > On 10/25/08, andrzej zaborowski <balrogg@gmail.com> wrote:
>  >> 2008/10/25 Blue Swirl <blauwirbel@gmail.com>:
>  >>  > On 10/25/08, andrzej zaborowski <balrogg@gmail.com> wrote:
>  >>  >> 2008/10/25 Blue Swirl <blauwirbel@gmail.com>:
>  >>  >>  > On 10/25/08, andrzej zaborowski <balrogg@gmail.com> wrote:
>  >>  >>  >> 2008/10/25 Blue Swirl <blauwirbel@gmail.com>:
>  >>  >>  >>
>  >>  >>  >> > 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

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

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

[andrzej zaborowski]

2008/10/26 Blue Swirl <blauwirbel@gmail.com>:
> On 10/25/08, andrzej zaborowski <balrogg@gmail.com> wrote:
>> 2008/10/25 Blue Swirl <blauwirbel@gmail.com>:
>>  > On 10/25/08, andrzej zaborowski <balrogg@gmail.com> wrote:
>>  >> 2008/10/25 Blue Swirl <blauwirbel@gmail.com>:
>>  >>  > On 10/25/08, andrzej zaborowski <balrogg@gmail.com> wrote:
>>  >>  >> 2008/10/25 Blue Swirl <blauwirbel@gmail.com>:
>>  >>  >>  > On 10/25/08, andrzej zaborowski <balrogg@gmail.com> wrote:
>>  >>  >>  >> 2008/10/25 Blue Swirl <blauwirbel@gmail.com>:
>>  >>  >>  >>
>>  >>  >>  >> > 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

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

[Warner Losh]

From: "andrzej zaborowski" <balrogg@gmail.com>
Subject: Re: [Qemu-devel] [5532] Replace uses of strndup (a GNU extension) with Qemu pstrdup
Date: Sat, 25 Oct 2008 14:03:40 +0200

> 2008/10/25 Blue Swirl <blauwirbel@gmail.com>:
> > 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.

Is this really safe?  If str is very long, this can still run off into
the weeds...

Warner

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

[andrzej zaborowski]

2008/10/25 Blue Swirl <blauwirbel@gmail.com>:
> 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;
> +}
> +
>  int strstart(const char *str, const char *val, const char **ptr)
>  {
>     const char *p, *q;
>
> Modified: trunk/hw/bt-hci.c
> ===================================================================
> --- trunk/hw/bt-hci.c   2008-10-25 11:21:28 UTC (rev 5531)
> +++ trunk/hw/bt-hci.c   2008-10-25 11:23:27 UTC (rev 5532)
> @@ -1814,7 +1814,7 @@
>
>         if (hci->device.lmp_name)
>             free((void *) hci->device.lmp_name);
> -        hci->device.lmp_name = strndup(PARAM(change_local_name, name),
> +        hci->device.lmp_name = pstrdup(PARAM(change_local_name, name),
>                         sizeof(PARAM(change_local_name, name)));
>         bt_hci_event_complete_status(hci, HCI_SUCCESS);
>         break;
>
> Modified: trunk/qemu-common.h
> ===================================================================
> --- trunk/qemu-common.h 2008-10-25 11:21:28 UTC (rev 5531)
> +++ trunk/qemu-common.h 2008-10-25 11:23:27 UTC (rev 5532)
> @@ -82,6 +82,7 @@
>  /* cutils.c */
>  void pstrcpy(char *buf, int buf_size, const char *str);
>  char *pstrcat(char *buf, int buf_size, const char *s);
> +char *pstrdup(const char *str, size_t buf_size);
>  int strstart(const char *str, const char *val, const char **ptr);
>  int stristart(const char *str, const char *val, const char **ptr);
>  time_t mktimegm(struct tm *tm);

I would like to revert this and part of the following commit so I can
merge the missing interface for bt and have it work.

Cheers

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

[Blue Swirl]

On 11/1/08, andrzej zaborowski <balrogg@gmail.com> wrote:
> 2008/10/25 Blue Swirl <blauwirbel@gmail.com>:
>
> > 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;
>  > +}
>  > +
>  >  int strstart(const char *str, const char *val, const char **ptr)
>  >  {
>  >     const char *p, *q;
>  >
>  > Modified: trunk/hw/bt-hci.c
>  > ===================================================================
>  > --- trunk/hw/bt-hci.c   2008-10-25 11:21:28 UTC (rev 5531)
>  > +++ trunk/hw/bt-hci.c   2008-10-25 11:23:27 UTC (rev 5532)
>  > @@ -1814,7 +1814,7 @@
>  >
>  >         if (hci->device.lmp_name)
>  >             free((void *) hci->device.lmp_name);
>  > -        hci->device.lmp_name = strndup(PARAM(change_local_name, name),
>  > +        hci->device.lmp_name = pstrdup(PARAM(change_local_name, name),
>  >                         sizeof(PARAM(change_local_name, name)));
>  >         bt_hci_event_complete_status(hci, HCI_SUCCESS);
>  >         break;
>  >
>  > Modified: trunk/qemu-common.h
>  > ===================================================================
>  > --- trunk/qemu-common.h 2008-10-25 11:21:28 UTC (rev 5531)
>  > +++ trunk/qemu-common.h 2008-10-25 11:23:27 UTC (rev 5532)
>  > @@ -82,6 +82,7 @@
>  >  /* cutils.c */
>  >  void pstrcpy(char *buf, int buf_size, const char *str);
>  >  char *pstrcat(char *buf, int buf_size, const char *s);
>  > +char *pstrdup(const char *str, size_t buf_size);
>  >  int strstart(const char *str, const char *val, const char **ptr);
>  >  int stristart(const char *str, const char *val, const char **ptr);
>  >  time_t mktimegm(struct tm *tm);
>
>
> I would like to revert this and part of the following commit so I can
>  merge the missing interface for bt and have it work.

Well, strndup is still a GNU extension, so it may not be present in
all systems. What if pstrdup was changed to match strndup, would that
be OK?

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

[andrzej zaborowski]

2008/11/1 Blue Swirl <blauwirbel@gmail.com>:
> On 11/1/08, andrzej zaborowski <balrogg@gmail.com> wrote:
>> 2008/10/25 Blue Swirl <blauwirbel@gmail.com>:
>>
>> > 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;
>>  > +}
>>  > +
>>  >  int strstart(const char *str, const char *val, const char **ptr)
>>  >  {
>>  >     const char *p, *q;
>>  >
>>  > Modified: trunk/hw/bt-hci.c
>>  > ===================================================================
>>  > --- trunk/hw/bt-hci.c   2008-10-25 11:21:28 UTC (rev 5531)
>>  > +++ trunk/hw/bt-hci.c   2008-10-25 11:23:27 UTC (rev 5532)
>>  > @@ -1814,7 +1814,7 @@
>>  >
>>  >         if (hci->device.lmp_name)
>>  >             free((void *) hci->device.lmp_name);
>>  > -        hci->device.lmp_name = strndup(PARAM(change_local_name, name),
>>  > +        hci->device.lmp_name = pstrdup(PARAM(change_local_name, name),
>>  >                         sizeof(PARAM(change_local_name, name)));
>>  >         bt_hci_event_complete_status(hci, HCI_SUCCESS);
>>  >         break;
>>  >
>>  > Modified: trunk/qemu-common.h
>>  > ===================================================================
>>  > --- trunk/qemu-common.h 2008-10-25 11:21:28 UTC (rev 5531)
>>  > +++ trunk/qemu-common.h 2008-10-25 11:23:27 UTC (rev 5532)
>>  > @@ -82,6 +82,7 @@
>>  >  /* cutils.c */
>>  >  void pstrcpy(char *buf, int buf_size, const char *str);
>>  >  char *pstrcat(char *buf, int buf_size, const char *s);
>>  > +char *pstrdup(const char *str, size_t buf_size);
>>  >  int strstart(const char *str, const char *val, const char **ptr);
>>  >  int stristart(const char *str, const char *val, const char **ptr);
>>  >  time_t mktimegm(struct tm *tm);
>>
>>
>> I would like to revert this and part of the following commit so I can
>>  merge the missing interface for bt and have it work.
>
> Well, strndup is still a GNU extension, so it may not be present in
> all systems. What if pstrdup was changed to match strndup, would that
> be OK?

There's still the strncpy change.

My idea was to just add strndup to osdep.c because if the semantics
are to match anyway then the rename only adds confusion.

Cheers