From b9bdd849f076302550221ff195e83e31dd7bddfc Mon Sep 17 00:00:00 2001 From: James Simmons Date: Fri, 12 Jan 2018 14:05:52 -0500 Subject: [PATCH] LU-9833 utils: resolve buffer over runs in lustre_rsync Newer version of gcc will report of snprintf is used in an incorrect way. For the case of the lustre_rsync application many times two buffers of size PATH_MAX are being placed into one buffer of the size PATH_MAX. This can easily lead to a buffer overrun. This patch resolves those bugs. Test-Parameters: trivial testlist=lustre-rsync-test Change-Id: I035b4a3b1d9695a16822649c2165e492e9f2879d Signed-off-by: James Simmons Reviewed-on: https://review.whamcloud.com/30373 Tested-by: Jenkins Reviewed-by: John L. Hammond Tested-by: Maloo Reviewed-by: Dmitry Eremin Reviewed-by: Oleg Drokin --- lustre/utils/lustre_rsync.c | 137 +++++++++++++++++++++++++------------------- 1 file changed, 78 insertions(+), 59 deletions(-) diff --git a/lustre/utils/lustre_rsync.c b/lustre/utils/lustre_rsync.c index a2f7002..2e23ccb 100644 --- a/lustre/utils/lustre_rsync.c +++ b/lustre/utils/lustre_rsync.c @@ -144,9 +144,6 @@ #define DINFO 1 #define DTRACE 2 -/* Not used; declared for fulfilling obd.c's dependency. */ -command_t cmdlist[0]; - /* Information for processing a changelog record. This structure is allocated on the heap instead of allocating large variables on the stack. */ @@ -161,13 +158,13 @@ struct lr_info { char spfid[LR_FID_STR_LEN]; char sname[NAME_MAX + 1]; char name[NAME_MAX + 1]; - char src[PATH_MAX + 1]; - char dest[PATH_MAX + 1]; + char src[3 * PATH_MAX + 1]; + char dest[3 * PATH_MAX + 1]; char path[PATH_MAX + 1]; char savedpath[PATH_MAX + 1]; char link[PATH_MAX + 1]; char linktmp[PATH_MAX + 1]; - char cmd[PATH_MAX]; + char cmd[PATH_MAX * 10]; int bufsize; char *buf; @@ -283,9 +280,8 @@ void * lr_grow_buf(void *buf, int size) /* Use rsync to replicate file data */ int lr_rsync_data(struct lr_info *info) { - int rc; - struct stat st_src, st_dest; - char cmd[PATH_MAX]; + struct stat st_src, st_dest; + int rc; lr_debug(DTRACE, "Syncing data%s\n", info->tfid); @@ -314,10 +310,14 @@ int lr_rsync_data(struct lr_info *info) * replication is also to be considered. */ int status; - snprintf(cmd, PATH_MAX, "%s --inplace %s %s", rsync, info->src, - info->dest); - lr_debug(DTRACE, "\t%s %s\n", cmd, info->tfid); - status = system(cmd); + + if (snprintf(info->cmd, sizeof(info->cmd), "%s --inplace %s %s", + rsync, info->src, info->dest) >= sizeof(info->cmd)) { + rc = -E2BIG; + goto err; + } + lr_debug(DTRACE, "\t%s %s\n", info->cmd, info->tfid); + status = system(info->cmd); if (status == -1) { rc = -errno; } else if (WIFEXITED(status)) { @@ -340,7 +340,7 @@ int lr_rsync_data(struct lr_info *info) lr_debug(DTRACE, "Not syncing %s and %s %s\n", info->src, info->dest, info->tfid); } - +err: return rc; } @@ -573,7 +573,7 @@ int lr_get_symlink(struct lr_info *info) strlen(status->ls_source)) == 0) { /* Strip source fs path and replace with target fs path. */ link = info->linktmp + strlen(status->ls_source); - snprintf(info->src, PATH_MAX, "%s%s", + snprintf(info->src, sizeof(info->src), "%s%s", status->ls_targets[info->target_no], link); link = info->src; } else { @@ -670,18 +670,22 @@ out_err: void lr_cascade_move(const char *fid, const char *dest, struct lr_info *info) { struct lr_parent_child_list *curr, *prev; - char *d; + char d[4 * PATH_MAX + 1]; int rc; - d = calloc(1, PATH_MAX + 1); prev = curr = parents; while (curr) { if (strcmp(curr->pc_log.pcl_pfid, fid) == 0) { - snprintf(d, PATH_MAX, "%s/%s", dest, - curr->pc_log.pcl_name); - snprintf(info->src, PATH_MAX, "%s/%s/%s", - status->ls_targets[info->target_no], - SPECIAL_DIR, curr->pc_log.pcl_tfid); + if (snprintf(d, sizeof(d), "%s/%s", dest, + curr->pc_log.pcl_name) >= sizeof(d)) { + fprintf(stderr, "Buffer truncated\n"); + return; + } + if (snprintf(info->src, sizeof(info->src), "%s/%s/%s", + status->ls_targets[info->target_no], + SPECIAL_DIR, curr->pc_log.pcl_tfid) >= + sizeof(info->src)) + return; rc = rename(info->src, d); if (rc == -1) { fprintf(stderr, "Error renaming file " @@ -702,8 +706,6 @@ void lr_cascade_move(const char *fid, const char *dest, struct lr_info *info) curr = curr->pc_next; } } - - free(d); } /* remove [info->spfid, info->sfid] from parents */ @@ -730,7 +732,7 @@ int lr_mk_special(struct lr_info *info) { int rc; - snprintf(info->dest, PATH_MAX, "%s/%s/%s", + snprintf(info->dest, sizeof(info->dest), "%s/%s/%s", status->ls_targets[info->target_no], SPECIAL_DIR, info->tfid); @@ -759,10 +761,11 @@ int lr_rmfile(struct lr_info *info) /* Recursively remove directory and its contents */ int lr_rm_recursive(struct lr_info *info) { - int rc; + int rc; - snprintf(info->cmd, PATH_MAX, "rm -rf %s", info->dest); - rc = system(info->cmd); + snprintf(info->cmd, sizeof(info->cmd), "rm -rf %s", + info->dest); + rc = system(info->cmd); if (rc == -1) rc = -errno; @@ -774,7 +777,7 @@ int lr_rm_special(struct lr_info *info) { int rc; - snprintf(info->dest, PATH_MAX, "%s/%s/%s", + snprintf(info->dest, sizeof(info->dest), "%s/%s/%s", status->ls_targets[info->target_no], SPECIAL_DIR, info->tfid); rc = lr_rmfile(info); @@ -818,11 +821,13 @@ int lr_create(struct lr_info *info) /* Is f2p(pfid)+name != f2p(tfid)? If not the file has moved. */ len = strlen(info->path); if (len == 1 && info->path[0] == '/') - snprintf(info->dest, PATH_MAX, "%s", info->name); + snprintf(info->dest, sizeof(info->dest), "%s", info->name); else if (len - 1 > 0 && info->path[len - 1] == '/') - snprintf(info->dest, PATH_MAX, "%s%s", info->path, info->name); + snprintf(info->dest, sizeof(info->dest), "%s%s", info->path, + info->name); else - snprintf(info->dest, PATH_MAX, "%s/%s", info->path, info->name); + snprintf(info->dest, sizeof(info->dest), "%s/%s", info->path, + info->name); lr_debug(DTRACE, "dest = %s; savedpath = %s\n", info->dest, info->savedpath); @@ -835,8 +840,8 @@ int lr_create(struct lr_info *info) /* Is f2p(pfid) present on the target? If not, the parent has moved */ if (!mkspecial) { - snprintf(info->dest, PATH_MAX, "%s/%s", status->ls_targets[0], - info->path); + snprintf(info->dest, sizeof(info->dest), "%s/%s", + status->ls_targets[0], info->path); if (access(info->dest, F_OK) != 0) { lr_debug(DTRACE, "create: parent %s not found\n", info->dest); @@ -845,10 +850,10 @@ int lr_create(struct lr_info *info) } for (info->target_no = 0; info->target_no < status->ls_num_targets; info->target_no++) { - snprintf(info->dest, PATH_MAX, "%s/%s", + snprintf(info->dest, sizeof(info->dest), "%s/%s", status->ls_targets[info->target_no], info->savedpath); lr_get_FID_PATH(status->ls_source, info->tfid, info->src, - PATH_MAX); + PATH_MAX); if (!mkspecial) rc1 = lr_mkfile(info); @@ -884,7 +889,7 @@ int lr_remove(struct lr_info *info) rc = rc1; continue; } - snprintf(info->dest, PATH_MAX, "%s/%s/%s", + snprintf(info->dest, sizeof(info->dest), "%s/%s/%s", status->ls_targets[info->target_no], info->path, info->name); @@ -927,21 +932,22 @@ int lr_move(struct lr_info *info) info->target_no++) { if (!rc_dest) { - snprintf(info->dest, PATH_MAX, "%s/%s", + snprintf(info->dest, sizeof(info->dest), "%s/%s", status->ls_targets[info->target_no], info->path); if (access(info->dest, F_OK) != 0) { rc_dest = -errno; } else { - snprintf(info->dest, PATH_MAX, "%s/%s/%s", - status->ls_targets[info->target_no], - info->path, info->name); + snprintf(info->dest, sizeof(info->dest), + "%s/%s/%s", + status->ls_targets[info->target_no], + info->path, info->name); } lr_debug(DINFO, "dest path %s rc_dest=%d\n", info->dest, rc_dest); } if (rc_dest == -ENOENT) { - snprintf(info->dest, PATH_MAX, "%s/%s/%s", + snprintf(info->dest, sizeof(info->dest), "%s/%s/%s", status->ls_targets[info->target_no], SPECIAL_DIR, info->sfid); special_dest = 1; @@ -949,7 +955,7 @@ int lr_move(struct lr_info *info) } if (!rc_src) { - snprintf(info->src, PATH_MAX, "%s/%s/%s", + snprintf(info->src, sizeof(info->src), "%s/%s/%s", status->ls_targets[info->target_no], srcpath, info->sname); lr_debug(DINFO, "src path %s rc_src=%d\n", info->src, @@ -957,7 +963,7 @@ int lr_move(struct lr_info *info) } if (rc_src == -ENOENT || (access(info->src, F_OK) != 0 && errno == ENOENT)) { - snprintf(info->src, PATH_MAX, "%s/%s/%s", + snprintf(info->src, sizeof(info->src), "%s/%s/%s", status->ls_targets[info->target_no], SPECIAL_DIR, info->sfid); special_src = 1; @@ -1028,6 +1034,8 @@ int lr_link(struct lr_info *info) /* Search through the hardlinks to get the src */ for (i = 0; i < st.st_nlink && info->src[0] == 0; i++) { + size_t len; + rc1 = lr_get_path_ln(info, info->tfid, i); lr_debug(rc1 ? 0:DTRACE, "\tfid2path %s, %s, %d rc=%d\n", info->path, info->name, i, rc1); @@ -1038,7 +1046,9 @@ int lr_link(struct lr_info *info) * Compare the path of target FID with info->dest * to find out info->src. */ - char srcpath[PATH_MAX]; + len = sizeof(status->ls_targets[info->target_no]) + + sizeof(info->path); + char srcpath[len + 1]; snprintf(srcpath, sizeof(srcpath), "%s/%s", status->ls_targets[info->target_no], @@ -1057,11 +1067,11 @@ int lr_link(struct lr_info *info) } if (info->src[0] == 0) - snprintf(info->src, PATH_MAX, "%s/%s/%s", + snprintf(info->src, sizeof(info->src), "%s/%s/%s", status->ls_targets[info->target_no], SPECIAL_DIR, info->tfid); else if (info->dest[0] == 0) - snprintf(info->dest, PATH_MAX, "%s/%s/%s", + snprintf(info->dest, sizeof(info->dest), "%s/%s/%s", status->ls_targets[info->target_no], SPECIAL_DIR, info->tfid); @@ -1094,7 +1104,7 @@ int lr_setattr(struct lr_info *info) for (info->target_no = 0; info->target_no < status->ls_num_targets; info->target_no++) { - snprintf(info->dest, PATH_MAX, "%s/%s", + snprintf(info->dest, sizeof(info->dest), "%s/%s", status->ls_targets[info->target_no], info->path); lr_debug(DINFO, "setattr: %s %s %s", info->src, info->dest, info->tfid); @@ -1125,7 +1135,7 @@ int lr_setxattr(struct lr_info *info) for (info->target_no = 0; info->target_no < status->ls_num_targets; info->target_no++) { - snprintf(info->dest, PATH_MAX, "%s/%s", + snprintf(info->dest, sizeof(info->dest), "%s/%s", status->ls_targets[info->target_no], info->path); lr_debug(DINFO, "setxattr: %s %s %s\n", info->src, info->dest, info->tfid); @@ -1212,7 +1222,7 @@ void lr_backup_log() if (logbackedup) return; - snprintf(backupfile, PATH_MAX, "%s.old", statuslog); + snprintf(backupfile, sizeof(backupfile), "%s.old", statuslog); (void) rename(statuslog, backupfile); logbackedup = 1; @@ -1415,12 +1425,12 @@ int lr_locate_rsync() int len; /* Locate rsync */ - snprintf(rsync, PATH_MAX, "%s -p %s", TYPE, RSYNC); + snprintf(rsync, sizeof(rsync), "%s -p %s", TYPE, RSYNC); fp = popen(rsync, "r"); if (fp == NULL) return -1; - if (fgets(rsync, PATH_MAX, fp) == NULL) { + if (fgets(rsync, sizeof(rsync), fp) == NULL) { fclose(fp); return -1; } @@ -1431,12 +1441,12 @@ int lr_locate_rsync() fclose(fp); /* Determine the version of rsync */ - snprintf(rsync_ver, PATH_MAX, "%s --version", rsync); + snprintf(rsync_ver, sizeof(rsync_ver), "%s --version", rsync); fp = popen(rsync_ver, "r"); if (fp == NULL) return -1; - if (fgets(rsync_ver, PATH_MAX, fp) == NULL) { + if (fgets(rsync_ver, sizeof(rsync_ver), fp) == NULL) { fclose(fp); return -1; } @@ -1506,9 +1516,18 @@ int lr_replicate() "mountpoint.\n"); goto out; } - if (status->ls_mdt_device[0] == '\0') - snprintf(status->ls_mdt_device, LR_NAME_MAXLEN, "%s%s", - status->ls_source_fs, DEFAULT_MDT); + + if (status->ls_mdt_device[0] == '\0') { + int len; + + len = snprintf(status->ls_mdt_device, + sizeof(status->ls_mdt_device), "%s%s", + status->ls_source_fs, DEFAULT_MDT); + if (len >= sizeof(status->ls_mdt_device)) { + rc = -E2BIG; + goto out; + } + } ext = calloc(1, sizeof(struct lr_info)); if (ext == NULL) { @@ -1517,8 +1536,8 @@ int lr_replicate() } for (i = 0, xattr_not_supp = 0; i < status->ls_num_targets; i++) { - snprintf(info->dest, PATH_MAX, "%s/%s", status->ls_targets[i], - SPECIAL_DIR); + snprintf(info->dest, sizeof(info->dest), "%s/%s", + status->ls_targets[i], SPECIAL_DIR); rc = mkdir(info->dest, 0777); if (rc == -1 && errno != EEXIST) { fprintf(stderr, "Error writing to target path %s.\n", -- 1.8.3.1