* [PATCH] Btrfs-progs use safe string manipulation functions @ 2011-02-07 12:22 Eduardo Silva 2011-02-07 18:17 ` Goffredo Baroncelli 2011-02-10 11:08 ` Thomas Bellman 0 siblings, 2 replies; 19+ messages in thread From: Eduardo Silva @ 2011-02-07 12:22 UTC (permalink / raw) To: linux-btrfs [-- Attachment #1: Type: text/plain, Size: 113 bytes --] Please find the attached patch which replace unsafe strcpy(3) by strncpy(3) functions. regards, Eduardo Silva [-- Attachment #2: 0001-PATCH-Btrfs-progs-use-safe-string-manipulation-funct.patch --] [-- Type: text/x-patch, Size: 4774 bytes --] >From 5fc888c71981e4b74ba29dd53bf0d5a65b3ee504 Mon Sep 17 00:00:00 2001 From: Eduardo Silva <eduardo.silva@oracle.com> Date: Mon, 7 Feb 2011 08:55:04 -0300 Subject: [PATCH] [PATCH] Btrfs-progs use safe string manipulation functions Signed-off-by: Eduardo Silva <eduardo.silva@oracle.com> --- btrfs_cmds.c | 14 +++++++------- btrfsctl.c | 2 +- convert.c | 2 +- utils.c | 9 +++++---- 4 files changed, 14 insertions(+), 13 deletions(-) diff --git a/btrfs_cmds.c b/btrfs_cmds.c index 8031c58..fffb423 100644 --- a/btrfs_cmds.c +++ b/btrfs_cmds.c @@ -375,7 +375,7 @@ int do_clone(int argc, char **argv) printf("Create a snapshot of '%s' in '%s/%s'\n", subvol, dstdir, newname); args.fd = fd; - strcpy(args.name, newname); + strncpy(args.name, newname, BTRFS_PATH_NAME_MAX); res = ioctl(fddst, BTRFS_IOC_SNAP_CREATE, &args); close(fd); @@ -436,7 +436,7 @@ int do_delete_subvolume(int argc, char **argv) } printf("Delete subvolume '%s/%s'\n", dname, vname); - strcpy(args.name, vname); + strncpy(args.name, vname, BTRFS_PATH_NAME_MAX); res = ioctl(fd, BTRFS_IOC_SNAP_DESTROY, &args); close(fd); @@ -490,7 +490,7 @@ int do_create_subvol(int argc, char **argv) } printf("Create subvolume '%s/%s'\n", dstdir, newname); - strcpy(args.name, newname); + strncpy(args.name, newname, BTRFS_PATH_NAME_MAX); res = ioctl(fddst, BTRFS_IOC_SUBVOL_CREATE, &args); close(fddst); @@ -553,7 +553,7 @@ int do_scan(int argc, char **argv) printf("Scanning for Btrfs filesystems in '%s'\n", argv[i]); - strcpy(args.name, argv[i]); + strncpy(args.name, argv[i], BTRFS_PATH_NAME_MAX); /* * FIXME: which are the error code returned by this ioctl ? * it seems that is impossible to understand if there no is @@ -593,7 +593,7 @@ int do_resize(int argc, char **argv) } printf("Resize '%s' of '%s'\n", path, amount); - strcpy(args.name, amount); + strncpy(args.name, amount, BTRFS_PATH_NAME_MAX); res = ioctl(fd, BTRFS_IOC_RESIZE, &args); close(fd); if( res < 0 ){ @@ -736,7 +736,7 @@ int do_add_volume(int nargs, char **args) } close(devfd); - strcpy(ioctl_args.name, args[i]); + strncpy(ioctl_args.name, args[i], BTRFS_PATH_NAME_MAX); res = ioctl(fdmnt, BTRFS_IOC_ADD_DEV, &ioctl_args); if(res<0){ fprintf(stderr, "ERROR: error adding the device '%s'\n", args[i]); @@ -792,7 +792,7 @@ int do_remove_volume(int nargs, char **args) struct btrfs_ioctl_vol_args arg; int res; - strcpy(arg.name, args[i]); + strncpy(arg.name, args[i], BTRFS_PATH_NAME_MAX); res = ioctl(fdmnt, BTRFS_IOC_RM_DEV, &arg); if(res<0){ fprintf(stderr, "ERROR: error removing the device '%s'\n", args[i]); diff --git a/btrfsctl.c b/btrfsctl.c index 92bdf39..adfa519 100644 --- a/btrfsctl.c +++ b/btrfsctl.c @@ -237,7 +237,7 @@ int main(int ac, char **av) } if (name) - strcpy(args.name, name); + strncpy(args.name, name, BTRFS_PATH_NAME_MAX + 1); else args.name[0] = '\0'; diff --git a/convert.c b/convert.c index d037c98..fbcf4a3 100644 --- a/convert.c +++ b/convert.c @@ -857,7 +857,7 @@ static int copy_single_xattr(struct btrfs_trans_handle *trans, data = databuf; datalen = bufsize; } - strcpy(namebuf, xattr_prefix_table[name_index]); + strncpy(namebuf, xattr_prefix_table[name_index], XATTR_NAME_MAX); strncat(namebuf, EXT2_EXT_ATTR_NAME(entry), entry->e_name_len); if (name_len + datalen > BTRFS_LEAF_DATA_SIZE(root) - sizeof(struct btrfs_item) - sizeof(struct btrfs_dir_item)) { diff --git a/utils.c b/utils.c index fd894f3..96ef94d 100644 --- a/utils.c +++ b/utils.c @@ -108,7 +108,7 @@ int make_btrfs(int fd, const char *device, const char *label, btrfs_set_super_csum_type(&super, BTRFS_CSUM_TYPE_CRC32); btrfs_set_super_chunk_root_generation(&super, 1); if (label) - strcpy(super.label, label); + strncpy(super.label, label, BTRFS_LABEL_SIZE - 1); buf = malloc(sizeof(*buf) + max(sectorsize, leafsize)); @@ -828,7 +828,7 @@ void btrfs_register_one_device(char *fname) "skipping device registration\n"); return; } - strcpy(args.name, fname); + strncpy(args.name, fname, BTRFS_PATH_NAME_MAX); ret = ioctl(fd, BTRFS_IOC_SCAN_DEV, &args); close(fd); } @@ -971,6 +971,7 @@ static char *size_strs[] = { "", "KB", "MB", "GB", "TB", char *pretty_sizes(u64 size) { int num_divs = 0; + int pretty_len = 16; u64 last_size = size; u64 fract_size = size; float fraction; @@ -988,8 +989,8 @@ char *pretty_sizes(u64 size) return NULL; fraction = (float)fract_size / 1024; - pretty = malloc(16); - sprintf(pretty, "%.2f%s", fraction, size_strs[num_divs-1]); + pretty = malloc(pretty_len); + snprintf(pretty, pretty_len, "%.2f%s", fraction, size_strs[num_divs-1]); return pretty; } -- 1.7.1 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] Btrfs-progs use safe string manipulation functions 2011-02-07 12:22 [PATCH] Btrfs-progs use safe string manipulation functions Eduardo Silva @ 2011-02-07 18:17 ` Goffredo Baroncelli 2011-02-10 11:08 ` Thomas Bellman 1 sibling, 0 replies; 19+ messages in thread From: Goffredo Baroncelli @ 2011-02-07 18:17 UTC (permalink / raw) To: Eduardo Silva; +Cc: linux-btrfs On 02/07/2011 01:22 PM, Eduardo Silva wrote: > Please find the attached patch which replace unsafe strcpy(3) by > strncpy(3) functions. > > regards, > > Eduardo Silva Hi Eduardo, even though some "strncpy" are unneeded because a check is performed before, I fully agree that "strncpy" is better than a simple strcpy. However I have to highlight that truncating a path without noticing may be lead to a problem (think a delete operation applied to a shorter path...). So can I ask you to add some checks to your patch, in order to warn the user that the path is too long ? Regards G.Baroncelli ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Btrfs-progs use safe string manipulation functions 2011-02-07 12:22 [PATCH] Btrfs-progs use safe string manipulation functions Eduardo Silva 2011-02-07 18:17 ` Goffredo Baroncelli @ 2011-02-10 11:08 ` Thomas Bellman 2011-02-10 11:21 ` Olaf van der Spek 2011-02-10 11:49 ` Eduardo Silva 1 sibling, 2 replies; 19+ messages in thread From: Thomas Bellman @ 2011-02-10 11:08 UTC (permalink / raw) To: Eduardo Silva; +Cc: linux-btrfs On 2011-02-07 13:22, Eduardo Silva wrote: > Please find the attached patch which replace unsafe strcpy(3) by > strncpy(3) functions. strncpy() doesn't NUL-terminate the destination buffer if the maximum length is reached. And as far as I can see, there is no other initialization of those buffers to zeroes, except for super.label in make_btrfs() in utils.c. So please change those strncpy() calls to something like: strncpy(args.name, source, BTRFS_PATH_NAME_MAX); args.name[BTRFS_PATH_NAME_MAX] = '\0'; (Note that the name member of struct btrfs_ioctl_vol_args is BTRFS_PATH_NAME_MAX + 1 long, so the above is correct for that field.) And of course similarly in those cases where you copy to something other than a struct btrfs_ioctl_vol_args. There were also a two places where you used spaces instead of tabs for indentation (in main() in btrfsctl.c, and the declaration of pretty_len in pretty_sizes() in utils.c). /Bellman ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Btrfs-progs use safe string manipulation functions 2011-02-10 11:08 ` Thomas Bellman @ 2011-02-10 11:21 ` Olaf van der Spek 2011-02-10 11:37 ` Jeremy Sanders 2011-02-10 11:49 ` Eduardo Silva 1 sibling, 1 reply; 19+ messages in thread From: Olaf van der Spek @ 2011-02-10 11:21 UTC (permalink / raw) To: Thomas Bellman; +Cc: Eduardo Silva, linux-btrfs On Thu, Feb 10, 2011 at 12:08 PM, Thomas Bellman <bellman@nsc.liu.se> w= rote: > =C2=A0 =C2=A0strncpy(args.name, source, BTRFS_PATH_NAME_MAX); > =C2=A0 =C2=A0args.name[BTRFS_PATH_NAME_MAX] =3D '\0'; That's silly. Isn't there a sane safe variant of strcpy? Olaf -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" = in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Btrfs-progs use safe string manipulation functions 2011-02-10 11:21 ` Olaf van der Spek @ 2011-02-10 11:37 ` Jeremy Sanders 2011-02-10 11:39 ` Olaf van der Spek ` (2 more replies) 0 siblings, 3 replies; 19+ messages in thread From: Jeremy Sanders @ 2011-02-10 11:37 UTC (permalink / raw) To: linux-btrfs Olaf van der Spek wrote: > On Thu, Feb 10, 2011 at 12:08 PM, Thomas Bellman <bellman@nsc.liu.se> > wrote: >> strncpy(args.name, source, BTRFS_PATH_NAME_MAX); >> args.name[BTRFS_PATH_NAME_MAX] = '\0'; > > That's silly. Isn't there a sane safe variant of strcpy? There's strlcpy, but it's not in glibc because of possible truncation errors! http://en.wikipedia.org/wiki/Strlcpy Of course C++ strings would be much better... :-) Jeremy ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Btrfs-progs use safe string manipulation functions 2011-02-10 11:37 ` Jeremy Sanders @ 2011-02-10 11:39 ` Olaf van der Spek 2011-02-10 13:29 ` Eduardo Silva 2011-02-10 11:54 ` Lars Wirzenius 2011-02-10 15:17 ` Olaf van der Spek 2 siblings, 1 reply; 19+ messages in thread From: Olaf van der Spek @ 2011-02-10 11:39 UTC (permalink / raw) To: Jeremy Sanders; +Cc: linux-btrfs On Thu, Feb 10, 2011 at 12:37 PM, Jeremy Sanders <jeremy@jeremysanders.net> wrote: > Olaf van der Spek wrote: > >> On Thu, Feb 10, 2011 at 12:08 PM, Thomas Bellman <bellman@nsc.liu.se> >> wrote: >>> strncpy(args.name, source, BTRFS_PATH_NAME_MAX); >>> args.name[BTRFS_PATH_NAME_MAX] = '\0'; >> >> That's silly. Isn't there a sane safe variant of strcpy? > > There's strlcpy, but it's not in glibc because of possible truncation > errors! Then use a private wrapper. -- Olaf ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Btrfs-progs use safe string manipulation functions 2011-02-10 11:39 ` Olaf van der Spek @ 2011-02-10 13:29 ` Eduardo Silva 2011-02-10 13:34 ` Olaf van der Spek ` (2 more replies) 0 siblings, 3 replies; 19+ messages in thread From: Eduardo Silva @ 2011-02-10 13:29 UTC (permalink / raw) To: Olaf van der Spek; +Cc: Jeremy Sanders, linux-btrfs [-- Attachment #1: Type: text/plain, Size: 1015 bytes --] On Thu, 2011-02-10 at 12:39 +0100, Olaf van der Spek wrote: > On Thu, Feb 10, 2011 at 12:37 PM, Jeremy Sanders > <jeremy@jeremysanders.net> wrote: > > Olaf van der Spek wrote: > > > >> On Thu, Feb 10, 2011 at 12:08 PM, Thomas Bellman <bellman@nsc.liu.se> > >> wrote: > >>> strncpy(args.name, source, BTRFS_PATH_NAME_MAX); > >>> args.name[BTRFS_PATH_NAME_MAX] = '\0'; > >> > >> That's silly. Isn't there a sane safe variant of strcpy? > > > > There's strlcpy, but it's not in glibc because of possible truncation > > errors! > > Then use a private wrapper. > Here's the new patch: ---- [PATCH] Add safe string manipulation functions Deprecate direct use of strcpy(3) The following string manipulation function has been added: - string_copy() : wrapper of strcpy(3) - string_ncopy(): wrapper of strncpy(3) both function compose safe NULL terminated strings. ---- I check that the code most of the time raise an error if the path is too long, so the new wrappers should be ok... best, Eduardo Silva [-- Attachment #2: 0001-Add-safe-string-manipulation-functions.patch --] [-- Type: text/x-patch, Size: 8559 bytes --] >From 6e551f4d9482a438beb336c4ec3a54735a15b76c Mon Sep 17 00:00:00 2001 From: Eduardo Silva <eduardo.silva@oracle.com> Date: Thu, 10 Feb 2011 10:25:15 -0300 Subject: [PATCH] Add safe string manipulation functions Deprecate direct use of strcpy(3) The following string manipulation function has been added: - string_copy() : wrapper of strcpy(3) - string_ncopy(): wrapper of strncpy(3) both function compose safe NULL terminated strings. Signed-off-by: Eduardo Silva <eduardo.silva@oracle.com> --- btrfs-list.c | 4 ++-- btrfs-vol.c | 2 +- btrfs.c | 3 ++- btrfs_cmds.c | 14 +++++++------- btrfsctl.c | 2 +- convert.c | 4 ++-- utils.c | 38 ++++++++++++++++++++++++++++++++------ utils.h | 5 +++++ 8 files changed, 52 insertions(+), 20 deletions(-) diff --git a/btrfs-list.c b/btrfs-list.c index 93766a8..ede51a4 100644 --- a/btrfs-list.c +++ b/btrfs-list.c @@ -291,7 +291,7 @@ static int lookup_ino_path(int fd, struct root_info *ri) perror("malloc failed"); exit(1); } - strcpy(ri->path, args.name); + string_copy(ri->path, args.name); strcat(ri->path, ri->name); } else { /* we're at the root of ref_tree */ @@ -448,7 +448,7 @@ char *build_name(char *dirid, char *name) full = malloc(strlen(dirid) + strlen(name) + 1); if (!full) return NULL; - strcpy(full, dirid); + string_copy(full, dirid); strcat(full, name); return full; } diff --git a/btrfs-vol.c b/btrfs-vol.c index 4ed799d..60361f6 100644 --- a/btrfs-vol.c +++ b/btrfs-vol.c @@ -151,7 +151,7 @@ int main(int ac, char **av) } fd = dirfd(dirstream); if (device) - strcpy(args.name, device); + string_copy(args.name, device); else args.name[0] = '\0'; diff --git a/btrfs.c b/btrfs.c index 46314cf..ddf4960 100644 --- a/btrfs.c +++ b/btrfs.c @@ -21,6 +21,7 @@ #include "kerncompat.h" #include "btrfs_cmds.h" +#include "utils.h" #include "version.h" typedef int (*CommandFunction)(int argc, char **argv); @@ -241,7 +242,7 @@ static int prepare_args(int *ac, char ***av, char *prgname, struct Command *cmd for(i=0; i < *ac ; i++ ) ret[i+1] = (*av)[i]; - strcpy(newname, prgname); + string_copy(newname, prgname); strcat(newname, " "); strcat(newname, cmd->verb); diff --git a/btrfs_cmds.c b/btrfs_cmds.c index 8031c58..f4be8c2 100644 --- a/btrfs_cmds.c +++ b/btrfs_cmds.c @@ -375,7 +375,7 @@ int do_clone(int argc, char **argv) printf("Create a snapshot of '%s' in '%s/%s'\n", subvol, dstdir, newname); args.fd = fd; - strcpy(args.name, newname); + string_ncopy(args.name, newname, BTRFS_PATH_NAME_MAX); res = ioctl(fddst, BTRFS_IOC_SNAP_CREATE, &args); close(fd); @@ -436,7 +436,7 @@ int do_delete_subvolume(int argc, char **argv) } printf("Delete subvolume '%s/%s'\n", dname, vname); - strcpy(args.name, vname); + string_ncopy(args.name, vname, BTRFS_PATH_NAME_MAX); res = ioctl(fd, BTRFS_IOC_SNAP_DESTROY, &args); close(fd); @@ -490,7 +490,7 @@ int do_create_subvol(int argc, char **argv) } printf("Create subvolume '%s/%s'\n", dstdir, newname); - strcpy(args.name, newname); + string_ncopy(args.name, newname, BTRFS_PATH_NAME_MAX); res = ioctl(fddst, BTRFS_IOC_SUBVOL_CREATE, &args); close(fddst); @@ -553,7 +553,7 @@ int do_scan(int argc, char **argv) printf("Scanning for Btrfs filesystems in '%s'\n", argv[i]); - strcpy(args.name, argv[i]); + string_ncopy(args.name, argv[i], BTRFS_PATH_NAME_MAX); /* * FIXME: which are the error code returned by this ioctl ? * it seems that is impossible to understand if there no is @@ -593,7 +593,7 @@ int do_resize(int argc, char **argv) } printf("Resize '%s' of '%s'\n", path, amount); - strcpy(args.name, amount); + string_ncopy(args.name, amount, BTRFS_VOL_NAME_MAX); res = ioctl(fd, BTRFS_IOC_RESIZE, &args); close(fd); if( res < 0 ){ @@ -736,7 +736,7 @@ int do_add_volume(int nargs, char **args) } close(devfd); - strcpy(ioctl_args.name, args[i]); + string_ncopy(ioctl_args.name, args[i], BTRFS_PATH_NAME_MAX); res = ioctl(fdmnt, BTRFS_IOC_ADD_DEV, &ioctl_args); if(res<0){ fprintf(stderr, "ERROR: error adding the device '%s'\n", args[i]); @@ -792,7 +792,7 @@ int do_remove_volume(int nargs, char **args) struct btrfs_ioctl_vol_args arg; int res; - strcpy(arg.name, args[i]); + string_ncopy(arg.name, args[i], BTRFS_PATH_NAME_MAX); res = ioctl(fdmnt, BTRFS_IOC_RM_DEV, &arg); if(res<0){ fprintf(stderr, "ERROR: error removing the device '%s'\n", args[i]); diff --git a/btrfsctl.c b/btrfsctl.c index 92bdf39..adfa519 100644 --- a/btrfsctl.c +++ b/btrfsctl.c @@ -237,7 +237,7 @@ int main(int ac, char **av) } if (name) - strcpy(args.name, name); + strncpy(args.name, name, BTRFS_PATH_NAME_MAX + 1); else args.name[0] = '\0'; diff --git a/convert.c b/convert.c index d037c98..67706f3 100644 --- a/convert.c +++ b/convert.c @@ -857,7 +857,7 @@ static int copy_single_xattr(struct btrfs_trans_handle *trans, data = databuf; datalen = bufsize; } - strcpy(namebuf, xattr_prefix_table[name_index]); + strncpy(namebuf, xattr_prefix_table[name_index], XATTR_NAME_MAX); strncat(namebuf, EXT2_EXT_ATTR_NAME(entry), entry->e_name_len); if (name_len + datalen > BTRFS_LEAF_DATA_SIZE(root) - sizeof(struct btrfs_item) - sizeof(struct btrfs_dir_item)) { @@ -1465,7 +1465,7 @@ struct btrfs_root *link_subvol(struct btrfs_root *root, const char *base, key.offset = (u64)-1; key.type = BTRFS_ROOT_ITEM_KEY; - strcpy(buf, base); + string_copy(buf, base); for (i = 0; i < 1024; i++) { ret = btrfs_insert_dir_item(trans, root, buf, strlen(buf), dirid, &key, BTRFS_FT_DIR, index); diff --git a/utils.c b/utils.c index fd894f3..0c052c1 100644 --- a/utils.c +++ b/utils.c @@ -108,7 +108,7 @@ int make_btrfs(int fd, const char *device, const char *label, btrfs_set_super_csum_type(&super, BTRFS_CSUM_TYPE_CRC32); btrfs_set_super_chunk_root_generation(&super, 1); if (label) - strcpy(super.label, label); + strncpy(super.label, label, BTRFS_LABEL_SIZE - 1); buf = malloc(sizeof(*buf) + max(sectorsize, leafsize)); @@ -828,7 +828,7 @@ void btrfs_register_one_device(char *fname) "skipping device registration\n"); return; } - strcpy(args.name, fname); + strncpy(args.name, fname, BTRFS_PATH_NAME_MAX); ret = ioctl(fd, BTRFS_IOC_SCAN_DEV, &args); close(fd); } @@ -853,7 +853,7 @@ int btrfs_scan_one_dir(char *dirname, int run_ioctl) pending = malloc(sizeof(*pending)); if (!pending) return -ENOMEM; - strcpy(pending->name, dirname); + string_copy(pending->name, dirname); again: dirname_len = strlen(pending->name); @@ -894,7 +894,7 @@ again: ret = -ENOMEM; goto fail; } - strcpy(next->name, fullpath); + string_copy(next->name, fullpath); list_add_tail(&next->list, &pending_list); } if (!S_ISBLK(st.st_mode)) { @@ -971,6 +971,7 @@ static char *size_strs[] = { "", "KB", "MB", "GB", "TB", char *pretty_sizes(u64 size) { int num_divs = 0; + int pretty_len = 16; u64 last_size = size; u64 fract_size = size; float fraction; @@ -988,8 +989,33 @@ char *pretty_sizes(u64 size) return NULL; fraction = (float)fract_size / 1024; - pretty = malloc(16); - sprintf(pretty, "%.2f%s", fraction, size_strs[num_divs-1]); + pretty = malloc(pretty_len); + snprintf(pretty, pretty_len, "%.2f%s", fraction, size_strs[num_divs-1]); return pretty; } +char *string_copy(char *dest, const char *src) +{ + if (!dest || !src) { + fprintf(stderr, "ERROR: invalid string_copy() parameters"); + exit(EXIT_FAILURE); + } + + memset(dest, '\0', sizeof(dest)); + return strcpy(dest, src); +} + +char *string_ncopy(char *dest, const char *src, size_t n) +{ + /* Just a basic test to avoid silly bugs */ + if (!dest || !src || n <= 0) { + fprintf(stderr, "ERROR: invalid string_ncopy() parameters\n"); + exit(EXIT_FAILURE); + } + + strncpy(dest, src, n); + dest[n] = '\0'; + + return dest; +} + diff --git a/utils.h b/utils.h index 9dce5b0..39c8455 100644 --- a/utils.h +++ b/utils.h @@ -19,6 +19,8 @@ #ifndef __UTILS__ #define __UTILS__ +#include "ctree.h" + #define BTRFS_MKFS_SYSTEM_GROUP_SIZE (4 * 1024 * 1024) int make_btrfs(int fd, const char *device, const char *label, @@ -40,4 +42,7 @@ int check_mounted(const char *devicename); int btrfs_device_already_in_root(struct btrfs_root *root, int fd, int super_offset); char *pretty_sizes(u64 size); + +char *string_copy(char *dest, const char *src); +char *string_ncopy(char *dest, const char *src, size_t n); #endif -- 1.7.1 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] Btrfs-progs use safe string manipulation functions 2011-02-10 13:29 ` Eduardo Silva @ 2011-02-10 13:34 ` Olaf van der Spek 2011-02-10 13:41 ` Eduardo Silva [not found] ` <1297345079.28159.14.camel@monotop> 2011-02-10 18:39 ` Goffredo Baroncelli 2011-02-11 12:41 ` Lars Wirzenius 2 siblings, 2 replies; 19+ messages in thread From: Olaf van der Spek @ 2011-02-10 13:34 UTC (permalink / raw) To: Eduardo Silva; +Cc: Jeremy Sanders, linux-btrfs On Thu, Feb 10, 2011 at 2:29 PM, Eduardo Silva <eduardo.silva@oracle.co= m> wrote: >> > There's strlcpy, but it's not in glibc because of possible truncat= ion >> > errors! >> >> Then use a private wrapper. >> > > Here's the new patch: > > ---- > [PATCH] Add safe string manipulation functions > > Deprecate direct use of strcpy(3) > The following string manipulation function has been added: > > =C2=A0 - string_copy() : wrapper of strcpy(3) > =C2=A0 - string_ncopy(): wrapper of strncpy(3) > > both function compose safe NULL terminated strings. > ---- > > I check that the code most of the time raise an error if the path is = too > long, so the new wrappers should be ok... string_copy seems pointless, it's kinda equivalent to strcpy. if (!dest || !src) should include an assert so it's easier to break in the debugger. --=20 Olaf -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" = in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Btrfs-progs use safe string manipulation functions 2011-02-10 13:34 ` Olaf van der Spek @ 2011-02-10 13:41 ` Eduardo Silva [not found] ` <1297345079.28159.14.camel@monotop> 1 sibling, 0 replies; 19+ messages in thread From: Eduardo Silva @ 2011-02-10 13:41 UTC (permalink / raw) To: Olaf van der Spek; +Cc: Jeremy Sanders, linux-btrfs On Thu, 2011-02-10 at 14:34 +0100, Olaf van der Spek wrote: > On Thu, Feb 10, 2011 at 2:29 PM, Eduardo Silva <eduardo.silva@oracle.com> wrote: > >> > There's strlcpy, but it's not in glibc because of possible truncation > >> > errors! > >> > >> Then use a private wrapper. > >> > > > > Here's the new patch: > > > > ---- > > [PATCH] Add safe string manipulation functions > > > > Deprecate direct use of strcpy(3) > > The following string manipulation function has been added: > > > > - string_copy() : wrapper of strcpy(3) > > - string_ncopy(): wrapper of strncpy(3) > > > > both function compose safe NULL terminated strings. > > ---- > > > > I check that the code most of the time raise an error if the path is too > > long, so the new wrappers should be ok... > > string_copy seems pointless, it's kinda equivalent to strcpy. got your point, but If we are creating wrappers for string manipulation, let's do it for the most common functions used. > if (!dest || !src) should include an assert so it's easier to break in > the debugger. > good one regards, Ed.- ^ permalink raw reply [flat|nested] 19+ messages in thread
[parent not found: <1297345079.28159.14.camel@monotop>]
* Re: [PATCH] Btrfs-progs use safe string manipulation functions [not found] ` <1297345079.28159.14.camel@monotop> @ 2011-02-10 13:52 ` Olaf van der Spek 2011-02-10 14:00 ` Eduardo Silva 0 siblings, 1 reply; 19+ messages in thread From: Olaf van der Spek @ 2011-02-10 13:52 UTC (permalink / raw) To: Eduardo Silva; +Cc: Jeremy Sanders, linux-btrfs On Thu, Feb 10, 2011 at 2:37 PM, Eduardo Silva <eduardo.silva@oracle.com> wrote: > string_copy seems pointless, it's kinda equivalent to strcpy. > > Yeah, but if we are thinking into write some wrappers let's create a couple > for the major string manipulation used... A wrapper should have a benefit, your string_copy doesn't have any. -- Olaf ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Btrfs-progs use safe string manipulation functions 2011-02-10 13:52 ` Olaf van der Spek @ 2011-02-10 14:00 ` Eduardo Silva 2011-02-10 14:05 ` Olaf van der Spek 0 siblings, 1 reply; 19+ messages in thread From: Eduardo Silva @ 2011-02-10 14:00 UTC (permalink / raw) To: Olaf van der Spek; +Cc: Jeremy Sanders, linux-btrfs On Thu, 2011-02-10 at 14:52 +0100, Olaf van der Spek wrote: > On Thu, Feb 10, 2011 at 2:37 PM, Eduardo Silva <eduardo.silva@oracle.com> wrote: > > string_copy seems pointless, it's kinda equivalent to strcpy. > > > > Yeah, but if we are thinking into write some wrappers let's create a couple > > for the major string manipulation used... > > A wrapper should have a benefit, your string_copy doesn't have any. It have it, take this example: char *a; a = malloc(1024); strcpy(a, NULL); at least with the wrapper you will get a notice about what's going on... ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Btrfs-progs use safe string manipulation functions 2011-02-10 14:00 ` Eduardo Silva @ 2011-02-10 14:05 ` Olaf van der Spek 0 siblings, 0 replies; 19+ messages in thread From: Olaf van der Spek @ 2011-02-10 14:05 UTC (permalink / raw) To: Eduardo Silva; +Cc: Jeremy Sanders, linux-btrfs On Thu, Feb 10, 2011 at 3:00 PM, Eduardo Silva <eduardo.silva@oracle.co= m> wrote: > On Thu, 2011-02-10 at 14:52 +0100, Olaf van der Spek wrote: >> On Thu, Feb 10, 2011 at 2:37 PM, Eduardo Silva <eduardo.silva@oracle= =2Ecom> wrote: >> > string_copy seems pointless, it's kinda equivalent to strcpy. >> > >> > Yeah, but if we are thinking into write some wrappers let's create= a couple >> > for the major string manipulation used... >> >> A wrapper should have a benefit, your string_copy doesn't have any. > > It have it, take this example: > > =C2=A0 =C2=A0 =C2=A0 =C2=A0char *a; > =C2=A0 =C2=A0 =C2=A0 =C2=A0a =3D malloc(1024); > =C2=A0 =C2=A0 =C2=A0 =C2=A0strcpy(a, NULL); > > at least with the wrapper you will get a notice about what's going > on... The debugger shows you what's going on without wrapper. --=20 Olaf -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" = in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Btrfs-progs use safe string manipulation functions 2011-02-10 13:29 ` Eduardo Silva 2011-02-10 13:34 ` Olaf van der Spek @ 2011-02-10 18:39 ` Goffredo Baroncelli 2011-02-11 12:41 ` Lars Wirzenius 2 siblings, 0 replies; 19+ messages in thread From: Goffredo Baroncelli @ 2011-02-10 18:39 UTC (permalink / raw) To: Eduardo Silva; +Cc: Olaf van der Spek, Jeremy Sanders, linux-btrfs On 02/10/2011 02:29 PM, Eduardo Silva wrote: > On Thu, 2011-02-10 at 12:39 +0100, Olaf van der Spek wrote: >> On Thu, Feb 10, 2011 at 12:37 PM, Jeremy Sanders >> <jeremy@jeremysanders.net> wrote: >>> Olaf van der Spek wrote: >>> >>>> On Thu, Feb 10, 2011 at 12:08 PM, Thomas Bellman <bellman@nsc.liu.se> >>>> wrote: >>>>> strncpy(args.name, source, BTRFS_PATH_NAME_MAX); >>>>> args.name[BTRFS_PATH_NAME_MAX] = '\0'; >>>> >>>> That's silly. Isn't there a sane safe variant of strcpy? >>> >>> There's strlcpy, but it's not in glibc because of possible truncation >>> errors! >> >> Then use a private wrapper. >> > > Here's the new patch: > > ---- > [PATCH] Add safe string manipulation functions > > Deprecate direct use of strcpy(3) > The following string manipulation function has been added: > > - string_copy() : wrapper of strcpy(3) > - string_ncopy(): wrapper of strncpy(3) > > both function compose safe NULL terminated strings. > ---- > > I check that the code most of the time raise an error if the path is too > long, so the new wrappers should be ok... > > best, > > Eduardo Silva > [...] +char *string_copy(char *dest, const char *src) +{ + if (!dest || !src) { + fprintf(stderr, "ERROR: invalid string_copy() parameters"); + exit(EXIT_FAILURE); + } + + memset(dest, '\0', sizeof(dest)); What is the purpose of the line above ? sizeof(dest) is a const value (typically 4 or 8) ! I agree with Olaf that string_copy() is usefulness. I suggest you to improve the check of the string length before the copy (not in the copy function), and raising an error when the length of the string is too long. Regards G.Baroncelli ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Btrfs-progs use safe string manipulation functions 2011-02-10 13:29 ` Eduardo Silva 2011-02-10 13:34 ` Olaf van der Spek 2011-02-10 18:39 ` Goffredo Baroncelli @ 2011-02-11 12:41 ` Lars Wirzenius 2 siblings, 0 replies; 19+ messages in thread From: Lars Wirzenius @ 2011-02-11 12:41 UTC (permalink / raw) To: Eduardo Silva; +Cc: linux-btrfs On to, 2011-02-10 at 10:29 -0300, Eduardo Silva wrote: > [PATCH] Add safe string manipulation functions > > Deprecate direct use of strcpy(3) > The following string manipulation function has been added: > > - string_copy() : wrapper of strcpy(3) > - string_ncopy(): wrapper of strncpy(3) > > both function compose safe NULL terminated strings. I'd like make some comments, which I hope will be acceptable. Firstly, calling strcpy dangerous is, to me, rather overblown. It is easy to make mistakes, but it is not at all dangerous the way, for example, gets(3) is dangerous. strcpy can be used safely, gets cannot. Also, if you consider strcpy to be dangerous, then strcat should be dangerous too. However, given the risk of overwriting a buffer with strcpy, I agree that it's good to see if an alternative can be found. Secondly, if you're going to make wrappers or helper functions for string handling in C, you need to decide several things right from the start: * do you do static or dynamic allocation? * how do you handle errors? * do you want a minimal wrapper or replacement, or a whole new library? I am not familiar enough with the btrfs-progs code base to give any strong recommendations, but off the top of my head I would suggest these, for this patch: * make use of fairly minimal wrappers/replacements (at least for now) * handle errors by calling abort or exit * don't allocate data dynamically (or else it's not a minimal wrapper) For error handling, there are two kinds of things that can happen: normal run-time errors (malloc returns NULL, writing to a file fails, etc), and programming errors (wrong parameters to functions). If we're doing a minimal wrapper without dynamic memory allocation, the only thing string functions should need to worry about is programming errors. For those, abort(3) is the appropriate way to terminate the program, since it causes a core dump, which can be inspected with a debugger. Since btrfs-progs are non-interactive command line tools, this should be OK. For checking function arguments, the assert macro is appropriate. It calls abort if the test fails. I am not sure I would check for parameters being non-NULL, though, since the kernel will trap such usage and cause a segfault, which, again, can be analyzed with a debugger. For things like string copying, another problem to consider is what to do if the target array is not large enough? The two possibilities is to silently truncate the output string, return an error code of some sort, or to abort. The error code is a bit tedious, since it requires the caller to check for it, and do something sensible if it's not enough. For btrfs-progs, I would suggest aborting. Taking all of these together, my suggestion for a "safer strcpy" would be along these lines (outline only, not tested code): void safer_strcpy(char *target, size_t tsize, const char *source) { size_t n; n = snprintf(target, tsize, "%s", source); assert(n < tsize); } void safer_strncpy(char *tgt, size_t tsize, const char *src, size_t n) { assert(n < tsize); /* There must be space for the '\0'. */ memset(tgt, '\0', tsize); strncpy(tgt, src, n); } Note that for any reasonable error checking to be possible the safety functions need to know the size of the target memory area. Otherwise no sensible checks can be done -- you have to rely on the caller to check that the target array is big enough, and then you're nowhere better than with plain strcpy. (Also note that I did not call the function string_copy, since global names starting with str are reserved to the C implementation.) Your function fills in the target array with zero bytes. Is that necessary? If it is, then the memset call needs to be added to safer_strcpy. (I don't find it useful to return the target array as the return value of the function, so I didn't do that.) -- Blog/wiki/website hosting with ikiwiki (free for free software): http://www.branchable.com/ ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Btrfs-progs use safe string manipulation functions 2011-02-10 11:37 ` Jeremy Sanders 2011-02-10 11:39 ` Olaf van der Spek @ 2011-02-10 11:54 ` Lars Wirzenius 2011-02-10 12:27 ` Olaf van der Spek 2011-02-10 15:17 ` Olaf van der Spek 2 siblings, 1 reply; 19+ messages in thread From: Lars Wirzenius @ 2011-02-10 11:54 UTC (permalink / raw) To: linux-btrfs On to, 2011-02-10 at 11:37 +0000, Jeremy Sanders wrote: > There's strlcpy, but it's not in glibc because of possible truncation > errors! snprintf is standard, and should be about as safe as it gets with the glibc functions. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Btrfs-progs use safe string manipulation functions 2011-02-10 11:54 ` Lars Wirzenius @ 2011-02-10 12:27 ` Olaf van der Spek 2011-02-10 12:41 ` Thomas Bellman 0 siblings, 1 reply; 19+ messages in thread From: Olaf van der Spek @ 2011-02-10 12:27 UTC (permalink / raw) To: Lars Wirzenius; +Cc: linux-btrfs On Thu, Feb 10, 2011 at 12:54 PM, Lars Wirzenius <liw@liw.fi> wrote: > On to, 2011-02-10 at 11:37 +0000, Jeremy Sanders wrote: >> There's strlcpy, but it's not in glibc because of possible truncation >> errors! > > snprintf is standard, and should be about as safe as it gets with the > glibc functions. But snprintf is not like strlcpy. Olaf ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Btrfs-progs use safe string manipulation functions 2011-02-10 12:27 ` Olaf van der Spek @ 2011-02-10 12:41 ` Thomas Bellman 0 siblings, 0 replies; 19+ messages in thread From: Thomas Bellman @ 2011-02-10 12:41 UTC (permalink / raw) To: Olaf van der Spek; +Cc: Lars Wirzenius, linux-btrfs On 2011-02-10 13:27, Olaf van der Spek wrote: > On Thu, Feb 10, 2011 at 12:54 PM, Lars Wirzenius <liw@liw.fi> wrote: >> snprintf is standard, and should be about as safe as it gets with the >> glibc functions. > > But snprintf is not like strlcpy. It is indeed uglier to write 'snprintf(dst, size, "%s", src)' instead of 'strlcpy(dst, src, size)', but both the effect and the return value should be identical. /Bellman ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Btrfs-progs use safe string manipulation functions 2011-02-10 11:37 ` Jeremy Sanders 2011-02-10 11:39 ` Olaf van der Spek 2011-02-10 11:54 ` Lars Wirzenius @ 2011-02-10 15:17 ` Olaf van der Spek 2 siblings, 0 replies; 19+ messages in thread From: Olaf van der Spek @ 2011-02-10 15:17 UTC (permalink / raw) To: Jeremy Sanders; +Cc: linux-btrfs On Thu, Feb 10, 2011 at 12:37 PM, Jeremy Sanders <jeremy@jeremysanders.net> wrote: > Of course C++ strings would be much better... :-) Yeah, why isn't C++ being used? Olaf ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Btrfs-progs use safe string manipulation functions 2011-02-10 11:08 ` Thomas Bellman 2011-02-10 11:21 ` Olaf van der Spek @ 2011-02-10 11:49 ` Eduardo Silva 1 sibling, 0 replies; 19+ messages in thread From: Eduardo Silva @ 2011-02-10 11:49 UTC (permalink / raw) To: Thomas Bellman; +Cc: linux-btrfs On Thu, 2011-02-10 at 12:08 +0100, Thomas Bellman wrote: > On 2011-02-07 13:22, Eduardo Silva wrote: > > > Please find the attached patch which replace unsafe strcpy(3) by > > strncpy(3) functions. > > strncpy() doesn't NUL-terminate the destination buffer if the > maximum length is reached. And as far as I can see, there is > no other initialization of those buffers to zeroes, except for > super.label in make_btrfs() in utils.c. > > So please change those strncpy() calls to something like: > > strncpy(args.name, source, BTRFS_PATH_NAME_MAX); > args.name[BTRFS_PATH_NAME_MAX] = '\0'; > Seems like a string manipulation function is the way to go, will send a new patch shortly, best, Eduardo ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2011-02-11 12:41 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-02-07 12:22 [PATCH] Btrfs-progs use safe string manipulation functions Eduardo Silva 2011-02-07 18:17 ` Goffredo Baroncelli 2011-02-10 11:08 ` Thomas Bellman 2011-02-10 11:21 ` Olaf van der Spek 2011-02-10 11:37 ` Jeremy Sanders 2011-02-10 11:39 ` Olaf van der Spek 2011-02-10 13:29 ` Eduardo Silva 2011-02-10 13:34 ` Olaf van der Spek 2011-02-10 13:41 ` Eduardo Silva [not found] ` <1297345079.28159.14.camel@monotop> 2011-02-10 13:52 ` Olaf van der Spek 2011-02-10 14:00 ` Eduardo Silva 2011-02-10 14:05 ` Olaf van der Spek 2011-02-10 18:39 ` Goffredo Baroncelli 2011-02-11 12:41 ` Lars Wirzenius 2011-02-10 11:54 ` Lars Wirzenius 2011-02-10 12:27 ` Olaf van der Spek 2011-02-10 12:41 ` Thomas Bellman 2011-02-10 15:17 ` Olaf van der Spek 2011-02-10 11:49 ` Eduardo Silva
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.