afsio: switch BreakUpPath to strdup
authorBenjamin Kaduk <kaduk@mit.edu>
Wed, 20 May 2015 14:57:53 +0000 (10:57 -0400)
committerDaria Brashear <shadow@your-file-system.com>
Tue, 26 May 2015 17:58:35 +0000 (13:58 -0400)
The current version of BreakUpPath is slightly broken, since
commit 4e68282e26b0c4569d25d076d54274f0da47a691 -- it has two
output parameters but takes only one length parameter for the
size of the output buffers passed in.  The callers ended up using
the shorter of the buffer lengths in question, so there is not
a risk of a buffer overrun, but long paths would not be properly
handled.

There is not really any need to pass in a length at all, since
what is going on is conceptually strdup, and there is no real
need to use strlcpy at all.  Make the change from strlcpy to
str(n)dup, and adjust callers to free the outputs as appropriate.

While here, convert writeFile() to use goto and a cleanup handler
to avoid leaks.

Change-Id: Ib742cb73a6d70aa863c8d30423416887b977677b
Reviewed-on: http://gerrit.openafs.org/11874
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Jeffrey Altman <jaltman@your-file-system.com>
Reviewed-by: Daria Brashear <shadow@your-file-system.com>

src/venus/afsio.c

index e32b08a..9e3b1e5 100644 (file)
@@ -75,7 +75,7 @@ static int CmdProlog(struct cmd_syndesc *, char **, char **,
 static int ScanFid(char *, struct AFSFid *);
 static afs_int32 GetVenusFidByFid(char *, char *, int, struct afscp_venusfid **);
 static afs_int32 GetVenusFidByPath(char *, char *, struct afscp_venusfid **);
-static int BreakUpPath(char *, char *, char *, size_t);
+static int BreakUpPath(char *, char **, char **);
 
 static char pnp[AFSPATHMAX];   /* filename of this program when called */
 static int verbose = 0;                /* Set if -verbose option given */
@@ -276,22 +276,23 @@ int
 main(int argc, char **argv)
 {
     struct cmd_syndesc *ts;
-    char baseName[AFSNAMEMAX];
+    char *baseName;
     int code;
 
     /* try to get only the base name of this executable for use in logs */
 #ifdef AFS_NT40_ENV
     char *p = strdup(argv[0]);
     ConvertAFSPath(&p);
-    code = BreakUpPath(p, NULL, baseName, AFSNAMEMAX);
+    code = BreakUpPath(p, NULL, &baseName);
     free(p);
 #else
-    code = BreakUpPath(argv[0], NULL, baseName, AFSNAMEMAX);
+    code = BreakUpPath(argv[0], NULL, &baseName);
 #endif
     if (code > 0)
        strlcpy(pnp, baseName, AFSNAMEMAX);
     else
        strlcpy(pnp, argv[0], AFSPATHMAX);
+    free(baseName);
 
 #ifdef AFS_PTHREAD_ENV
     opr_Verify(pthread_key_create(&uclient_key, NULL) == 0);
@@ -543,22 +544,25 @@ GetVenusFidByFid(char *fidString, char *cellName, int onlyRW,
  * Split a full path up into dirName and baseName components
  *
  * \param[in]  fullPath        can be absolute, relative, or local
- * \param[out] dirName         pointer to allocated char buffer or NULL
- * \param[out] baseName        pointer to allocated char buffer or NULL
+ * \param[out] dirName         pointer to output string or NULL
+ * \param[out] baseName        pointer to output string or NULL
  *
- * \post To the fulleset extent possible, the rightmost full path
- *       component will be copied into baseName and all other
- *       components into dirName (minus the trailing path separator).
- *       If either dirName or baseName are NULL, only the non-NULL
- *       pointer will be filled in (but both can't be null or it would
- *       be pointless) -- so the caller can retrieve, say, only baseName
- *       if desired.  The return code is the number of strings copied:
+ * \post A buffer of appropriate size will be allocated into the output
+ *       parameter baseName and the rightmost full path component of the
+ *       fullPath copied into it; likewise, the other components of the
+ *       fullPath (minus the trailing path separator) will be placed into
+ *       the dirName output, which is also allocated to be the appropriate
+ *       size.  If either dirName or baseName are NULL, only the non-NULL
+ *       pointer will be allocated and filled in (but both can't be null
+ *       or it would be pointless) -- so the caller can retrieve, say,
+ *       only baseName if desired.  The return code is the number of
+ *       strings allocated and copied:
  *       0 if neither dirName nor baseName could be filled in
  *       1 if either dirName or baseName were filled in
  *       2 if both dirName and baseName were filled in
  */
 static int
-BreakUpPath(char *fullPath, char *dirName, char *baseName, size_t baseNameSize)
+BreakUpPath(char *fullPath, char **dirName, char **baseName)
 {
     char *lastSlash;
     size_t dirNameLen = 0;
@@ -568,10 +572,15 @@ BreakUpPath(char *fullPath, char *dirName, char *baseName, size_t baseNameSize)
        return code;
     }
 
+    /* Track what we need to output and initialize output variables to NULL. */
     if (dirName == NULL)
        useDirName = 0;
+    else
+       *dirName = NULL;
     if (baseName == NULL)
        useBaseName = 0;
+    else
+       *baseName = NULL;
     if (!useBaseName && !useDirName) {
        /* would be pointless to continue -- must be error in call */
        return code;
@@ -581,19 +590,25 @@ BreakUpPath(char *fullPath, char *dirName, char *baseName, size_t baseNameSize)
        /* then lastSlash points to the last path separator in fullPath */
        if (useDirName) {
            dirNameLen = strlen(fullPath) - strlen(lastSlash);
-           strlcpy(dirName, fullPath, min(dirNameLen + 1, baseNameSize));
-           code++;
+           *dirName = strdup(fullPath);
+           if (*dirName != NULL) {
+               code++;
+               /* Wastes some memory, but avoids needing libroken. */
+               *dirName[dirNameLen] = '\0';
+           }
        }
        if (useBaseName) {
            lastSlash++;
-           strlcpy(baseName, lastSlash, min(strlen(lastSlash) + 1, baseNameSize));
-           code++;
+           *baseName = strdup(lastSlash);
+           if (*baseName != NULL)
+               code++;
        }
     } else {
        /* there are no path separators in fullPath -- it's just a baseName */
        if (useBaseName) {
-           strlcpy(baseName, fullPath, min(strlen(fullPath) + 1, baseNameSize));
-           code++;
+           *baseName = strdup(fullPath);
+           if (*baseName != NULL)
+               code++;
        }
     }
     return code;
@@ -855,13 +870,12 @@ writeFile(struct cmd_syndesc *as, void *unused)
     afs_int64 Pos;
     afs_int64 length, Len, synthlength = 0, offset = 0;
     afs_int64 bytes;
-    int worstCode = 0;
     int synthesize = 0;
     int overWrite = 0;
     struct wbuf *bufchain = 0;
     struct wbuf *previous, *tbuf;
-    char dirName[AFSPATHMAX];
-    char baseName[AFSNAMEMAX];
+    char *dirName = NULL;
+    char *baseName = NULL;
     char ipv4_addr[16];
 
 #ifdef AFS_NT40_ENV
@@ -885,7 +899,7 @@ writeFile(struct cmd_syndesc *as, void *unused)
        if (code != 0) {
            afs_com_err(pnp, code, "(invalid value for synthesize length %s)",
                        sSynthLen);
-           return code;
+           goto cleanup;
        }
        synthesize = 1;
     }
@@ -893,9 +907,8 @@ writeFile(struct cmd_syndesc *as, void *unused)
     if (useFid) {
        code = GetVenusFidByFid(fname, cell, 1, &newvfp);
        if (code != 0) {
-           afscp_FreeFid(newvfp);
            afs_com_err(pnp, code, "(GetVenusFidByFid returned code %d)", code);
-           return code;
+           goto cleanup;
        }
     } else {
        code = GetVenusFidByPath(fname, cell, &newvfp);
@@ -908,23 +921,21 @@ writeFile(struct cmd_syndesc *as, void *unused)
                 * appending to it unless user forces overwrite
                 */
                code = EEXIST;
-               afscp_FreeFid(newvfp);
                afs_com_err(pnp, code, "(use -force to overwrite)");
-               return code;
+               goto cleanup;
            }
        } else { /* file not found */
            if (append) {
                code = ENOENT;
                afs_com_err(pnp, code, "(cannot append to non-existent file)");
-               return code;
+               goto cleanup;
            }
        }
        if (!append && !overWrite) { /* must create a new file in this case */
-           if ( BreakUpPath(fname, dirName, baseName, AFSNAMEMAX) != 2 ) {
+           if ( BreakUpPath(fname, &dirName, &baseName) != 2 ) {
                code = EINVAL;
                afs_com_err(pnp, code, "(must provide full AFS path)");
-               afscp_FreeFid(newvfp);
-               return code;
+               goto cleanup;
            }
 
            code = GetVenusFidByPath(dirName, cell, &dirvfp);
@@ -932,7 +943,7 @@ writeFile(struct cmd_syndesc *as, void *unused)
            newvfp = NULL;
            if (code != 0) {
                afs_com_err(pnp, code, "(is dir %s in AFS?)", dirName);
-               return code;
+               goto cleanup;
            }
        }
     }
@@ -940,9 +951,7 @@ writeFile(struct cmd_syndesc *as, void *unused)
     if ( (newvfp != NULL) && (newvfp->fid.Vnode & 1) ) {
        code = EISDIR;
        afs_com_err(pnp, code, "(%s is a directory, not a file)", fname);
-       afscp_FreeFid(newvfp);
-       afscp_FreeFid(dirvfp);
-       return code;
+       goto cleanup;
     }
     gettimeofday(&starttime, &Timezone);
 
@@ -956,7 +965,7 @@ writeFile(struct cmd_syndesc *as, void *unused)
                        baseName, afs_printable_uint32_lu(dirvfp->fid.Volume),
                        afs_printable_uint32_lu(dirvfp->fid.Vnode),
                        afs_printable_uint32_lu(dirvfp->fid.Unique));
