patch 8.1.0470: pointer ownership around fname_expand() is unclear

brammool · brammool · commit 3d6014f0336d · 2018-10-11T19:27:47.000+02:00
Problem:    Pointer ownership around fname_expand() is unclear.
Solution:   Allow b_ffname and b_sfname to point to the same allocated memory,
            only free one.  Update comments.
diff --git a/src/buffer.c b/src/buffer.c
@@ -663,8 +663,11 @@ close_buffer(
 	    workshop_file_closed_lineno((char *)buf->b_ffname,
 			(int)buf->b_last_cursor.lnum);
 #endif
-	vim_free(buf->b_ffname);
-	vim_free(buf->b_sfname);
+	if (buf->b_sfname != buf->b_ffname)
+	    VIM_CLEAR(buf->b_sfname);
+	else
+	    buf->b_sfname = NULL;
+	VIM_CLEAR(buf->b_ffname);
 	if (buf->b_prev == NULL)
 	    firstbuf = buf->b_next;
 	else
@@ -1877,11 +1880,13 @@ curbuf_reusable(void)
  */
     buf_T *
 buflist_new(
-    char_u	*ffname,	/* full path of fname or relative */
-    char_u	*sfname,	/* short fname or NULL */
-    linenr_T	lnum,		/* preferred cursor line */
-    int		flags)		/* BLN_ defines */
+    char_u	*ffname_arg,	// full path of fname or relative
+    char_u	*sfname_arg,	// short fname or NULL
+    linenr_T	lnum,		// preferred cursor line
+    int		flags)		// BLN_ defines
 {
+    char_u	*ffname = ffname_arg;
+    char_u	*sfname = sfname_arg;
     buf_T	*buf;
 #ifdef UNIX
     stat_T	st;
@@ -1890,7 +1895,7 @@ buflist_new(
     if (top_file_num == 1)
 	hash_init(&buf_hashtab);
 
-    fname_expand(curbuf, &ffname, &sfname);	/* will allocate ffname */
+    fname_expand(curbuf, &ffname, &sfname);	// will allocate ffname
 
     /*
      * If file name already exists in the list, update the entry.
@@ -1997,8 +2002,11 @@ buflist_new(
     if ((ffname != NULL && (buf->b_ffname == NULL || buf->b_sfname == NULL))
 	    || buf->b_wininfo == NULL)
     {
+	if (buf->b_sfname != buf->b_ffname)
+	    VIM_CLEAR(buf->b_sfname);
+	else
+	    buf->b_sfname = NULL;
 	VIM_CLEAR(buf->b_ffname);
-	VIM_CLEAR(buf->b_sfname);
 	if (buf != curbuf)
 	    free_buffer(buf);
 	return NULL;
@@ -3103,18 +3111,21 @@ buflist_name_nr(
 }
 
 /*
- * Set the file name for "buf"' to 'ffname', short file name to 'sfname'.
+ * Set the file name for "buf"' to "ffname_arg", short file name to
+ * "sfname_arg".
  * The file name with the full path is also remembered, for when :cd is used.
  * Returns FAIL for failure (file name already in use by other buffer)
  *	OK otherwise.
  */
     int
 setfname(
     buf_T	*buf,
-    char_u	*ffname,
-    char_u	*sfname,
+    char_u	*ffname_arg,
+    char_u	*sfname_arg,
     int		message)	/* give message when buffer already exists */
 {
+    char_u	*ffname = ffname_arg;
+    char_u	*sfname = sfname_arg;
     buf_T	*obuf = NULL;
 #ifdef UNIX
     stat_T	st;
@@ -3123,8 +3134,11 @@ setfname(
     if (ffname == NULL || *ffname == NUL)
     {
 	/* Removing the name. */
+	if (buf->b_sfname != buf->b_ffname)
+	    VIM_CLEAR(buf->b_sfname);
+	else
+	    buf->b_sfname = NULL;
 	VIM_CLEAR(buf->b_ffname);
-	VIM_CLEAR(buf->b_sfname);
 #ifdef UNIX
 	st.st_dev = (dev_T)-1;
 #endif
@@ -3175,8 +3189,9 @@ setfname(
 # endif
 	    fname_case(sfname, 0);    /* set correct case for short file name */
 #endif
+	if (buf->b_sfname != buf->b_ffname)
+	    vim_free(buf->b_sfname);
 	vim_free(buf->b_ffname);
-	vim_free(buf->b_sfname);
 	buf->b_ffname = ffname;
 	buf->b_sfname = sfname;
     }
@@ -3210,7 +3225,8 @@ buf_set_name(int fnum, char_u *name)
     buf = buflist_findnr(fnum);
     if (buf != NULL)
     {
-	vim_free(buf->b_sfname);
+	if (buf->b_sfname != buf->b_ffname)
+	    vim_free(buf->b_sfname);
 	vim_free(buf->b_ffname);
 	buf->b_ffname = vim_strsave(name);
 	buf->b_sfname = NULL;
@@ -4820,27 +4836,31 @@ fix_fname(char_u  *fname)
 }
 
 /*
- * Make "ffname" a full file name, set "sfname" to "ffname" if not NULL.
- * "ffname" becomes a pointer to allocated memory (or NULL).
+ * Make "*ffname" a full file name, set "*sfname" to "*ffname" if not NULL.
+ * "*ffname" becomes a pointer to allocated memory (or NULL).
+ * When resolving a link both "*sfname" and "*ffname" will point to the same
+ * allocated memory.
+ * The "*ffname" and "*sfname" pointer values on call will not be freed.
+ * Note that the resulting "*ffname" pointer should be considered not allocaed.
  */
     void
 fname_expand(
     buf_T	*buf UNUSED,
     char_u	**ffname,
     char_u	**sfname)
 {
-    if (*ffname == NULL)	/* if no file name given, nothing to do */
+    if (*ffname == NULL)	    // no file name given, nothing to do
 	return;
-    if (*sfname == NULL)	/* if no short file name given, use ffname */
+    if (*sfname == NULL)	    // no short file name given, use ffname
 	*sfname = *ffname;
-    *ffname = fix_fname(*ffname);   /* expand to full path */
+    *ffname = fix_fname(*ffname);   // expand to full path
 
 #ifdef FEAT_SHORTCUT
     if (!buf->b_p_bin)
     {
 	char_u  *rfname;
 
-	/* If the file name is a shortcut file, use the file it links to. */
+	// If the file name is a shortcut file, use the file it links to.
 	rfname = mch_resolve_shortcut(*ffname);
 	if (rfname != NULL)
 	{
diff --git a/src/ex_cmds.c b/src/ex_cmds.c
@@ -3648,8 +3648,8 @@ check_readonly(int *forceit, buf_T *buf)
 }
 
 /*
- * Try to abandon current file and edit a new or existing file.
- * "fnum" is the number of the file, if zero use ffname/sfname.
+ * Try to abandon the current file and edit a new or existing file.
+ * "fnum" is the number of the file, if zero use "ffname_arg"/"sfname_arg".
  * "lnum" is the line number for the cursor in the new file (if non-zero).
  *
  * Return:
@@ -3661,12 +3661,14 @@ check_readonly(int *forceit, buf_T *buf)
     int
 getfile(
     int		fnum,
-    char_u	*ffname,
-    char_u	*sfname,
+    char_u	*ffname_arg,
+    char_u	*sfname_arg,
     int		setpm,
     linenr_T	lnum,
     int		forceit)
 {
+    char_u	*ffname = ffname_arg;
+    char_u	*sfname = sfname_arg;
     int		other;
     int		retval;
     char_u	*free_me = NULL;
diff --git a/src/fileio.c b/src/fileio.c
@@ -6187,7 +6187,8 @@ shorten_buf_fname(buf_T *buf, char_u *dirname, int force)
 		|| buf->b_sfname == NULL
 		|| mch_isFullName(buf->b_sfname)))
     {
-	VIM_CLEAR(buf->b_sfname);
+	if (buf->b_sfname != buf->b_ffname)
+	    VIM_CLEAR(buf->b_sfname);
 	p = shorten_fname(buf->b_ffname, dirname);
 	if (p != NULL)
 	{
diff --git a/src/structs.h b/src/structs.h
@@ -1972,9 +1972,11 @@ struct file_buffer
      * b_fname is the same as b_sfname, unless ":cd" has been done,
      *		then it is the same as b_ffname (NULL for no name).
      */
-    char_u	*b_ffname;	/* full path file name */
-    char_u	*b_sfname;	/* short file name */
-    char_u	*b_fname;	/* current file name */
+    char_u	*b_ffname;	// full path file name, allocated
+    char_u	*b_sfname;	// short file name, allocated, may be equal to
+				// b_ffname
+    char_u	*b_fname;	// current file name, points to b_ffname or
+				// b_sfname
 
 #ifdef UNIX
     int		b_dev_valid;	/* TRUE when b_dev has a valid number */
diff --git a/src/version.c b/src/version.c
@@ -792,6 +792,8 @@ static char *(features[]) =
 
 static int included_patches[] =
 {   /* Add new patch number below this line */
+/**/
+    470,
 /**/
     469,
 /**/

Original file line number	Diff line number	Diff line change
`@@ -6187,7 +6187,8 @@ shorten_buf_fname(buf_T buf, char_u dirname, int force)`
`6187`	`6187`	`\|\| buf->b_sfname == NULL`
`6188`	`6188`	`\|\| mch_isFullName(buf->b_sfname)))`
`6189`	`6189`	`{`
`6190`		`- VIM_CLEAR(buf->b_sfname);`
	`6190`	`+ if (buf->b_sfname != buf->b_ffname)`
	`6191`	`+ VIM_CLEAR(buf->b_sfname);`
`6191`	`6192`	`p = shorten_fname(buf->b_ffname, dirname);`
`6192`	`6193`	`if (p != NULL)`
`6193`	`6194`	`{`