svn commit: r186504 - head/sbin/mount

Christoph Mallon christoph.mallon at gmx.de
Mon Jan 12 01:44:30 PST 2009


David O'Brien schrieb:
> On Sun, Jan 11, 2009 at 08:56:22AM +0100, Christoph Mallon wrote:
>> David O'Brien schrieb:
>>> On Sun, Jan 04, 2009 at 07:06:18PM +0100, Christoph Mallon wrote:
>>>> I'm pretty sure $SUPERNATURAL_BEING_OF_YOUR_CHOICE killed a kitten for 
>>>> the ugly hack you added to mount. The moment you overflow a buffer, you 
>>>> are in no man's land and there's no escape. I appended a patch, which 
>>>> solves this issue once and for all: The argv array gets dynamically 
>>>> expanded, when its limit is reached.
>>>> Please - for all kittens out there - commit this patch.
>>> Hi Christoph,
>>> Unfortunately your patch doesn't work.
>>> For a 'ufs' file system listed in /etc/fstab
>>>     $ umount /foo
>>>     $ mount /foo
>>> Does not work.
>> Why haven't you told me earlier?
> 
> Wow, wish I did have the gift to know something before I know something.
> Especially tomorrow's lotto #'s. ;-)

I sent you my patch almost a week before you commited your changes. I 
think, there was enough time to tell me, that my patch had a flaw. 
Alternatively I would have gladly reviewed your solution.

>> It was a trivial glitch - just a missing 
>> "--mnt_argc;". I would've corrected it right away. I tested with a CD and 
>> this takes a different code path, which does not trigger the problem. 
>> Attached is the corrected patch.
> 
> Actually this version doesn't work either.
>     Trying to mount root from ufs:/dev/ad4s1a
>     <fsck of all UFS's in /etc/fstab>
>     ..
>     usage: mount [-t fstype] [-o options] target_fs mount_point
>     Mounting /etc/fstab filesystems failed,  startup aborted
>     ERROR: ABORTING BOOT (sending SIGTERM to parent)!
> 
> Your over use of global variables made my adult cat cry in pain.
> Too bad you didn't at least follow the lead of what I committed.
> 
> I've committed something in the spirit of your patches.  I hope
> that suits everyone.

Actually this version doesn't work either.

tron# ./mount -a
usage: mount [-t fstype] [-o options] target_fs mount_point
tron# ./mount -d -a
mount -t ufs -o rw -o update /dev/ad0s1a /
mount -t ufs ��`X (濿��`X /dev/ad0s1f /data
(Sorry for the garbage, it actually printed that. You can turn it into a 
"clean" segfault by memset()ing mnt_argv just after its declaration)

Your incorrect use of local variables and the resulting undefined 
behaviour made the cat, who visits me on a roof behind the house 
sometimes, almost fall from said roof, when he saw your commit: You 
expect the local variable "struct cpa mnt_argv" still to have the same 
values after mountfs() was left and reentered. Too bad you didn't at 
least follow the lead of what I proposed.

I've attached a corrected version of my patch, which has a mnt_argc = 0; 
added in order to reset the argument vector on reentry of mountfs() 
(instead of appending to the arguments of the last round).
-------------- next part --------------
Index: mount.c
===================================================================
--- mount.c	(Revision 187093)
+++ mount.c	(Arbeitskopie)
@@ -67,18 +67,16 @@
 #define MOUNT_META_OPTION_CURRENT	"current"
 
 int debug, fstab_style, verbose;
+static char **mnt_argv;
+static int mnt_argv_size;
+static int mnt_argc;
 
-struct cpa {
-	char	**a;
-	int	c;
-};
-
 char   *catopt(char *, const char *);
 struct statfs *getmntpt(const char *);
 int	hasopt(const char *, const char *);
 int	ismounted(struct fstab *, struct statfs *, int);
 int	isremountable(const char *);
-void	mangle(char *, struct cpa *);
+static void	mangle(char *);
 char   *update_options(char *, char *, int);
 int	mountfs(const char *, const char *, const char *,
 			int, const char *, const char *);
