[sheepdog] [PATCH] avoid strcpy and strncpy
MORITA Kazutaka
morita.kazutaka at lab.ntt.co.jp
Fri Oct 26 06:02:43 CEST 2012
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.kazutaka at 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
More information about the sheepdog
mailing list