strcpy has a risk of buffer overflow, and strncpy may not set '\0' to the destination buffer. This patch introduces pstrcpy to copy strings safely.
Signed-off-by: MORITA Kazutaka <morita.kazut...@lab.ntt.co.jp> --- collie/cluster.c | 7 ++++--- collie/treeview.c | 6 +++--- collie/vdi.c | 26 +++++++++++++------------- include/util.h | 1 + lib/logger.c | 8 ++++---- lib/util.c | 27 +++++++++++++++++++++++++++ sheep/config.c | 2 +- sheep/group.c | 2 +- sheep/journal.c | 4 ++-- sheep/vdi.c | 2 +- 10 files changed, 57 insertions(+), 28 deletions(-) diff --git a/collie/cluster.c b/collie/cluster.c index 385ffff..065d2ac 100644 --- a/collie/cluster.c +++ b/collie/cluster.c @@ -97,9 +97,9 @@ static int cluster_format(int argc, char **argv) hdr.ctime = (uint64_t) tv.tv_sec << 32 | tv.tv_usec * 1000; if (strlen(cluster_cmd_data.name)) - strncpy(store_name, cluster_cmd_data.name, STORE_LEN); + pstrcpy(store_name, STORE_LEN, cluster_cmd_data.name); else - strcpy(store_name, DEFAULT_STORE); + pstrcpy(store_name, STORE_LEN, DEFAULT_STORE); hdr.data_length = strlen(store_name) + 1; hdr.flags |= SD_FLAG_CMD_WRITE; @@ -436,7 +436,8 @@ static int cluster_parser(int ch, char *opt) switch (ch) { case 'b': - strncpy(cluster_cmd_data.name, opt, 10); + pstrcpy(cluster_cmd_data.name, sizeof(cluster_cmd_data.name), + opt); break; case 'c': copies = strtol(opt, &p, 10); diff --git a/collie/treeview.c b/collie/treeview.c index 1938321..21b4cc1 100644 --- a/collie/treeview.c +++ b/collie/treeview.c @@ -13,7 +13,7 @@ #include <string.h> #include <stdint.h> -#include "list.h" +#include "util.h" #include "treeview.h" #ifndef MAX_DEPTH @@ -62,8 +62,8 @@ static struct vdi_tree *new_vdi(const char *name, const char *label, fprintf(stderr, "Failed to allocate memory\n"); return NULL; } - strcpy(vdi->name, name); - strcpy(vdi->label, label); + pstrcpy(vdi->name, sizeof(vdi->name), name); + pstrcpy(vdi->label, sizeof(vdi->label), label); vdi->vid = vid; vdi->pvid = pvid; vdi->highlight = highlight; diff --git a/collie/vdi.c b/collie/vdi.c index 3734dfb..e4ea64c 100644 --- a/collie/vdi.c +++ b/collie/vdi.c @@ -151,7 +151,7 @@ static void print_vdi_tree(uint32_t vid, const char *name, const char *tag, char buf[128]; if (is_current(i)) - strcpy(buf, "(you are here)"); + pstrcpy(buf, sizeof(buf), "(you are here)"); else { ti = i->create_time >> 32; localtime_r(&ti, &tm); @@ -390,8 +390,8 @@ static int find_vdi_name(const char *vdiname, uint32_t snapid, const char *tag, return -1; memset(buf, 0, sizeof(buf)); - strncpy(buf, vdiname, SD_MAX_VDI_LEN); - strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN); + pstrcpy(buf, SD_MAX_VDI_LEN, vdiname); + pstrcpy(buf + SD_MAX_VDI_LEN, SD_MAX_VDI_TAG_LEN, tag); if (for_snapshot) sd_init_req(&hdr, SD_OP_GET_VDI_INFO); @@ -470,7 +470,7 @@ static int do_vdi_create(const char *vdiname, int64_t vdi_size, } memset(buf, 0, sizeof(buf)); - strncpy(buf, vdiname, SD_MAX_VDI_LEN); + pstrcpy(buf, SD_MAX_VDI_LEN, vdiname); sd_init_req(&hdr, SD_OP_NEW_VDI); hdr.flags = SD_FLAG_CMD_WRITE; @@ -738,9 +738,9 @@ static int do_vdi_delete(const char *vdiname, int snap_id, const char *snap_tag) hdr.data_length = sizeof(data); hdr.vdi.snapid = snap_id; memset(data, 0, sizeof(data)); - strncpy(data, vdiname, SD_MAX_VDI_LEN); + pstrcpy(data, SD_MAX_VDI_LEN, vdiname); if (snap_tag) - strncpy(data + SD_MAX_VDI_LEN, snap_tag, SD_MAX_VDI_TAG_LEN); + pstrcpy(data + SD_MAX_VDI_LEN, SD_MAX_VDI_TAG_LEN, snap_tag); ret = exec_req(fd, &hdr, data); close(fd); @@ -985,10 +985,10 @@ static int find_vdi_attr_oid(const char *vdiname, const char *tag, uint32_t snap struct sheepdog_vdi_attr vattr; memset(&vattr, 0, sizeof(vattr)); - strncpy(vattr.name, vdiname, SD_MAX_VDI_LEN); - strncpy(vattr.tag, vdi_cmd_data.snapshot_tag, SD_MAX_VDI_TAG_LEN); + pstrcpy(vattr.name, SD_MAX_VDI_LEN, vdiname); + pstrcpy(vattr.tag, SD_MAX_VDI_TAG_LEN, vdi_cmd_data.snapshot_tag); vattr.snap_id = vdi_cmd_data.snapshot_id; - strncpy(vattr.key, key, SD_MAX_VDI_ATTR_KEY_LEN); + pstrcpy(vattr.key, SD_MAX_VDI_ATTR_KEY_LEN, key); if (value && value_len) { vattr.value_len = value_len; memcpy(vattr.value, value, value_len); @@ -1917,8 +1917,8 @@ static int vdi_parser(int ch, char *opt) vdi_cmd_data.snapshot_id = strtol(opt, &p, 10); if (opt == p) { vdi_cmd_data.snapshot_id = 0; - strncpy(vdi_cmd_data.snapshot_tag, opt, - sizeof(vdi_cmd_data.snapshot_tag)); + pstrcpy(vdi_cmd_data.snapshot_tag, + sizeof(vdi_cmd_data.snapshot_tag), opt); } break; case 'x': @@ -1942,8 +1942,8 @@ static int vdi_parser(int ch, char *opt) vdi_cmd_data.from_snapshot_id = strtol(opt, &p, 10); if (opt == p) { vdi_cmd_data.from_snapshot_id = 0; - strncpy(vdi_cmd_data.from_snapshot_tag, opt, - sizeof(vdi_cmd_data.from_snapshot_tag)); + pstrcpy(vdi_cmd_data.from_snapshot_tag, + sizeof(vdi_cmd_data.from_snapshot_tag), opt); } } diff --git a/include/util.h b/include/util.h index d0bf659..afab903 100644 --- a/include/util.h +++ b/include/util.h @@ -78,6 +78,7 @@ extern ssize_t xread(int fd, void *buf, size_t len); extern ssize_t xwrite(int fd, const void *buf, size_t len); extern ssize_t xpread(int fd, void *buf, size_t count, off_t offset); extern ssize_t xpwrite(int fd, const void *buf, size_t count, off_t offset); +extern void pstrcpy(char *buf, int buf_size, const char *str); extern int rmdir_r(char *dir_path); void trim_zero_sectors(void *buf, uint64_t *offset, uint32_t *len); diff --git a/lib/logger.c b/lib/logger.c index aa1c012..3979bee 100644 --- a/lib/logger.c +++ b/lib/logger.c @@ -161,7 +161,7 @@ static notrace int log_vsnprintf(char *buff, size_t size, int prio, else if (worker_name) snprintf(p, size, "[%s] ", worker_name); else - strncpy(p, "[main] ", size); + pstrcpy(p, size, "[main] "); p += strlen(p); snprintf(p, size - strlen(buff), "%s(%d) ", func, line); @@ -186,7 +186,7 @@ static notrace void log_syslog(const struct logmsg *msg) localtime_r(&msg->t, &tm); len = strftime(str, sizeof(str), "%b %2d %H:%M:%S ", &tm); - strncpy(str + len, msg->str, sizeof(str) - len); + pstrcpy(str + len, sizeof(str) - len, msg->str); if (log_fd >= 0) xwrite(log_fd, str, strlen(str)); @@ -412,8 +412,8 @@ notrace int log_init(const char *program_name, int size, bool to_stdout, log_name = program_name; log_nowname = outfile; - strcpy(tmp, outfile); - strcpy(log_dir, dirname(tmp)); + pstrcpy(tmp, sizeof(tmp), outfile); + pstrcpy(log_dir, sizeof(log_dir), dirname(tmp)); semkey = random(); diff --git a/lib/util.c b/lib/util.c index 3b3870d..ea00afd 100644 --- a/lib/util.c +++ b/lib/util.c @@ -216,6 +216,33 @@ ssize_t xpwrite(int fd, const void *buf, size_t count, off_t offset) return total; } +/** + * Copy the string str to buf. If str length is bigger than buf_size - + * 1 then it is clamped to buf_size - 1. + * NOTE: this function does what strncpy should have done to be + * useful. NEVER use strncpy. + * + * @param buf destination buffer + * @param buf_size size of destination buffer + * @param str source string + */ +void pstrcpy(char *buf, int buf_size, const char *str) +{ + int c; + char *q = buf; + + if (buf_size <= 0) + return; + + while (true) { + c = *str++; + if (c == 0 || q >= buf + buf_size - 1) + break; + *q++ = c; + } + *q = '\0'; +} + /* remove directory recursively */ int rmdir_r(char *dir_path) { diff --git a/sheep/config.c b/sheep/config.c index e38bea0..c8f806a 100644 --- a/sheep/config.c +++ b/sheep/config.c @@ -169,7 +169,7 @@ int get_cluster_flags(uint16_t *flags) int set_cluster_store(const char *name) { memset(config.store, 0, sizeof(config.store)); - strcpy((char *)config.store, name); + pstrcpy((char *)config.store, sizeof(config.store), name); return write_config(); } diff --git a/sheep/group.c b/sheep/group.c index e9feaf6..05e4f0a 100644 --- a/sheep/group.c +++ b/sheep/group.c @@ -981,7 +981,7 @@ enum cluster_join_result sd_check_join_cb(const struct sd_node *joining, jm->disable_recovery = sys->disable_recovery; if (sd_store) - strcpy((char *)jm->store, sd_store->name); + pstrcpy((char *)jm->store, sizeof(jm->store), sd_store->name); if (jm->cluster_status != SD_STATUS_OK && (ret == CJ_RES_SUCCESS || ret == CJ_RES_JOIN_LATER)) diff --git a/sheep/journal.c b/sheep/journal.c index b6beabc..8c115c1 100644 --- a/sheep/journal.c +++ b/sheep/journal.c @@ -39,7 +39,7 @@ struct jrnl_descriptor { /* Journal APIs */ static int jrnl_open(struct jrnl_descriptor *jd, const char *path) { - strcpy(jd->path, path); + pstrcpy(jd->path, sizeof(jd->path), path); jd->fd = open(path, O_RDONLY); if (jd->fd < 0) { @@ -180,7 +180,7 @@ struct jrnl_descriptor *jrnl_begin(const void *buf, size_t count, off_t offset, jd->head.offset = offset; jd->head.size = count; - strcpy(jd->head.target_path, path); + pstrcpy(jd->head.target_path, sizeof(jd->head.target_path), path); jd->data = buf; diff --git a/sheep/vdi.c b/sheep/vdi.c index 6613db8..f98a1f6 100644 --- a/sheep/vdi.c +++ b/sheep/vdi.c @@ -254,7 +254,7 @@ static int create_vdi_obj(struct vdi_iocb *iocb, uint32_t new_vid, base->snap_ctime = (uint64_t) tv.tv_sec << 32 | tv.tv_usec * 1000; } - strncpy(new->name, name, sizeof(new->name)); + pstrcpy(new->name, sizeof(new->name), name); new->vdi_id = new_vid; new->create_time = (uint64_t) tv.tv_sec << 32 | tv.tv_usec * 1000; new->vdi_size = iocb->size; -- 1.7.2.5 -- sheepdog mailing list sheepdog@lists.wpkg.org http://lists.wpkg.org/mailman/listinfo/sheepdog