From 7d70a63b947e9a654a4e3fffa0ffa355f5549ec7 Mon Sep 17 00:00:00 2001 From: Andrea Bolognani Date: Fri, 20 Jul 2018 13:00:44 +0200 Subject: [PATCH] util: Improve virStrncpy() implementation We finally get rid of the strncpy()-like semantics and implement our own, more sensible ones instead. As a bonus, this also fixes compilation on MinGW. Signed-off-by: Andrea Bolognani --- docs/hacking.html.in | 29 ++++++++++----------- src/util/virstring.c | 62 ++++++++++++++++++++++++++++++-------------- 2 files changed, 55 insertions(+), 36 deletions(-) diff --git a/docs/hacking.html.in b/docs/hacking.html.in index 6c1a5121a4..f99d143b7b 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -1121,22 +1121,22 @@

Do not use the strncpy function. According to the man page, it does not guarantee a NULL-terminated buffer, which makes - it extremely dangerous to use. Instead, use one of the - functionally equivalent functions: + it extremely dangerous to use. Instead, use one of the replacement + functions provided by libvirt: 

   virStrncpy(char *dest, const char *src, size_t n, size_t destbytes)

- The first three arguments have the same meaning as for strncpy; - namely the destination, source, and number of bytes to copy, - respectively. The last argument is the number of bytes - available in the destination string; if a copy of the source - string (including a \0) will not fit into the destination, no - bytes are copied and the routine returns <0. Otherwise, n - bytes from the source are copied into the destination and a - trailing \0 is appended. + The first two arguments have the same meaning as for strncpy, + namely the destination and source of the copy operation. Unlike + strncpy, the function will always copy exactly the number of bytes + requested and make sure the destination is NULL-terminated, as the + source is required to be; sanity checks are performed to ensure the + size of the destination, as specified by the last argument, is + sufficient for the operation to succeed. On success, 0 is returned; + on failure, a value <0 is returned instead. 

@@ -1144,10 +1144,8 @@

Use this variant if you know you want to copy the entire src - string into dest. Note that this is a macro, so arguments could - be evaluated more than once. This is equivalent to - virStrncpy(dest, src, strlen(src), destbytes) -

+ string into dest. + 

   virStrcpyStatic(char *dest, const char *src)
@@ -1157,8 +1155,7 @@
       string into dest and you know that your destination string is
       a static string (i.e. that sizeof(dest) returns something
       meaningful).  Note that this is a macro, so arguments could be
-      evaluated more than once.  This is equivalent to
-      virStrncpy(dest, src, strlen(src), sizeof(dest)).
+      evaluated more than once.
     

 
 
diff --git a/src/util/virstring.c b/src/util/virstring.c
index 3e2f85465f..93fda69d7f 100644
--- a/src/util/virstring.c
+++ b/src/util/virstring.c
@@ -769,44 +769,66 @@ virAsprintfInternal(bool report,
 }
 
 /**
- * virStrncpy
+ * virStrncpy:
  *
- * A safe version of strncpy.  The last parameter is the number of bytes
- * available in the destination string, *not* the number of bytes you want
- * to copy.  If the destination is not large enough to hold all n of the
- * src string bytes plus a \0, <0 is returned and no data is copied.
- * If the destination is large enough to hold the n bytes plus \0, then the
- * string is copied and 0 is returned.
+ * @dest: destination buffer
+ * @src: source buffer
+ * @n: number of bytes to copy
+ * @destbytes: number of bytes the destination can accomodate
+ *
+ * Copies the first @n bytes of @src to @dest.
+ *
+ * @src must be NULL-terminated; if successful, @dest is guaranteed to
+ * be NULL-terminated as well.
+ *
+ * @n must be a reasonable value, that is, it must not exceed either
+ * the length of @src or the size of @dest. For the latter constraint,
+ * the fact that @dest needs to accomodate a NULL byte in addition to
+ * the bytes copied from @src must be taken into account.
+ *
+ * If you want to copy *all* of @src to @dest, use virStrcpy() or
+ * virStrcpyStatic() instead.
+ *
+ * Returns: 0 on success, <0 on failure.
  */
 int
 virStrncpy(char *dest, const char *src, size_t n, size_t destbytes)
 {
-    if (n > (destbytes - 1))
+    size_t src_len = strlen(src);
+
+    /* As a special case, -1 means "copy the entire string".
+     *
+     * This is to avoid calling strlen() twice, once in the virStrcpy()
+     * wrapper and once here for bound checking purposes. */
+    if (n == -1)
+        n = src_len;
+
+    if (n <= 0 || n > src_len || n > (destbytes - 1))
         return -1;
 
-    strncpy(dest, src, n);
-    /* strncpy NULL terminates iff the last character is \0.  Therefore
-     * force the last byte to be \0
-     */
+    memcpy(dest, src, n);
     dest[n] = '\0';
 
     return 0;
 }
 
 /**
- * virStrcpy
+ * virStrcpy:
  *
- * A safe version of strcpy.  The last parameter is the number of bytes
- * available in the destination string, *not* the number of bytes you want
- * to copy.  If the destination is not large enough to hold all n of the
- * src string bytes plus a \0, <0 is returned and no data is copied.
- * If the destination is large enough to hold the source plus \0, then the
- * string is copied and 0 is returned.
+ * @dest: destination buffer
+ * @src: source buffer
+ * @destbytes: number of bytes the destination can accomodate
+ *
+ * Copies @src to @dest.
+ *
+ * See virStrncpy() for more information.
+ *
+ * Returns: 0 on success, <0 on failure.
  */
 int
 virStrcpy(char *dest, const char *src, size_t destbytes)
 {
-    return virStrncpy(dest, src, strlen(src), destbytes);
+    return virStrncpy(dest, src, -1, destbytes);
 }
 
 /**