-           return code;
+           goto cleanup;
        }
     }
     code = afscp_GetStatus(newvfp, &OutStatus);
@@ -964,9 +973,7 @@ writeFile(struct cmd_syndesc *as, void *unused)
        afs_inet_ntoa_r(newvfp->cell->fsservers[0]->addrs[0], ipv4_addr);
        afs_com_err(pnp, code, "(failed to get status of file %s from"
                    "server %s, code = %d)", fname, ipv4_addr, code);
-       afscp_FreeFid(newvfp);
-       afscp_FreeFid(dirvfp);
-       return code;
+       goto cleanup;
     }
 
     if ( !append && !force &&
@@ -977,10 +984,8 @@ writeFile(struct cmd_syndesc *as, void *unused)
         * (covers fidwrite edge case)
         */
        code = EEXIST;
-       afscp_FreeFid(newvfp);
-       afscp_FreeFid(dirvfp);
        afs_com_err(pnp, code, "(use -force to overwrite)");
-       return code;
+       goto cleanup;
     }
 
     if (append) {
@@ -1007,10 +1012,8 @@ writeFile(struct cmd_syndesc *as, void *unused)
        if (tbuf == NULL) {
            if (!bufchain) {
                code = ENOMEM;
-               afscp_FreeFid(newvfp);
-               afscp_FreeFid(dirvfp);
                afs_com_err(pnp, code, "(cannot allocate buffer)");
-               return code;
+               goto cleanup;
            }
            break;
        }
@@ -1093,8 +1096,6 @@ writeFile(struct cmd_syndesc *as, void *unused)
            }
        }
     }
-    afscp_FreeFid(newvfp);
-    afscp_FreeFid(dirvfp);
 
     gettimeofday(&writetime, &Timezone);
     if (code) {
@@ -1112,5 +1113,10 @@ writeFile(struct cmd_syndesc *as, void *unused)
     if (md5sum)
        summarizeMD5(fname);
 
-    return worstCode;
+cleanup:
+    free(baseName);
+    free(dirName);
+    afscp_FreeFid(newvfp);
+    afscp_FreeFid(dirvfp);
+    return code;
 } /* writeFile */