[master] 3a1fd9bb6 Kill strcat and strcpy usage in VIN_n_Arg

Dridi Boukelmoune dridi.boukelmoune at gmail.com
Tue May 14 09:13:09 UTC 2019


commit 3a1fd9bb611927b47ee45979dcd6781e67bdfc0f
Author: Dridi Boukelmoune <dridi.boukelmoune at gmail.com>
Date:   Tue May 14 00:24:53 2019 +0200

    Kill strcat and strcpy usage in VIN_n_Arg
    
    If an absolute path is provided as n_arg with a length of exactly
    PATH_MAX-1 then the combination of strcpy and strcat for the trailing
    slash '/' overflows dn by one byte, writing its new null-terminating
    character '\0' right after dn's upper bound.
    
    By using a fixed-length VSB we can simply ensure that we stay within
    bounds at a reasonable cost. Guarding VSB operations should silence
    Flexelint as a nice side effect.
    
    VIN_n_Arg is not exposed outside of the source tree, and both callers
    today provide a valid dir argument, so we can now make it part of the
    contract with an assertion, simplifying the strdup error handling.

diff --git a/lib/libvarnish/vin.c b/lib/libvarnish/vin.c
index 6c098afb7..053e9ed36 100644
--- a/lib/libvarnish/vin.c
+++ b/lib/libvarnish/vin.c
@@ -39,15 +39,19 @@
 
 #include "vdef.h"
 
-#include "vas.h"	// XXX Flexelint "not used" - but req'ed for assert()
+#include "vas.h"
 #include "vin.h"
+#include "vsb.h"
 
 int
 VIN_n_Arg(const char *n_arg, char **dir)
 {
 	char nm[PATH_MAX];
 	char dn[PATH_MAX];
+	struct vsb vsb[1];
+	int i;
 
+	AN(dir);
 
 	/* First: determine the name */
 
@@ -61,25 +65,26 @@ VIN_n_Arg(const char *n_arg, char **dir)
 	} else
 		bprintf(nm, "%s", n_arg);
 
-
 	/* Second: find the directory name */
 
+	AN(VSB_new(vsb, dn, sizeof dn, VSB_FIXEDLEN));
+
 	if (*nm == '/')
-		strcpy(dn, nm);
-	else if (strlen(VARNISH_STATE_DIR) + 1 + strlen(nm) >= sizeof dn){
-		/* preliminary length check to avoid overflowing dm */
+		i = VSB_printf(vsb, "%s/", nm);
+	else
+		i = VSB_printf(vsb, "%s/%s/", VARNISH_STATE_DIR, nm);
+
+	if (i != 0) {
 		errno = ENAMETOOLONG;
 		return (-1);
-	} else {
-		bprintf(dn, "%s/%s", VARNISH_STATE_DIR, nm);
 	}
 
-	strcat(dn, "/");
+	AZ(VSB_finish(vsb));
+	VSB_clear(vsb);
+
+	*dir = strdup(dn);
+	if (*dir == NULL)
+		return (-1);
 
-	if (dir != NULL) {
-		*dir = strdup(dn);
-		if (*dir == NULL)
-			return (-1);
-	}
 	return (0);
 }


More information about the varnish-commit mailing list