[Qemu-devel] [5531] Replace uses of strncpy (a GNU extension) with Qemu pstrcpy

[Qemu-devel] [5531] Replace uses of strncpy (a GNU extension) with Qemu pstrcpy

[Blue Swirl]

Revision: 5531
          http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=5531
Author:   blueswir1
Date:     2008-10-25 11:21:28 +0000 (Sat, 25 Oct 2008)

Log Message:
-----------
Replace uses of strncpy (a GNU extension) with Qemu pstrcpy

Modified Paths:
--------------
    trunk/block-vmdk.c
    trunk/block-vvfat.c
    trunk/hw/bt-hci.c
    trunk/linux-user/syscall.c
    trunk/slirp/udp.c
    trunk/vl.c
    trunk/vnc.c

Modified: trunk/block-vmdk.c
===================================================================
--- trunk/block-vmdk.c	2008-10-25 11:19:14 UTC (rev 5530)
+++ trunk/block-vmdk.c	2008-10-25 11:21:28 UTC (rev 5531)
@@ -344,7 +344,7 @@
         if ((end_name - p_name) > sizeof (s->hd->backing_file) - 1)
             return -1;
 
-        strncpy(s->hd->backing_file, p_name, end_name - p_name);
+        pstrcpy(s->hd->backing_file, end_name - p_name, p_name);
         if (stat(s->hd->backing_file, &file_buf) != 0) {
             path_combine(parent_img_name, sizeof(parent_img_name),
                          filename, s->hd->backing_file);

Modified: trunk/block-vvfat.c
===================================================================
--- trunk/block-vvfat.c	2008-10-25 11:19:14 UTC (rev 5530)
+++ trunk/block-vvfat.c	2008-10-25 11:21:28 UTC (rev 5531)
@@ -625,7 +625,7 @@
 
     entry=array_get_next(&(s->directory));
     memset(entry->name,0x20,11);
-    strncpy((char*)entry->name,filename,i);
+    pstrcpy((char*)entry->name, i, filename);
 
     if(j > 0)
 	for (i = 0; i < 3 && filename[j+1+i]; i++)

Modified: trunk/hw/bt-hci.c
===================================================================
--- trunk/hw/bt-hci.c	2008-10-25 11:19:14 UTC (rev 5530)
+++ trunk/hw/bt-hci.c	2008-10-25 11:21:28 UTC (rev 5531)
@@ -1385,7 +1385,7 @@
     params.status = HCI_SUCCESS;
     memset(params.name, 0, sizeof(params.name));
     if (hci->device.lmp_name)
-        strncpy(params.name, hci->device.lmp_name, sizeof(params.name));
+        pstrcpy(params.name, sizeof(params.name), hci->device.lmp_name);
 
     bt_hci_event_complete(hci, &params, READ_LOCAL_NAME_RP_SIZE);
 }

Modified: trunk/linux-user/syscall.c
===================================================================
--- trunk/linux-user/syscall.c	2008-10-25 11:19:14 UTC (rev 5530)
+++ trunk/linux-user/syscall.c	2008-10-25 11:21:28 UTC (rev 5531)
@@ -4993,7 +4993,7 @@
 		    if (tnamelen > 256)
                         tnamelen = 256;
                     /* XXX: may not be correct */
-		    strncpy(tde->d_name, de->d_name, tnamelen);
+                    pstrcpy(tde->d_name, tnamelen, de->d_name);
                     de = (struct linux_dirent *)((char *)de + reclen);
                     len -= reclen;
                     tde = (struct target_dirent *)((char *)tde + treclen);

Modified: trunk/slirp/udp.c
===================================================================
--- trunk/slirp/udp.c	2008-10-25 11:19:14 UTC (rev 5530)
+++ trunk/slirp/udp.c	2008-10-25 11:21:28 UTC (rev 5531)
@@ -475,14 +475,14 @@
 			type = omsg->type;
 			OTOSIN(omsg, ctl_addr)->sin_port = addr.sin_port;
 			OTOSIN(omsg, ctl_addr)->sin_addr = our_addr;
-			strncpy(omsg->l_name, getlogin(), NAME_SIZE_OLD);
+                        pstrcpy(omsg->l_name, NAME_SIZE_OLD, getlogin());
 		} else {		/* new talk */
 			omsg = (CTL_MSG_OLD *) buff;
 			nmsg = mtod(m, CTL_MSG *);
 			type = nmsg->type;
 			OTOSIN(nmsg, ctl_addr)->sin_port = addr.sin_port;
 			OTOSIN(nmsg, ctl_addr)->sin_addr = our_addr;
-			strncpy(nmsg->l_name, getlogin(), NAME_SIZE_OLD);
+                        pstrcpy(nmsg->l_name, NAME_SIZE_OLD, getlogin());
 		}
 
 		if (type == LOOK_UP)

Modified: trunk/vl.c
===================================================================
--- trunk/vl.c	2008-10-25 11:19:14 UTC (rev 5530)
+++ trunk/vl.c	2008-10-25 11:21:28 UTC (rev 5531)
@@ -4446,7 +4446,7 @@
 	syslog(LOG_ERR, "Can't get flags\n");
 
     snprintf (actual_name, 32, "tap%d", ppa);
-    strncpy (ifr.lifr_name, actual_name, sizeof (ifr.lifr_name));
+    pstrcpy(ifr.lifr_name, sizeof(ifr.lifr_name), actual_name);
 
     ifr.lifr_ppa = ppa;
     /* Assign ppa according to the unit number returned by tun device */
@@ -4489,7 +4489,7 @@
     close (if_fd);
 
     memset(&ifr, 0x0, sizeof(ifr));
