[patch 1/1][mdadm] Fix needed to enable RAID device creation on SAS devices.

[patch 1/1][mdadm] Fix needed to enable RAID device creation on SAS devices.

[Wojcik, Artur]

Neil,
Please add this patch to your repository in order to correct problems with RAID creation on SAS devices. The sysfs paths for SAS devices are longer then 50 characters and lots of functions assume that 50 characters should be enough. This problem is especially visible when creating RAID on SAS devices installed behind an expander. The patch will exchange some unsecure functions with secure versions, too.

Regards,
Artur

diff --git a/Detail.c b/Detail.c
index e2cf028..6124d52 100644
--- a/Detail.c
+++ b/Detail.c
@@ -408,13 +408,13 @@ This is pretty boring
                        printf("  Member Arrays :");

                        while (dir && (de = readdir(dir)) != NULL) {
-                               char path[200];
+                               char path[PATH_MAX];
                                char vbuf[1024];
                                int nlen = strlen(sra->sys_name);
                                int dn;
                                if (de->d_name[0] == '.')
                                        continue;
-                               sprintf(path, "/sys/block/%s/md/metadata_version",
+                               snprintf(path, PATH_MAX, "/sys/block/%s/md/metadata_version",
                                        de->d_name);
                                if (load_sys(path, vbuf) < 0)
                                        continue;
diff --git a/platform-intel.c b/platform-intel.c
index d568ca6..8b11f29 100644
--- a/platform-intel.c
+++ b/platform-intel.c
@@ -44,15 +44,15 @@ void free_sys_dev(struct sys_dev **list)
 struct sys_dev *find_driver_devices(const char *bus, const char *driver)
 {
        /* search sysfs for devices driven by 'driver' */
-       char path[256];
-       char link[256];
+       char path[PATH_MAX];
+       char link[PATH_MAX];
        char *c;
        DIR *driver_dir;
        struct dirent *de;
        struct sys_dev *head = NULL;
        struct sys_dev *list = NULL;

-       sprintf(path, "/sys/bus/%s/drivers/%s", bus, driver);
+       snprintf(path, PATH_MAX, "/sys/bus/%s/drivers/%s", bus, driver);
        driver_dir = opendir(path);
        if (!driver_dir)
                return NULL;
@@ -60,7 +60,7 @@ struct sys_dev *find_driver_devices(const char *bus, const char *driver)
                /* is 'de' a device? check that the 'subsystem' link exists and
                 * that its target matches 'bus'
                 */
-               sprintf(path, "/sys/bus/%s/drivers/%s/%s/subsystem",
+               snprintf(path, PATH_MAX, "/sys/bus/%s/drivers/%s/%s/subsystem",
                        bus, driver, de->d_name);
                if (readlink(path, link, sizeof(link)) < 0)
                        continue;
@@ -85,7 +85,7 @@ struct sys_dev *find_driver_devices(const char *bus, const char *driver)
                }

                /* generate canonical path name for the device */
-               sprintf(path, "/sys/bus/%s/drivers/%s/%s",
+               snprintf(path, PATH_MAX, "/sys/bus/%s/drivers/%s/%s",
                        bus, driver, de->d_name);
                list->path = canonicalize_file_name(path);
                list->next = NULL;
@@ -96,13 +96,13 @@ struct sys_dev *find_driver_devices(const char *bus, const char *driver)

 __u16 devpath_to_vendor(const char *dev_path)
 {
-       char path[strlen(dev_path) + strlen("/vendor") + 1];
+       char path[PATH_MAX];
        char vendor[7];
        int fd;
        __u16 id = 0xffff;
        int n;

-       sprintf(path, "%s/vendor", dev_path);
+       snprintf(path, PATH_MAX, "%s/vendor", dev_path);

        fd = open(path, O_RDONLY);
        if (fd < 0)
@@ -202,9 +202,9 @@ const struct imsm_orom *find_imsm_orom(void)

 char *devt_to_devpath(dev_t dev)
 {
-       char device[40];
+       char device[PATH_MAX];

-       sprintf(device, "/sys/dev/block/%d:%d/device", major(dev), minor(dev));
+       snprintf(device, PATH_MAX, "/sys/dev/block/%d:%d/device", major(dev), minor(dev));
        return canonicalize_file_name(device);
 }

diff --git a/super-intel.c b/super-intel.c
index 9a99d60..561645c 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -882,7 +882,7 @@ static int imsm_enumerate_ports(const char *hba_path, int port_count, int host_b
                char vendor[64];
                char buf[1024];
                int major, minor;
-               char *device;
+               char device[PATH_MAX];
                char *c;
                int port;
                int type;
@@ -898,20 +898,12 @@ static int imsm_enumerate_ports(const char *hba_path, int port_count, int host_b
                        continue;
                }

-               /* retrieve the scsi device type */
-               if (asprintf(&device, "/sys/dev/block/%d:%d/device/xxxxxxx", major, minor) < 0) {
-                       if (verbose)
-                               fprintf(stderr, Name ": failed to allocate 'device'\n");
-                       err = 2;
-                       break;
-               }
-               sprintf(device, "/sys/dev/block/%d:%d/device/type", major, minor);
+               snprintf(device, PATH_MAX, "/sys/dev/block/%d:%d/device/type", major, minor);
                if (load_sys(device, buf) != 0) {
                        if (verbose)
                                fprintf(stderr, Name ": failed to read device type for %s\n",
                                        path);
                        err = 2;
-                       free(device);
                        break;
                }
                type = strtoul(buf, NULL, 10);
@@ -920,7 +912,7 @@ static int imsm_enumerate_ports(const char *hba_path, int port_count, int host_b
                if (!(type == 0 || type == 7 || type == 14)) {
                        vendor[0] = '\0';
                        model[0] = '\0';
-                       sprintf(device, "/sys/dev/block/%d:%d/device/vendor", major, minor);
+                       snprintf(device, PATH_MAX, "/sys/dev/block/%d:%d/device/vendor", major, minor);
                        if (load_sys(device, buf) == 0) {
                                strncpy(vendor, buf, sizeof(vendor));
                                vendor[sizeof(vendor) - 1] = '\0';
@@ -929,7 +921,7 @@ static int imsm_enumerate_ports(const char *hba_path, int port_count, int host_b
                                        *c-- = '\0';

                        }
-                       sprintf(device, "/sys/dev/block/%d:%d/device/model", major, minor);
+                       snprintf(device, PATH_MAX, "/sys/dev/block/%d:%d/device/model", major, minor);
                        if (load_sys(device, buf) == 0) {
                                strncpy(model, buf, sizeof(model));
                                model[sizeof(model) - 1] = '\0';
@@ -953,9 +945,9 @@ static int imsm_enumerate_ports(const char *hba_path, int port_count, int host_b
                                case 12: sprintf(buf, "raid"); break;
                                default: sprintf(buf, "unknown");
                                }
-               } else
+               } else {
                        buf[0] = '\0';
-               free(device);
+               }

                /* chop device path to 'host%d' and calculate the port number */
                c = strchr(&path[hba_len], '/');
@@ -1576,18 +1568,18 @@ static int compare_super_imsm(struct supertype *st, struct supertype *tst)
 static void fd2devname(int fd, char *name)
 {
        struct stat st;
-       char path[256];
-       char dname[100];
+       char path[PATH_MAX];
+       char dname[PATH_MAX];
        char *nm;
        int rv;

        name[0] = '\0';
        if (fstat(fd, &st) != 0)
                return;
-       sprintf(path, "/sys/dev/block/%d:%d",
+       snprintf(path, PATH_MAX, "/sys/dev/block/%d:%d",
                major(st.st_rdev), minor(st.st_rdev));

-       rv = readlink(path, dname, sizeof(dname));
+       rv = readlink(path, dname, PATH_MAX);
        if (rv <= 0)
                return;

diff --git a/sysfs.c b/sysfs.c
index 35dfbd4..071340f 100644
--- a/sysfs.c
+++ b/sysfs.c
@@ -59,14 +59,14 @@ void sysfs_free(struct mdinfo *sra)

 int sysfs_open(int devnum, char *devname, char *attr)
 {
-       char fname[50];
+       char fname[PATH_MAX];
        int fd;
        char *mdname = devnum2devname(devnum);

        if (!mdname)
                return -1;

-       sprintf(fname, "/sys/block/%s/md/", mdname);
+       snprintf(fname, PATH_MAX, "/sys/block/%s/md/", mdname);
        if (devname) {
                strcat(fname, devname);
                strcat(fname, "/");
@@ -100,12 +100,7 @@ void sysfs_init(struct mdinfo *mdi, int fd, int devnum)

 struct mdinfo *sysfs_read(int fd, int devnum, unsigned long options)
 {
-       /* Longest possible name in sysfs, mounted at /sys, is
-        *  /sys/block/md_dXXX/md/dev-XXXXX/block/dev
-        *  /sys/block/md_dXXX/md/metadata_version
-        * which is about 41 characters.  50 should do for now
-        */
-       char fname[50];
+       char fname[PATH_MAX];
        char buf[1024];
        char *base;
        char *dbase;
@@ -124,7 +119,7 @@ struct mdinfo *sysfs_read(int fd, int devnum, unsigned long options)
                return NULL;
        }

-       sprintf(fname, "/sys/block/%s/md/", sra->sys_name);
+       snprintf(fname, PATH_MAX, "/sys/block/%s/md/", sra->sys_name);
        base = fname + strlen(fname);

        sra->devs = NULL;
@@ -376,14 +371,14 @@ unsigned long long get_component_size(int fd)
         * This returns in units of sectors.
         */
        struct stat stb;
-       char fname[50];
+       char fname[PATH_MAX];
        int n;
        if (fstat(fd, &stb)) return 0;
        if (major(stb.st_rdev) != get_mdp_major())
-               sprintf(fname, "/sys/block/md%d/md/component_size",
+               snprintf(fname, PATH_MAX, "/sys/block/md%d/md/component_size",
                        (int)minor(stb.st_rdev));
        else
-               sprintf(fname, "/sys/block/md_d%d/md/component_size",
+               snprintf(fname, PATH_MAX, "/sys/block/md_d%d/md/component_size",
                        (int)minor(stb.st_rdev)>>MdpMinorShift);
        fd = open(fname, O_RDONLY);
        if (fd < 0)
@@ -399,11 +394,11 @@ unsigned long long get_component_size(int fd)
 int sysfs_set_str(struct mdinfo *sra, struct mdinfo *dev,
                  char *name, char *val)
 {
-       char fname[50];
+       char fname[PATH_MAX];
        int n;
        int fd;

-       sprintf(fname, "/sys/block/%s/md/%s/%s",
+       snprintf(fname, PATH_MAX, "/sys/block/%s/md/%s/%s",
                sra->sys_name, dev?dev->sys_name:"", name);
        fd = open(fname, O_WRONLY);
        if (fd < 0)
@@ -428,11 +423,11 @@ int sysfs_set_num(struct mdinfo *sra, struct mdinfo *dev,

 int sysfs_uevent(struct mdinfo *sra, char *event)
 {
-       char fname[50];
+       char fname[PATH_MAX];
        int n;
        int fd;

-       sprintf(fname, "/sys/block/%s/uevent",
+       snprintf(fname, PATH_MAX, "/sys/block/%s/uevent",
                sra->sys_name);
        fd = open(fname, O_WRONLY);
        if (fd < 0)
@@ -445,10 +440,10 @@ int sysfs_uevent(struct mdinfo *sra, char *event)
 int sysfs_get_fd(struct mdinfo *sra, struct mdinfo *dev,
                       char *name)
 {
-       char fname[50];
+       char fname[PATH_MAX];
        int fd;

-       sprintf(fname, "/sys/block/%s/md/%s/%s",
+       snprintf(fname, PATH_MAX, "/sys/block/%s/md/%s/%s",
                sra->sys_name, dev?dev->sys_name:"", name);
        fd = open(fname, O_RDWR);
        if (fd < 0)
@@ -574,18 +569,18 @@ int sysfs_set_array(struct mdinfo *info, int vers)

 int sysfs_add_disk(struct mdinfo *sra, struct mdinfo *sd, int in_sync)
 {
-       char dv[100];
-       char nm[100];
+       char dv[PATH_MAX];
+       char nm[PATH_MAX];
        char *dname;
        int rv;

-       sprintf(dv, "%d:%d", sd->disk.major, sd->disk.minor);
+       snprintf(dv, PATH_MAX, "%d:%d", sd->disk.major, sd->disk.minor);
        rv = sysfs_set_str(sra, NULL, "new_dev", dv);
        if (rv)
                return rv;

        memset(nm, 0, sizeof(nm));
-       sprintf(dv, "/sys/dev/block/%d:%d", sd->disk.major, sd->disk.minor);
+       snprintf(dv, PATH_MAX, "/sys/dev/block/%d:%d", sd->disk.major, sd->disk.minor);
        rv = readlink(dv, nm, sizeof(nm));
        if (rv <= 0)
                return -1;
@@ -615,8 +610,8 @@ int sysfs_disk_to_sg(int fd)
         * scsi_generic interface
         */
        struct stat st;
-       char path[256];
-       char sg_path[256];
+       char path[PATH_MAX];
+       char sg_path[PATH_MAX];
        char sg_major_minor[8];
        char *c;
        DIR *dir;
@@ -626,7 +621,7 @@ int sysfs_disk_to_sg(int fd)
        if (fstat(fd, &st))
                return -1;

-       snprintf(path, sizeof(path), "/sys/dev/block/%d:%d/device",
+       snprintf(path, PATH_MAX, "/sys/dev/block/%d:%d/device",
                 major(st.st_rdev), minor(st.st_rdev));

        dir = opendir(path);
@@ -662,7 +657,7 @@ int sysfs_disk_to_sg(int fd)
        c++;
        major = strtol(sg_major_minor, NULL, 10);
        minor = strtol(c, NULL, 10);
-       snprintf(path, sizeof(path), "/dev/.tmp.md.%d:%d:%d",
+       snprintf(path, PATH_MAX, "/dev/.tmp.md.%d:%d:%d",
                 (int) getpid(), major, minor);
        if (mknod(path, S_IFCHR|0600, makedev(major, minor))==0) {
                        fd = open(path, O_RDONLY);
@@ -678,7 +673,7 @@ int sysfs_disk_to_scsi_id(int fd, __u32 *id)
 {
        /* from an open block device, try to retrieve it scsi_id */
        struct stat st;
-       char path[256];
+       char path[PATH_MAX];
        char *c1, *c2;
        DIR *dir;
        struct dirent *de;
@@ -686,7 +681,7 @@ int sysfs_disk_to_scsi_id(int fd, __u32 *id)
        if (fstat(fd, &st))
                return 1;

-       snprintf(path, sizeof(path), "/sys/dev/block/%d:%d/device",
+       snprintf(path, PATH_MAX, "/sys/dev/block/%d:%d/device",
                 major(st.st_rdev), minor(st.st_rdev));

        dir = opendir(path);
@@ -734,10 +729,10 @@ int sysfs_unique_holder(int devnum, long rdev)
         */
        DIR *dir;
        struct dirent *de;
-       char dirname[100];
+       char dirname[PATH_MAX];
        char l;
        int found = 0;
-       sprintf(dirname, "/sys/dev/block/%d:%d/holders",
+       snprintf(dirname, PATH_MAX, "/sys/dev/block/%d:%d/holders",
                major(rdev), minor(rdev));
        dir = opendir(dirname);
        errno = ENOENT;
diff --git a/util.c b/util.c
index a9e2504..95db3fa 100644
--- a/util.c
+++ b/util.c
@@ -1279,7 +1279,7 @@ int open_container(int fd)
        /* 'fd' is a block device.  Find out if it is in use
         * by a container, and return an open fd on that container.
         */
-       char path[256];
+       char path[PATH_MAX];
        char *e;
        DIR *dir;
        struct dirent *de;
@@ -1290,7 +1290,7 @@ int open_container(int fd)

        if (fstat(fd, &st) != 0)
                return -1;
-       sprintf(path, "/sys/dev/block/%d:%d/holders",
+       snprintf(path, PATH_MAX, "/sys/dev/block/%d:%d/holders",
                (int)major(st.st_rdev), (int)minor(st.st_rdev));
        e = path + strlen(path);

@@ -1399,8 +1399,8 @@ int devname2devnum(char *name)

 int stat2devnum(struct stat *st)
 {
-       char path[30];
-       char link[200];
+       char path[PATH_MAX];
+       char link[PATH_MAX];
        char *cp;
        int n;

@@ -1414,7 +1414,7 @@ int stat2devnum(struct stat *st)
                 * /sys/dev/block/%d:%d link which must look like
                 * ../../block/mdXXX/mdXXXpYY
                 */
-               sprintf(path, "/sys/dev/block/%d:%d", major(st->st_rdev),
+               snprintf(path, PATH_MAX, "/sys/dev/block/%d:%d", major(st->st_rdev),
                        minor(st->st_rdev));
                n = readlink(path, link, sizeof(link)-1);
                if (n <= 0)
@@ -1440,42 +1440,50 @@ int fd2devnum(int fd)

 int mdmon_running(int devnum)
 {
-       char path[100];
+       char path[PATH_MAX];
        char pid[10];
-       int fd;
-       int n;
-       sprintf(path, "/var/run/mdadm/%s.pid", devnum2devname(devnum));
+       int fd, n;
+       char *devname = devnum2devname(devnum);
+       if (devname) {
+               snprintf(path, PATH_MAX, "/var/run/mdadm/%s.pid", devname);
+               free(devname);
+       } else {
+               return 0;
+       }
        fd = open(path, O_RDONLY, 0);
-
-       if (fd < 0)
+       if (fd < 0) {
                return 0;
+       }
        n = read(fd, pid, 9);
        close(fd);
-       if (n <= 0)
+       if (n <= 0) {
                return 0;
-       if (kill(atoi(pid), 0) == 0)
-               return 1;
-       return 0;
+       }
+       return (kill(atoi(pid), 0) == 0);
 }

 int signal_mdmon(int devnum)
 {
-       char path[100];
+       char path[PATH_MAX];
        char pid[10];
-       int fd;
-       int n;
-       sprintf(path, "/var/run/mdadm/%s.pid", devnum2devname(devnum));
+       int fd, n;
+       char *devname = devnum2devname(devnum);
+       if (devname) {
+               sprintf(path, "/var/run/mdadm/%s.pid", devname);
+               free(devname);
+       } else {
+               return 0;
+       }
        fd = open(path, O_RDONLY, 0);
-
-       if (fd < 0)
+       if (fd < 0) {
                return 0;
+       }
        n = read(fd, pid, 9);
        close(fd);
-       if (n <= 0)
+       if (n <= 0) {
                return 0;
-       if (kill(atoi(pid), SIGUSR1) == 0)
-               return 1;
-       return 0;
+       }
+       return (kill(atoi(pid), SIGUSR1) == 0);
 }

 int start_mdmon(int devnum)
@@ -1484,7 +1492,7 @@ int start_mdmon(int devnum)
        int len;
        pid_t pid;
        int status;
-       char pathbuf[1024];
+       char pathbuf[PATH_MAX];
        char *paths[4] = {
                pathbuf,
                "/sbin/mdmon",
@@ -1495,7 +1503,7 @@ int start_mdmon(int devnum)
        if (check_env("MDADM_NO_MDMON"))
                return 0;

-       len = readlink("/proc/self/exe", pathbuf, sizeof(pathbuf));
+       len = readlink("/proc/self/exe", pathbuf, PATH_MAX);
        if (len > 0) {
                char *sl;
                pathbuf[len] = 0;

Re: [patch 1/1][mdadm] Fix needed to enable RAID device creation on SAS devices.

[Andre Noll]

[-- Attachment #1: Type: text/plain, Size: 1625 bytes --]

On 10:43, Wojcik, Artur wrote:
> -                               char path[200];
> +                               char path[PATH_MAX];

PATH_MAX might be undefined if we're not using glibc. AFAIK the best
one can do is something like

	#ifdef PATH_MAX
		path_max = PATH_MAX;
	#else
		/*
		 * The result of pathconf(3) may be huge and unsuitable for
		 * mallocing memory. OTOH pathconf(3) may return -1 to signify
		 * that PATH_MAX is not bounded.
		 */
		path_max = pathconf(name, _PC_PATH_MAX);
		if (path_max <= 0 || path_max >= 4096)
			path_max = 4096;
	#endif


>                                 char vbuf[1024];
>                                 int nlen = strlen(sra->sys_name);
>                                 int dn;
>                                 if (de->d_name[0] == '.')
>                                         continue;
> -                               sprintf(path, "/sys/block/%s/md/metadata_version",
> +                               snprintf(path, PATH_MAX, "/sys/block/%s/md/metadata_version",
>                                         de->d_name);
>                                 if (load_sys(path, vbuf) < 0)
>                                         continue;

If you are really paranoid, you have to terminate the buffer when
using snprintf. Moreover, one should prefer sizeof(path) over the
explicit PATH_MAX because if you ever change the size of path again,
you avoid the risk to change it only in the definition of path but
not in the snprintf() call.

Best
Andre
-- 
The only person who always got his work done by Friday was Robinson Crusoe

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [patch 0/1][mdadm] Fix needed to enable RAID volumes on SAS devices (version 2).

[Artur Wojcik]

Neil/Andre,
I incorporated the suggestions Andre gave me and this resulted in the
second version of patch. However I decided to not use pathconf()
function but to define PATH_MAX instead. It is just the size of a buffer
and I would like this to be resolved during compilation time rather then
dynamically during execution time. Please review it and let me know if
something needs to be changed.

As for the being paranoid... so I really am, if we talk about software
security and vulnerability. I decided to introduce a new __str_fmt()
function and str_fmt() helper macro. Both are documented in source code
(let me know if you need separate patch for this).

-------------------------------------------------

This fix allows to create a RAID arrays on SAS devices direct-attached
or installed behind an expander. The cause of the problem is assumption
that 50 characters is enough to fit sysfs path of a storage device. This
is true for devices connected to AHCI and IDE controllers. However for
SAS devices (especially installed behind an expander) 'libsas' may
create a path much longer then 50 characters and in consequence the path
is truncated and incorrectly evaluated.
---
 Detail.c         |    5 ++-
 platform-intel.c |   19 ++++++++-------
 super-intel.c    |   27 ++++++++-------------
 sysfs.c          |   59 ++++++++++++++++++++++-------------------------
 util.c           |   55 +++++++++++++++++++++++++++++++++++---------
 util.h           |   67
++++++++++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 162 insertions(+), 70 deletions(-)
 create mode 100644 util.h
---
Artur

Re: [patch 1/1][mdadm] Fix needed to enable RAID volumes on SAS devices (version 2).

[Artur Wojcik]

diff --git a/Detail.c b/Detail.c
index e2cf028..377f75d 100644
--- a/Detail.c
+++ b/Detail.c
@@ -26,6 +26,7 @@
 #include	"md_p.h"
 #include	"md_u.h"
 #include	<dirent.h>
+#include	"util.h"
 
 int Detail(char *dev, int brief, int export, int test, char *homehost)
 {
@@ -408,13 +409,13 @@ This is pretty boring
 			printf("  Member Arrays :");
 
 			while (dir && (de = readdir(dir)) != NULL) {
-				char path[200];
+				char path[PATH_MAX];
 				char vbuf[1024];
 				int nlen = strlen(sra->sys_name);
 				int dn;
 				if (de->d_name[0] == '.')
 					continue;
-				sprintf(path, "/sys/block/%s/md/metadata_version",
+				str_fmt(path, "/sys/block/%s/md/metadata_version",
 					de->d_name);
 				if (load_sys(path, vbuf) < 0)
 					continue;
diff --git a/platform-intel.c b/platform-intel.c
index d568ca6..c61b2f1 100644
--- a/platform-intel.c
+++ b/platform-intel.c
@@ -28,6 +28,7 @@
 #include <sys/mman.h>
 #include <sys/types.h>
 #include <sys/stat.h>
+#include "util.h"
 
 void free_sys_dev(struct sys_dev **list)
 {
@@ -44,15 +45,15 @@ void free_sys_dev(struct sys_dev **list)
 struct sys_dev *find_driver_devices(const char *bus, const char
*driver)
 {
 	/* search sysfs for devices driven by 'driver' */
-	char path[256];
-	char link[256];
+	char path[PATH_MAX];
+	char link[PATH_MAX];
 	char *c;
 	DIR *driver_dir;
 	struct dirent *de;
 	struct sys_dev *head = NULL;
 	struct sys_dev *list = NULL;
 
-	sprintf(path, "/sys/bus/%s/drivers/%s", bus, driver);
+	str_fmt(path, "/sys/bus/%s/drivers/%s", bus, driver);
 	driver_dir = opendir(path);
 	if (!driver_dir)
 		return NULL;
@@ -60,7 +61,7 @@ struct sys_dev *find_driver_devices(const char *bus,
const char *driver)
 		/* is 'de' a device? check that the 'subsystem' link exists and
 		 * that its target matches 'bus'
 		 */
-		sprintf(path, "/sys/bus/%s/drivers/%s/%s/subsystem",
+		str_fmt(path, "/sys/bus/%s/drivers/%s/%s/subsystem",
 			bus, driver, de->d_name);
 		if (readlink(path, link, sizeof(link)) < 0)
 			continue;
@@ -85,7 +86,7 @@ struct sys_dev *find_driver_devices(const char *bus,
const char *driver)
 		}
 
 		/* generate canonical path name for the device */
-		sprintf(path, "/sys/bus/%s/drivers/%s/%s",
+		str_fmt(path, "/sys/bus/%s/drivers/%s/%s",
 			bus, driver, de->d_name);
 		list->path = canonicalize_file_name(path);
 		list->next = NULL;
@@ -96,13 +97,13 @@ struct sys_dev *find_driver_devices(const char *bus,
const char *driver)
 
 __u16 devpath_to_vendor(const char *dev_path)
 {
-	char path[strlen(dev_path) + strlen("/vendor") + 1];
+	char path[PATH_MAX];
 	char vendor[7];
 	int fd;
 	__u16 id = 0xffff;
 	int n;
 
-	sprintf(path, "%s/vendor", dev_path);
+	str_fmt(path, "%s/vendor", dev_path);
 
 	fd = open(path, O_RDONLY);
 	if (fd < 0)
@@ -202,9 +203,9 @@ const struct imsm_orom *find_imsm_orom(void)
 
 char *devt_to_devpath(dev_t dev)
 {
-	char device[40];
+	char device[PATH_MAX];
 
-	sprintf(device, "/sys/dev/block/%d:%d/device", major(dev),
minor(dev));
+	str_fmt(device, "/sys/dev/block/%d:%d/device", major(dev),
minor(dev));
 	return canonicalize_file_name(device);
 }
 
diff --git a/super-intel.c b/super-intel.c
index 9a99d60..7991295 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -26,6 +26,7 @@
 #include <scsi/sg.h>
 #include <ctype.h>
 #include <dirent.h>
+#include "util.h"
 
 /* MPB == Metadata Parameter Block */
 #define MPB_SIGNATURE "Intel Raid ISM Cfg Sig. "
@@ -882,7 +883,7 @@ static int imsm_enumerate_ports(const char
*hba_path, int port_count, int host_b
 		char vendor[64];
 		char buf[1024];
 		int major, minor;
-		char *device;
+		char device[PATH_MAX];
 		char *c;
 		int port;
 		int type;
@@ -899,19 +900,12 @@ static int imsm_enumerate_ports(const char
*hba_path, int port_count, int host_b
 		}
 
 		/* retrieve the scsi device type */
-		if (asprintf(&device, "/sys/dev/block/%d:%d/device/xxxxxxx", major,
minor) < 0) {
-			if (verbose)
-				fprintf(stderr, Name ": failed to allocate 'device'\n");
-			err = 2;
-			break;
-		}
-		sprintf(device, "/sys/dev/block/%d:%d/device/type", major, minor);
+		str_fmt(device, "/sys/dev/block/%d:%d/device/type", major, minor);
 		if (load_sys(device, buf) != 0) {
 			if (verbose)
 				fprintf(stderr, Name ": failed to read device type for %s\n",
 					path);
 			err = 2;
-			free(device);
 			break;
 		}
 		type = strtoul(buf, NULL, 10);
@@ -920,7 +914,7 @@ static int imsm_enumerate_ports(const char
*hba_path, int port_count, int host_b
 		if (!(type == 0 || type == 7 || type == 14)) {
 			vendor[0] = '\0';
 			model[0] = '\0';
-			sprintf(device, "/sys/dev/block/%d:%d/device/vendor", major, minor);
+			str_fmt(device, "/sys/dev/block/%d:%d/device/vendor", major, minor);
 			if (load_sys(device, buf) == 0) {
 				strncpy(vendor, buf, sizeof(vendor));
 				vendor[sizeof(vendor) - 1] = '\0';
@@ -929,7 +923,7 @@ static int imsm_enumerate_ports(const char
*hba_path, int port_count, int host_b
 					*c-- = '\0';
 
 			}
-			sprintf(device, "/sys/dev/block/%d:%d/device/model", major, minor);
+			str_fmt(device, "/sys/dev/block/%d:%d/device/model", major, minor);
 			if (load_sys(device, buf) == 0) {
 				strncpy(model, buf, sizeof(model));
 				model[sizeof(model) - 1] = '\0';
@@ -953,10 +947,9 @@ static int imsm_enumerate_ports(const char
*hba_path, int port_count, int host_b
 				case 12: sprintf(buf, "raid"); break;
 				default: sprintf(buf, "unknown");
 				}
-		} else
+		} else {
 			buf[0] = '\0';
-		free(device);
-
+		}
 		/* chop device path to 'host%d' and calculate the port number */
 		c = strchr(&path[hba_len], '/');
 		*c = '\0';
@@ -1576,15 +1569,15 @@ static int compare_super_imsm(struct supertype
*st, struct supertype *tst)
 static void fd2devname(int fd, char *name)
 {
 	struct stat st;
-	char path[256];
-	char dname[100];
+	char path[PATH_MAX];
+	char dname[PATH_MAX];
 	char *nm;
 	int rv;
 
 	name[0] = '\0';
 	if (fstat(fd, &st) != 0)
 		return;
-	sprintf(path, "/sys/dev/block/%d:%d",
+	str_fmt(path, "/sys/dev/block/%d:%d",
 		major(st.st_rdev), minor(st.st_rdev));
 
 	rv = readlink(path, dname, sizeof(dname));
diff --git a/sysfs.c b/sysfs.c
index 35dfbd4..a51cfb6 100644
--- a/sysfs.c
+++ b/sysfs.c
@@ -26,6 +26,7 @@
 #include	"mdadm.h"
 #include	<dirent.h>
 #include	<ctype.h>
+#include	"util.h"
 
 int load_sys(char *path, char *buf)
 {
@@ -59,14 +60,14 @@ void sysfs_free(struct mdinfo *sra)
 
 int sysfs_open(int devnum, char *devname, char *attr)
 {
-	char fname[50];
+	char fname[PATH_MAX];
 	int fd;
 	char *mdname = devnum2devname(devnum);
 
 	if (!mdname)
 		return -1;
 
-	sprintf(fname, "/sys/block/%s/md/", mdname);
+	str_fmt(fname, "/sys/block/%s/md/", mdname);
 	if (devname) {
 		strcat(fname, devname);
 		strcat(fname, "/");
@@ -100,12 +101,7 @@ void sysfs_init(struct mdinfo *mdi, int fd, int
devnum)
 
 struct mdinfo *sysfs_read(int fd, int devnum, unsigned long options)
 {
-	/* Longest possible name in sysfs, mounted at /sys, is
-	 *  /sys/block/md_dXXX/md/dev-XXXXX/block/dev
-	 *  /sys/block/md_dXXX/md/metadata_version
-	 * which is about 41 characters.  50 should do for now
-	 */
-	char fname[50];
+	char fname[PATH_MAX];
 	char buf[1024];
 	char *base;
 	char *dbase;
@@ -124,7 +120,7 @@ struct mdinfo *sysfs_read(int fd, int devnum,
unsigned long options)
 		return NULL;
 	}
 
-	sprintf(fname, "/sys/block/%s/md/", sra->sys_name);
+	str_fmt(fname, "/sys/block/%s/md/", sra->sys_name);
 	base = fname + strlen(fname);
 
 	sra->devs = NULL;
@@ -376,14 +372,14 @@ unsigned long long get_component_size(int fd)
 	 * This returns in units of sectors.
 	 */
 	struct stat stb;
-	char fname[50];
+	char fname[PATH_MAX];
 	int n;
 	if (fstat(fd, &stb)) return 0;
 	if (major(stb.st_rdev) != get_mdp_major())
-		sprintf(fname, "/sys/block/md%d/md/component_size",
+		str_fmt(fname, "/sys/block/md%d/md/component_size",
 			(int)minor(stb.st_rdev));
 	else
-		sprintf(fname, "/sys/block/md_d%d/md/component_size",
+		str_fmt(fname, "/sys/block/md_d%d/md/component_size",
 			(int)minor(stb.st_rdev)>>MdpMinorShift);
 	fd = open(fname, O_RDONLY);
 	if (fd < 0)
@@ -399,11 +395,11 @@ unsigned long long get_component_size(int fd)
 int sysfs_set_str(struct mdinfo *sra, struct mdinfo *dev,
 		  char *name, char *val)
 {
-	char fname[50];
+	char fname[PATH_MAX];
 	int n;
 	int fd;
 
-	sprintf(fname, "/sys/block/%s/md/%s/%s",
+	str_fmt(fname, "/sys/block/%s/md/%s/%s",
 		sra->sys_name, dev?dev->sys_name:"", name);
 	fd = open(fname, O_WRONLY);
 	if (fd < 0)
@@ -428,11 +424,11 @@ int sysfs_set_num(struct mdinfo *sra, struct
mdinfo *dev,
 
 int sysfs_uevent(struct mdinfo *sra, char *event)
 {
-	char fname[50];
+	char fname[PATH_MAX];
 	int n;
 	int fd;
 
-	sprintf(fname, "/sys/block/%s/uevent",
+	str_fmt(fname, "/sys/block/%s/uevent",
 		sra->sys_name);
 	fd = open(fname, O_WRONLY);
 	if (fd < 0)
@@ -445,10 +441,10 @@ int sysfs_uevent(struct mdinfo *sra, char *event)
 int sysfs_get_fd(struct mdinfo *sra, struct mdinfo *dev,
 		       char *name)
 {
-	char fname[50];
+	char fname[PATH_MAX];
 	int fd;
 
-	sprintf(fname, "/sys/block/%s/md/%s/%s",
+	str_fmt(fname, "/sys/block/%s/md/%s/%s",
 		sra->sys_name, dev?dev->sys_name:"", name);
 	fd = open(fname, O_RDWR);
 	if (fd < 0)
@@ -574,18 +570,18 @@ int sysfs_set_array(struct mdinfo *info, int vers)
 
 int sysfs_add_disk(struct mdinfo *sra, struct mdinfo *sd, int in_sync)
 {
-	char dv[100];
-	char nm[100];
+	char dv[PATH_MAX];
+	char nm[PATH_MAX];
 	char *dname;
 	int rv;
 
-	sprintf(dv, "%d:%d", sd->disk.major, sd->disk.minor);
+	str_fmt(dv, "%d:%d", sd->disk.major, sd->disk.minor);
 	rv = sysfs_set_str(sra, NULL, "new_dev", dv);
 	if (rv)
 		return rv;
 
 	memset(nm, 0, sizeof(nm));
-	sprintf(dv, "/sys/dev/block/%d:%d", sd->disk.major, sd->disk.minor);
+	str_fmt(dv, "/sys/dev/block/%d:%d", sd->disk.major, sd->disk.minor);
 	rv = readlink(dv, nm, sizeof(nm));
 	if (rv <= 0)
 		return -1;
@@ -615,8 +611,8 @@ int sysfs_disk_to_sg(int fd)
 	 * scsi_generic interface
 	 */
 	struct stat st;
-	char path[256];
-	char sg_path[256];
+	char path[PATH_MAX];
+	char sg_path[PATH_MAX];
 	char sg_major_minor[8];
 	char *c;
 	DIR *dir;
@@ -626,7 +622,7 @@ int sysfs_disk_to_sg(int fd)
 	if (fstat(fd, &st))
 		return -1;
 
-	snprintf(path, sizeof(path), "/sys/dev/block/%d:%d/device",
+	str_fmt(path, "/sys/dev/block/%d:%d/device",
 		 major(st.st_rdev), minor(st.st_rdev));
 
 	dir = opendir(path);
@@ -645,7 +641,7 @@ int sysfs_disk_to_sg(int fd)
 	if (!de)
 		return -1;
 
-	snprintf(sg_path, sizeof(sg_path), "%s/%s/dev", path, de->d_name);
+	str_fmt(sg_path, "%s/%s/dev", path, de->d_name);
 	fd = open(sg_path, O_RDONLY);
 	if (fd < 0)
 		return fd;
@@ -662,7 +658,7 @@ int sysfs_disk_to_sg(int fd)
 	c++;
 	major = strtol(sg_major_minor, NULL, 10);
 	minor = strtol(c, NULL, 10);
-	snprintf(path, sizeof(path), "/dev/.tmp.md.%d:%d:%d",
+	str_fmt(path, "/dev/.tmp.md.%d:%d:%d",
 		 (int) getpid(), major, minor);
 	if (mknod(path, S_IFCHR|0600, makedev(major, minor))==0) {
 			fd = open(path, O_RDONLY);
@@ -678,7 +674,7 @@ int sysfs_disk_to_scsi_id(int fd, __u32 *id)
 {
 	/* from an open block device, try to retrieve it scsi_id */
 	struct stat st;
-	char path[256];
+	char path[PATH_MAX];
 	char *c1, *c2;
 	DIR *dir;
 	struct dirent *de;
@@ -686,7 +682,7 @@ int sysfs_disk_to_scsi_id(int fd, __u32 *id)
 	if (fstat(fd, &st))
 		return 1;
 
-	snprintf(path, sizeof(path), "/sys/dev/block/%d:%d/device",
+	str_fmt(path, "/sys/dev/block/%d:%d/device",
 		 major(st.st_rdev), minor(st.st_rdev));
 
 	dir = opendir(path);
@@ -734,10 +730,10 @@ int sysfs_unique_holder(int devnum, long rdev)
 	 */
 	DIR *dir;
 	struct dirent *de;
-	char dirname[100];
+	char dirname[PATH_MAX];
 	char l;
 	int found = 0;
-	sprintf(dirname, "/sys/dev/block/%d:%d/holders",
+	str_fmt(dirname, "/sys/dev/block/%d:%d/holders",
 		major(rdev), minor(rdev));
 	dir = opendir(dirname);
 	errno = ENOENT;
@@ -892,3 +888,4 @@ int WaitClean(char *dev, int sock, int verbose)
 	return rv;
 }
 #endif /* MDASSEMBLE */
+
diff --git a/util.c b/util.c
index 048c39f..cdae3b4 100644
--- a/util.c
+++ b/util.c
@@ -3,7 +3,6 @@
  *
  * Copyright (C) 2001-2009 Neil Brown <neilb@suse.de>
  *
- *
  *    This program is free software; you can redistribute it and/or
modify
  *    it under the terms of the GNU General Public License as published
by
  *    the Free Software Foundation; either version 2 of the License, or
@@ -31,6 +30,7 @@
 #include	<ctype.h>
 #include	<dirent.h>
 #include	<signal.h>
+#include	"util.h"
 
 /*
  * following taken from linux/blkpg.h because they aren't
@@ -1110,7 +1110,7 @@ int open_container(int fd)
 	/* 'fd' is a block device.  Find out if it is in use
 	 * by a container, and return an open fd on that container.
 	 */
-	char path[256];
+	char path[PATH_MAX];
 	char *e;
 	DIR *dir;
 	struct dirent *de;
@@ -1121,7 +1121,7 @@ int open_container(int fd)
 
 	if (fstat(fd, &st) != 0)
 		return -1;
-	sprintf(path, "/sys/dev/block/%d:%d/holders",
+	str_fmt(path, "/sys/dev/block/%d:%d/holders",
 		(int)major(st.st_rdev), (int)minor(st.st_rdev));
 	e = path + strlen(path);
 
@@ -1230,8 +1230,8 @@ int devname2devnum(char *name)
 
 int stat2devnum(struct stat *st)
 {
-	char path[30];
-	char link[200];
+	char path[PATH_MAX];
+	char link[PATH_MAX];
 	char *cp;
 	int n;
 
@@ -1245,7 +1245,7 @@ int stat2devnum(struct stat *st)
 		 * /sys/dev/block/%d:%d link which must look like
 		 * ../../block/mdXXX/mdXXXpYY
 		 */
-		sprintf(path, "/sys/dev/block/%d:%d", major(st->st_rdev),
+		str_fmt(path, "/sys/dev/block/%d:%d", major(st->st_rdev),
 			minor(st->st_rdev));
 		n = readlink(path, link, sizeof(link)-1);
 		if (n <= 0)
@@ -1271,11 +1271,11 @@ int fd2devnum(int fd)
 
 int mdmon_running(int devnum)
 {
-	char path[100];
+	char path[PATH_MAX];
 	char pid[10];
 	int fd;
 	int n;
-	sprintf(path, "/var/run/mdadm/%s.pid", devnum2devname(devnum));
+	str_fmt(path, "/var/run/mdadm/%s.pid", devnum2devname(devnum));
 	fd = open(path, O_RDONLY, 0);
 
 	if (fd < 0)
@@ -1291,11 +1291,11 @@ int mdmon_running(int devnum)
 
 int signal_mdmon(int devnum)
 {
-	char path[100];
+	char path[PATH_MAX];
 	char pid[10];
 	int fd;
 	int n;
-	sprintf(path, "/var/run/mdadm/%s.pid", devnum2devname(devnum));
+	str_fmt(path, "/var/run/mdadm/%s.pid", devnum2devname(devnum));
 	fd = open(path, O_RDONLY, 0);
 
 	if (fd < 0)
@@ -1315,7 +1315,7 @@ int start_mdmon(int devnum)
 	int len;
 	pid_t pid;	
 	int status;
-	char pathbuf[1024];
+	char pathbuf[PATH_MAX];
 	char *paths[4] = {
 		pathbuf,
 		"/sbin/mdmon",
@@ -1329,6 +1329,9 @@ int start_mdmon(int devnum)
 	len = readlink("/proc/self/exe", pathbuf, sizeof(pathbuf));
 	if (len > 0) {
 		char *sl;
+		if (len >= sizeof(pathbuf)) {
+			len = sizeof(pathbuf) - 1;
+		}
 		pathbuf[len] = 0;
 		sl = strrchr(pathbuf, '/');
 		if (sl)
@@ -1425,6 +1428,36 @@ void append_metadata_update(struct supertype *st,
void *buf, int len)
 }
 #endif /* MDASSEMBLE */
 
+/* Copyright (C) 2009 Intel Corporation. All rights reserved.
+ *
+ * This function formats a string according to format pattern. The
buffer is
+ * always null terminated even if source string does not fit in
destination 
+ * buffer. The function returns -1 in case of an error and this means 
+ * either one of the input parameters is NULL or there's not enough
space in 
+ * destination buffer to fit even a single character. Otherwise the
function 
+ * returns the number of character put in the destination buffer.
+ */
+int __str_fmt(char *buf, size_t buf_size, const char *fmt, ...)
+{
+	va_list vl;
+
+	if (((int)(--buf_size)) <= 0) {
+		errno = ENOBUFS;
+		return -1;
+	}
+	if ((buf == NULL) || (fmt == NULL)) {
+		errno = EINVAL;
+		return -1;
+	}
+	va_start(vl, fmt);
+	int result = vsnprintf(buf, buf_size, fmt, vl);
+	va_end(vl);
+	if ((result < 0) || (result >= buf_size)) {
+		buf[result = buf_size] = '\0';
+	}
+	return result;
+}
+
 #ifdef __TINYC__
 /* tinyc doesn't optimize this check in ioctl.h out ... */
 unsigned int __invalid_size_argument_for_IOC = 0;
diff --git a/util.h b/util.h
new file mode 100644
index 0000000..4be9bc8
--- /dev/null
+++ b/util.h
@@ -0,0 +1,67 @@
+/*
+ * Linux RAID Management Application
+ * Copyright (c) 2009 Intel Corporation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
it
+ * under the terms of the GNU General Public License version 2 as
published
+ * by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warrany of
MERCHANTABILITY
+ * or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public
License
+ * for more details.
+ *
+ * You should have received a copy of the GNU General Public License
along
+ * with this program; if not, write to the Free Software Foundation,
Inc.,
+ * 51 Franklin Street, Fifhth Floor, Boston, MA 02110-1301, USA.
+ */
+
+#ifndef _UTIL_H_INCLUDED
+#define _UTIL_H_INCLUDED
+
+/* Define PATH_MAX in case we don't use GLIBC or the standard library
does 
+   not have PATH_MAX defined. Assume maximum path length is 4K
characters. */
+#ifndef PATH_MAX
+#define PATH_MAX      4096
+#endif /* PATH_MAX */
+
+/**
+ * @brief Wrapper macro for __str_fmt function.
+ *
+ * This macro makes use of __str_fmt() function easier. Use this macro
with 
+ * caution and only for arrays. Do not use this macro with pointers
because 
+ * the result might be unexpected.
+ *
+ * @param[in]     __buf          Destination buffer.
+ * @param[in]     __fmt          Format string.
+ *
+ * @return The return value of this marco is the same as for
__str_fmt() 
+ *         function. See description of __str_fmt() for more details.
+ */
+#define str_fmt(__buf, __fmt, ...) \
+	__str_fmt((char *)(__buf), sizeof(__buf), (const char *)(__fmt), ##
__VA_ARGS__)
+
+/**
+ * @brief Formats a string according to pattern.
+ *
+ * This is printf() like function which formats a text buffer according
to
+ * format pattern. The function stores the result of formating in a
destination
+ * buffer. The destrination buffer is always null terminated even if
result 
+ * does not fit in it. See description of printf() function for details
on how
+ * to format the output.
+ *
+ * @param[in]     buf            Pointer to destination buffer where
the 
+ *                               result of formating will be stored.
+ * @param[in]     buf_size       The capacity of destination buffer
including
+ *                               null ('\0') character.
+ * @param[in]     fmt            Pointer to buffer containging the
pattern.
+ *
+ * @return If successful the function returns number of characters put
in 
+ *         destination buffer, otherwise the function returns -1. Check
errno
+ *         variable to get details about the cause of an error.
+ */
+extern int __str_fmt(char *buf, size_t buf_size, const char *fmt, ...)
+	__attribute__((format(printf, 3, 4)));
+
+#endif /* _UTIL_H_INCLUDED */
+
-- 
1.6.3.3

Re: [patch 1/1][mdadm] Fix needed to enable RAID volumes on SAS devices (version 2).

[Dan Williams]

Hi Artur,

This patch has whitespace damage.    If you use Evolution to insert
the patch make sure you use the "Preformat" formatting option.  A
better alternative is to use the "stg mail" command from Stacked GIT
which, among other things, will format the patch according to akpm's
"perfect patch" guidelines [1].

Some comments below:

> @@ -899,19 +900,12 @@ static int imsm_enumerate_ports(const char
> *hba_path, int port_count, int host_b
>                }
>
>                /* retrieve the scsi device type */
> -               if (asprintf(&device, "/sys/dev/block/%d:%d/device/xxxxxxx", major,
> minor) < 0) {
> -                       if (verbose)
> -                               fprintf(stderr, Name ": failed to allocate 'device'\n");
> -                       err = 2;
> -                       break;
> -               }
> -               sprintf(device, "/sys/dev/block/%d:%d/device/type", major, minor);
> +               str_fmt(device, "/sys/dev/block/%d:%d/device/type", major, minor);

Admittedly this change isn't needed because asprintf has guaranteed
that the buffer is large enough, but I can see why you made the change
if the goal is eradication of all sprintf calls.

> @@ -1425,6 +1428,36 @@ void append_metadata_update(struct supertype *st,
> void *buf, int len)
>  }
>  #endif /* MDASSEMBLE */
>
> +/* Copyright (C) 2009 Intel Corporation. All rights reserved.
> + *

It is not conventional to claim copyright on a per function basis.

> + * This function formats a string according to format pattern. The
> buffer is
> + * always null terminated even if source string does not fit in
> destination
> + * buffer. The function returns -1 in case of an error and this means
> + * either one of the input parameters is NULL or there's not enough
> space in
> + * destination buffer to fit even a single character. Otherwise the
> function
> + * returns the number of character put in the destination buffer.
> + */
> +int __str_fmt(char *buf, size_t buf_size, const char *fmt, ...)
> +{
> +       va_list vl;
> +
> +       if (((int)(--buf_size)) <= 0) {

This seems wrong.  Why check buf_size? Just let the normal return
value from vnsprintf indicate if the buffer is too small.  Also it
clips potentially valid sizes that appear negative when casting from
size_t to int.

> +               errno = ENOBUFS;
> +               return -1;
> +       }
> +       if ((buf == NULL) || (fmt == NULL)) {
> +               errno = EINVAL;
> +               return -1;
> +       }
> +       va_start(vl, fmt);
> +       int result = vsnprintf(buf, buf_size, fmt, vl);
> +       va_end(vl);
> +       if ((result < 0) || (result >= buf_size)) {
> +               buf[result = buf_size] = '\0';

Move that assignment to its own line.  In general mdadm tends to
follow kernel coding standards, Neil, of course feel free to clarify.

> +       }
> +       return result;
> +}
> +
>  #ifdef __TINYC__
>  /* tinyc doesn't optimize this check in ioctl.h out ... */
>  unsigned int __invalid_size_argument_for_IOC = 0;
> diff --git a/util.h b/util.h
> new file mode 100644
> index 0000000..4be9bc8
> --- /dev/null
> +++ b/util.h
> @@ -0,0 +1,67 @@
> +/*
> + * Linux RAID Management Application
> + * Copyright (c) 2009 Intel Corporation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> it
> + * under the terms of the GNU General Public License version 2 as
> published
> + * by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warrany of
> MERCHANTABILITY
> + * or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public
> License
> + * for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> along
> + * with this program; if not, write to the Free Software Foundation,
> Inc.,
> + * 51 Franklin Street, Fifhth Floor, Boston, MA 02110-1301, USA.
> + */
> +
> +#ifndef _UTIL_H_INCLUDED
> +#define _UTIL_H_INCLUDED
> +
> +/* Define PATH_MAX in case we don't use GLIBC or the standard library
> does
> +   not have PATH_MAX defined. Assume maximum path length is 4K
> characters. */
> +#ifndef PATH_MAX
> +#define PATH_MAX      4096
> +#endif /* PATH_MAX */
> +
> +/**
> + * @brief Wrapper macro for __str_fmt function.
> + *
> + * This macro makes use of __str_fmt() function easier. Use this macro
> with
> + * caution and only for arrays. Do not use this macro with pointers
> because
> + * the result might be unexpected.
> + *
> + * @param[in]     __buf          Destination buffer.
> + * @param[in]     __fmt          Format string.
> + *
> + * @return The return value of this marco is the same as for
> __str_fmt()
> + *         function. See description of __str_fmt() for more details.
> + */
> +#define str_fmt(__buf, __fmt, ...) \
> +       __str_fmt((char *)(__buf), sizeof(__buf), (const char *)(__fmt), ##
> __VA_ARGS__)
> +
> +/**
> + * @brief Formats a string according to pattern.
> + *
> + * This is printf() like function which formats a text buffer according
> to
> + * format pattern. The function stores the result of formating in a
> destination
> + * buffer. The destrination buffer is always null terminated even if
> result
> + * does not fit in it. See description of printf() function for details
> on how
> + * to format the output.
> + *
> + * @param[in]     buf            Pointer to destination buffer where
> the
> + *                               result of formating will be stored.
> + * @param[in]     buf_size       The capacity of destination buffer
> including
> + *                               null ('\0') character.
> + * @param[in]     fmt            Pointer to buffer containging the
> pattern.
> + *
> + * @return If successful the function returns number of characters put
> in
> + *         destination buffer, otherwise the function returns -1. Check
> errno
> + *         variable to get details about the cause of an error.
> + */
> +extern int __str_fmt(char *buf, size_t buf_size, const char *fmt, ...)
> +       __attribute__((format(printf, 3, 4)));

I don't like that this function silently does not work with pointers,
and its name belies the fact that it does checking in addition to
formatting.

Perhaps something can be done with gcc builtins for this case.  Have
you looked into __builtin_object_size, __builtin___snprintf_chk and
friends.  The goal being to use these builtins to:

1/ Get a compile time warning when an overflow is detected (currently
supported by the builtins)
2/ Get a compile time warning if the bounds cannot be checked at
compile time (would need some investigation)

--
Dan

[1] http://userweb.kernel.org/~akpm/stuff/tpp.txt
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch 1/1][mdadm] Fix needed to enable RAID volumes on SAS devices (version 2).

[Artur Wojcik]

Hi Dan,
Thank you for your comments. All you suggestions I will incorporate in
next version of a patch. Please see my comments below.

> better alternative is to use the "stg mail" command from Stacked GIT
> which, among other things, will format the patch according to akpm's
> "perfect patch" guidelines [1].

Definitely I should follow your suggestion.

> > @@ -899,19 +900,12 @@ static int imsm_enumerate_ports(const char
> > *hba_path, int port_count, int host_b
> >                }
> >
> >                /* retrieve the scsi device type */
> > -               if (asprintf(&device, "/sys/dev/block/%d:%d/device/xxxxxxx", major,
> > minor) < 0) {
> > -                       if (verbose)
> > -                               fprintf(stderr, Name ": failed to allocate 'device'\n");
> > -                       err = 2;
> > -                       break;
> > -               }
> > -               sprintf(device, "/sys/dev/block/%d:%d/device/type", major, minor);
> > +               str_fmt(device, "/sys/dev/block/%d:%d/device/type", major, minor);
> 
> Admittedly this change isn't needed because asprintf has guaranteed
> that the buffer is large enough, but I can see why you made the change
> if the goal is eradication of all sprintf calls.

Yes, that is my intention and if you are ok with this I would keep it.

> > @@ -1425,6 +1428,36 @@ void append_metadata_update(struct supertype *st,
> > void *buf, int len)
> >  }
> >  #endif /* MDASSEMBLE */
> >
> > +/* Copyright (C) 2009 Intel Corporation. All rights reserved.
> > + *
> 
> It is not conventional to claim copyright on a per function basis.

These are Intel guidelines, but I clarify this with Intel SQA.

> > + * This function formats a string according to format pattern. The
> > buffer is
> > + * always null terminated even if source string does not fit in
> > destination
> > + * buffer. The function returns -1 in case of an error and this means
> > + * either one of the input parameters is NULL or there's not enough
> > space in
> > + * destination buffer to fit even a single character. Otherwise the
> > function
> > + * returns the number of character put in the destination buffer.
> > + */
> > +int __str_fmt(char *buf, size_t buf_size, const char *fmt, ...)
> > +{
> > +       va_list vl;
> > +
> > +       if (((int)(--buf_size)) <= 0) {
> 
> This seems wrong.  Why check buf_size? Just let the normal return
> value from vnsprintf indicate if the buffer is too small.  Also it
> clips potentially valid sizes that appear negative when casting from
> size_t to int.

You right. I assumed no one will request buffers bigger then INT_MAX,
but this is wrong assumption. I agree the better solution is to let
vnsprintf() function to resolve this issue.

> > +extern int __str_fmt(char *buf, size_t buf_size, const char *fmt, ...)
> > +       __attribute__((format(printf, 3, 4)));
> 
> I don't like that this function silently does not work with pointers,
> and its name belies the fact that it does checking in addition to
> formatting.
> 
> Perhaps something can be done with gcc builtins for this case.  Have
> you looked into __builtin_object_size, __builtin___snprintf_chk and
> friends.  The goal being to use these builtins to:
> 
> 1/ Get a compile time warning when an overflow is detected (currently
> supported by the builtins)
> 2/ Get a compile time warning if the bounds cannot be checked at
> compile time (would need some investigation)

I will investigate this and correct the patch.
--
Artur

Re: [patch 0/1][mdadm] Fix needed to enable RAID volumes on SAS devices (version 2).

[Neil Brown]

On Mon, 30 Nov 2009 16:12:13 +0100
Artur Wojcik <artur.wojcik@intel.com> wrote:

> Neil/Andre,
> I incorporated the suggestions Andre gave me and this resulted in the
> second version of patch. However I decided to not use pathconf()
> function but to define PATH_MAX instead. It is just the size of a buffer
> and I would like this to be resolved during compilation time rather then
> dynamically during execution time. Please review it and let me know if
> something needs to be changed.
> 
> As for the being paranoid... so I really am, if we talk about software
> security and vulnerability. I decided to introduce a new __str_fmt()
> function and str_fmt() helper macro. Both are documented in source code
> (let me know if you need separate patch for this).
> 

I'm afraid that I really don't like this.
Creating little functions that do almost-but-not-quite-exactly what some
standard library function does always bothers me.  It makes the code harder
to read.
If you just used it in 2 or 3 places where you actually needed it then it
might be OK, but you have used it everywhere, including several places where
it isn't needed at all as the buffer is known to be big enough.

And I don't like the introduction of a new header file just to store one or 2
definitions.  Just put new definitions in mdadm.h  Keep It Simple.

I am perfectly happy with making 'buf' larger as appropriate, even making it
PATH_MAX in some cases.
I would be happy with more use of asprinf.
I would be happy adding checks before certain critical sprintf calls that
the result will not exceed the buffer.

But I'm afraid that I don't like str_fmt at all.

Can we just fix the bits that are actually broken please?

NeilBrown

Re: [patch 0/1][mdadm] Fix needed to enable RAID volumes on SAS devices (version 2).

[Artur Wojcik]

Neil,
The patch version 3 is actually bare minimum but it relay does the
trick. I think it is what I suppose to sent to you at first. Please
forgive my mistakes, but I'm just learning how to cooperate 
with the community.

> And I don't like the introduction of a new header file just to store one or 2
> definitions.  Just put new definitions in mdadm.h  Keep It Simple.

Sorry but I don't agree. At the moment the source code of mdadm/mdmon is
not simple. It's quite complicated. It is hard to analyze how the
control flows and there's a lot of data and code redundancy in it. In my
opinion "refactorization" is what this code needs. Splitting code into
files would be helpful, too.

> I am perfectly happy with making 'buf' larger as appropriate, even making it
> PATH_MAX in some cases.
> I would be happy with more use of asprinf.
> I would be happy adding checks before certain critical sprintf calls that
> the result will not exceed the buffer.

I did not use asprintf function, because it's GNU specific. I had an
impression that one may try to build mdmdm using non-GNU toolchain.
---
Artur

Re: [patch 0/1][mdadm] Fix needed to enable RAID volumes on SAS devices (version 2).

[Dan Williams]

Wojcik, Artur wrote:
>> And I don't like the introduction of a new header file just to store one or 2
>> definitions.  Just put new definitions in mdadm.h  Keep It Simple.
> 
> Sorry but I don't agree. At the moment the source code of mdadm/mdmon is
> not simple.

Adding a new header file for a few definitions does not reduce complexity.

> It's quite complicated. It is hard to analyze how the
> control flows and there's a lot of data and code redundancy in it. In my
> opinion "refactorization" is what this code needs. Splitting code into
> files would be helpful, too.

The current file split is pretty simple and self explanatory.  Each 
major operating mode of mdadm has its own file.

I will say that there are some long functions like Assemble() that could 
use some helper routines to split the major stages.  If only to add 
documentation points and clarify the current working set of parameters. 
  That being said I have only had to touch Assemble.c a handful of 
times, so the cost-benefit ratio of doing this refactoring never became < 1.

If you find some redundant code that would be a good starting point for 
simplification patches.

>> I am perfectly happy with making 'buf' larger as appropriate, even making it
>> PATH_MAX in some cases.
>> I would be happy with more use of asprinf.
>> I would be happy adding checks before certain critical sprintf calls that
>> the result will not exceed the buffer.
> 
> I did not use asprintf function, because it's GNU specific. I had an
> impression that one may try to build mdmdm using non-GNU toolchain.

It is safe to assume that mdadm will always run on a system with glibc, 
uclibc, or an equivalent library that has an asprintf() implementation.

--
Dan