From b8648dbefb3968329d20cad8976ce15947428678 Mon Sep 17 00:00:00 2001 From: Benjamin Kaduk Date: Wed, 20 May 2015 10:57:53 -0400 Subject: [PATCH] afsio: switch BreakUpPath to strdup 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 Reviewed-by: Jeffrey Altman Reviewed-by: Daria Brashear --- src/venus/afsio.c | 104 +++++++++++++++++++++++++++++------------------------- 1 file changed, 55 insertions(+), 49 deletions(-) diff --git a/src/venus/afsio.c b/src/venus/afsio.c index e32b08a..9e3b1e5 100644 --- a/src/venus/afsio.c +++ b/src/venus/afsio.c @@ -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 */ -- 1.9.4