-    strncpy (ifr.lifr_name, actual_name, sizeof (ifr.lifr_name));
+    pstrcpy(ifr.lifr_name, sizeof(ifr.lifr_name), actual_name);
     ifr.lifr_ip_muxid  = ip_muxid;
     ifr.lifr_arp_muxid = arp_muxid;
 

Modified: trunk/vnc.c
===================================================================
--- trunk/vnc.c	2008-10-25 11:19:14 UTC (rev 5530)
+++ trunk/vnc.c	2008-10-25 11:21:28 UTC (rev 5531)
@@ -2337,7 +2337,8 @@
 	    if (start && (!end || (start < end))) {
 		int len = end ? end-(start+1) : strlen(start+1);
 		char *path = qemu_malloc(len+1);
-		strncpy(path, start+1, len);
+
+                pstrcpy(path, len, start + 1);
 		path[len] = '\0';
 		VNC_DEBUG("Trying certificate path '%s'\n", path);
 		if (vnc_set_x509_credential_dir(vs, path) < 0) {

Re: [Qemu-devel] [5531] Replace uses of strncpy (a GNU extension) with Qemu pstrcpy

[Blue Swirl]

On 10/25/08, Blue Swirl <blauwirbel@gmail.com> wrote:
> Revision: 5531
>           http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=5531
>  Author:   blueswir1
>  Date:     2008-10-25 11:21:28 +0000 (Sat, 25 Oct 2008)
>
>  Log Message:
>  -----------
>  Replace uses of strncpy (a GNU extension) with Qemu pstrcpy

Actually, strncpy is not a GNU extension.

Re: [Qemu-devel] [5531] Replace uses of strncpy (a GNU extension) with Qemu pstrcpy

[andrzej zaborowski]

2008/10/25 Blue Swirl <blauwirbel@gmail.com>:
> On 10/25/08, Blue Swirl <blauwirbel@gmail.com> wrote:
>> Revision: 5531
>>           http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=5531
>>  Author:   blueswir1
>>  Date:     2008-10-25 11:21:28 +0000 (Sat, 25 Oct 2008)
>>
>>  Log Message:
>>  -----------
>>  Replace uses of strncpy (a GNU extension) with Qemu pstrcpy
>
> Actually, strncpy is not a GNU extension.

It's also not equivalent with pstrcpy because the latter always
appends a \0, so in the bt-hci.c case the original code was more
correct (but it only matters if a device's LMP name is exactly 248
chars long, which would be rare)

Re: [Qemu-devel] [5531] Replace uses of strncpy (a GNU extension) with Qemu pstrcpy

[malc]

On Sat, 25 Oct 2008, Blue Swirl wrote:

> Revision: 5531
>          http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=5531
> Author:   blueswir1
> Date:     2008-10-25 11:21:28 +0000 (Sat, 25 Oct 2008)
>

[..snip..]

> Modified: trunk/block-vvfat.c
> ===================================================================
> --- trunk/block-vvfat.c	2008-10-25 11:19:14 UTC (rev 5530)
> +++ trunk/block-vvfat.c	2008-10-25 11:21:28 UTC (rev 5531)
> @@ -625,7 +625,7 @@
>
>     entry=array_get_next(&(s->directory));
>     memset(entry->name,0x20,11);
> -    strncpy((char*)entry->name,filename,i);
> +    pstrcpy((char*)entry->name, i, filename);
>
>     if(j > 0)
> 	for (i = 0; i < 3 && filename[j+1+i]; i++)

This is wrong and broke vvfat:

a. pstrcpy is abused, second argument should be the size of the output
    buffer, not the length of the second argument

b. even if replaced with pstrcpy(.., meta_sizeof(entry->name), ...)
    it would have been still wrong, since we must _i_ elements of
    entry->name and NOT try to zero terminate the output buffer

I think this was one of those rare cases when strncpy was used in
accordance with it's definition and not what programmer was imagining.

I think plain memcpy(...,filename,i) is the most warranted thing here.

[..snip..]

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [5531] Replace uses of strncpy (a GNU extension) with Qemu pstrcpy

[Blue Swirl]

On 10/25/08, malc <av1474@comtv.ru> wrote:
> On Sat, 25 Oct 2008, Blue Swirl wrote:
>
>
> > Revision: 5531
> >
> http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=5531
> > Author:   blueswir1
> > Date:     2008-10-25 11:21:28 +0000 (Sat, 25 Oct 2008)
> >
> >
>
>  [..snip..]
>
>
> > Modified: trunk/block-vvfat.c
> >
> ===================================================================
> > --- trunk/block-vvfat.c 2008-10-25 11:19:14 UTC (rev 5530)
> > +++ trunk/block-vvfat.c 2008-10-25 11:21:28 UTC (rev 5531)
> > @@ -625,7 +625,7 @@
> >
> >    entry=array_get_next(&(s->directory));
> >    memset(entry->name,0x20,11);
> > -    strncpy((char*)entry->name,filename,i);
> > +    pstrcpy((char*)entry->name, i, filename);
> >
> >    if(j > 0)
> >        for (i = 0; i < 3 && filename[j+1+i]; i++)
> >
>
>  This is wrong and broke vvfat:
>
>  a. pstrcpy is abused, second argument should be the size of the output
>    buffer, not the length of the second argument

It's actually not the raw length, but MAX(length, 8).

>  b. even if replaced with pstrcpy(.., meta_sizeof(entry->name), ...)
>    it would have been still wrong, since we must _i_ elements of
>    entry->name and NOT try to zero terminate the output buffer
>
>  I think this was one of those rare cases when strncpy was used in
>  accordance with it's definition and not what programmer was imagining.
>
>  I think plain memcpy(...,filename,i) is the most warranted thing here.

Yes, that should do the right thing.

I think the cutils.c needs some more commenting to document these corner cases.