Skip to content
Browse files

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.
  • Loading branch information...
Dridi committed May 13, 2019
1 parent 3e02788 commit 3a1fd9bb611927b47ee45979dcd6781e67bdfc0f
Showing with 18 additions and 13 deletions.
  1. +18 −13 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"

VIN_n_Arg(const char *n_arg, char **dir)
char nm[PATH_MAX];
char dn[PATH_MAX];
struct vsb vsb[1];
int i;


/* 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);
i = VSB_printf(vsb, "%s/%s/", VARNISH_STATE_DIR, nm);

if (i != 0) {
return (-1);
} else {
bprintf(dn, "%s/%s", VARNISH_STATE_DIR, nm);

strcat(dn, "/");

*dir = strdup(dn);
if (*dir == NULL)
return (-1);

if (dir != NULL) {
*dir = strdup(dn);
if (*dir == NULL)
return (-1);
return (0);

0 comments on commit 3a1fd9b

Please sign in to comment.
You can’t perform that action at this time.