@@ -501,28 +499,24 @@
 }
 
 static void
-append_arg(struct cpa *sa, char *arg)
+append_argv(char *arg)
 {
-	static int a_sz;
-
-	if (sa->c + 1 == a_sz) {
-		a_sz = a_sz == 0 ? 8 : a_sz * 2;
-		sa->a = realloc(sa->a, sizeof(sa->a) * a_sz);
-		if (sa->a == NULL)
+	if (mnt_argc == mnt_argv_size) {
+		mnt_argv_size = mnt_argv_size == 0 ? 16 : mnt_argv_size * 2;
+		mnt_argv = realloc(mnt_argv, sizeof(*mnt_argv) * mnt_argv_size);
+		if (mnt_argv == NULL)
 			errx(1, "realloc failed");
 	}
-	sa->a[++sa->c] = arg;
+	mnt_argv[mnt_argc++] = arg;
 }
 
 int
 mountfs(const char *vfstype, const char *spec, const char *name, int flags,
 	const char *options, const char *mntopts)
 {
-	struct cpa mnt_argv;
 	struct statfs sf;
 	int i, ret;
 	char *optbuf, execname[PATH_MAX], mntpath[PATH_MAX];
-	static int mnt_argv_inited;
 
 	/* resolve the mountpoint with realpath(3) */
 	(void)checkpath(name, mntpath);
@@ -557,32 +551,29 @@
 	/* Construct the name of the appropriate mount command */
 	(void)snprintf(execname, sizeof(execname), "mount_%s", vfstype);
 
-	if (!mnt_argv_inited) {
-		mnt_argv_inited++;
-		mnt_argv.a = NULL;
-	}
-	mnt_argv.c = -1;
-	append_arg(&mnt_argv, execname);
-	mangle(optbuf, &mnt_argv);
-	append_arg(&mnt_argv, strdup(spec));
-	append_arg(&mnt_argv, strdup(name));
-	append_arg(&mnt_argv, NULL);
+	mnt_argc = 0; /* Reset argument vector */
+	append_argv(execname);
+	mangle(optbuf);
+	append_argv(strdup(spec));
+	append_argv(strdup(name));
+	append_argv(NULL);
+	--mnt_argc; /* Do not count the terminating null pointer */
 
 	if (debug) {
 		if (use_mountprog(vfstype))
 			printf("exec: mount_%s", vfstype);
 		else
 			printf("mount -t %s", vfstype);
-		for (i = 1; i < mnt_argv.c; i++)
-			(void)printf(" %s", mnt_argv.a[i]);
+		for (i = 1; i < mnt_argc; i++)
+			(void)printf(" %s", mnt_argv[i]);
 		(void)printf("\n");
 		return (0);
 	}
 
 	if (use_mountprog(vfstype)) {
-		ret = exec_mountprog(name, execname, mnt_argv.a);
+		ret = exec_mountprog(name, execname, mnt_argv);
 	} else {
-		ret = mount_fs(vfstype, mnt_argv.c, mnt_argv.a);
+		ret = mount_fs(vfstype, mnt_argc, mnt_argv);
 	}
 
 	free(optbuf);
@@ -684,8 +675,8 @@
 	return (cp);
 }
 
-void
-mangle(char *options, struct cpa *a)
+static void
+mangle(char *options)
 {
 	char *p, *s;
 
@@ -720,15 +711,15 @@
 			    sizeof(groupquotaeq) - 1) == 0) {
 				continue;
 			} else if (*p == '-') {
-				append_arg(a, p);
+				append_argv(p);
 				p = strchr(p, '=');
 				if (p != NULL) {
 					*p = '\0';
-					append_arg(a, p + 1);
+					append_argv(p + 1);
 				}
 			} else {
-				append_arg(a, strdup("-o"));
-				append_arg(a, p);
+				append_argv(strdup("-o"));
+				append_argv(p);
 			}
 		}
 }


More information about the svn-src-head mailing list