From e98f7f60095bceba1f32e448c421b3e862b728aa Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Sun, 3 Aug 2008 18:47:56 +0000 Subject: [PATCH] bug #23319: rework the way we handle the working directory when invoking the user's shell, so that we don't have issues when the cwd contains a space or shell special character. --- ChangeLog | 9 +++ test/tests/whatnow/test-attach-detach | 38 +++++++++ test/tests/whatnow/test-cd | 34 ++++++++ test/tests/whatnow/test-ls | 33 ++++++++ uip/whatnowsbr.c | 137 +++++++++++++++++++++------------ 5 files changed, 203 insertions(+), 48 deletions(-) create mode 100644 test/tests/whatnow/test-attach-detach create mode 100644 test/tests/whatnow/test-cd create mode 100644 test/tests/whatnow/test-ls diff --git a/ChangeLog b/ChangeLog index 1e9e31e..6afe9ad 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,14 @@ 2008-08-03 Peter Maydell + * test/setup-test: use 'set -e' so we stop on compile failure. + Configure --enable-debug for convenience in debugging. + + * uip/whatnowsbr.c: bug #23319: rework the way we handle the working + directory when invoking the user's shell, so that we don't have + issues when the cwd contains a space or shell special character. + * test/tests/whatnow/test-attach-detach, test/tests/whatnow/test-cd, + test/tests/whatnow/test-ls: test cases for this. + * sbr/fmt_compile.c: add 'do { ... } while (0)' wrappers to various multi-statement macros to avoid nasty surprises if the macros are used in if() clauses. diff --git a/test/tests/whatnow/test-attach-detach b/test/tests/whatnow/test-attach-detach new file mode 100644 index 0000000..1a36d8d --- /dev/null +++ b/test/tests/whatnow/test-attach-detach @@ -0,0 +1,38 @@ +#!/bin/sh +###################################################### +# +# Test that whatnow's "ls" copes with directory names +# which have spaces in them (see bug #23319) +# +###################################################### + +set -e + +cd "$MH_TEST_DIR" +rm -f "baz's boz" +touch "baz's boz" +# whatnow's attach stuff needs a draft to work on +cp "$MH_TEST_DIR/Mail/inbox/1" "$MH_TEST_DIR/Mail/draft" + +expectederr=$MH_TEST_DIR/$$.expectederr +actualerr=$MH_TEST_DIR/$$.actualerr +expected=$MH_TEST_DIR/$$.expected +actual=$MH_TEST_DIR/$$.actual + +rm -f $expected $expectederr $actual $actualerr +touch $expected $expectederr $actual $actualerr + +cat > $expected <> $actualerr >> $actual +echo "alist" | whatnow -attach foo -noedit -prompt '' 2>> $actualerr >> $actual +echo "detach baz\\'s\\ boz" | whatnow -attach foo -noedit -prompt '' 2>> $actualerr >> $actual +echo "alist" | whatnow -attach foo -noedit -prompt '' 2>> $actualerr >> $actual +set -e + +diff -u $expectederr $actualerr +diff -u $expected $actual diff --git a/test/tests/whatnow/test-cd b/test/tests/whatnow/test-cd new file mode 100644 index 0000000..6bc2040 --- /dev/null +++ b/test/tests/whatnow/test-cd @@ -0,0 +1,34 @@ +#!/bin/sh +###################################################### +# +# Test that whatnow's "ls" copes with directory names +# which have spaces and quotes in them (see bug #23319) +# +###################################################### + +set -e +SPDIR="$MH_TEST_DIR/foo's bar" +rm -rf "$SPDIR" +mkdir "$SPDIR" +cd "$SPDIR" +touch baz boz +cd + +expectederr=$MH_TEST_DIR/$$.expectederr +actualerr=$MH_TEST_DIR/$$.actualerr +expected=$MH_TEST_DIR/$$.expected +actual=$MH_TEST_DIR/$$.actual + +cat > $expected < $expectederr < "$actualerr" > "$actual" || true + +diff -u $expectederr $actualerr +diff -u $expected $actual diff --git a/test/tests/whatnow/test-ls b/test/tests/whatnow/test-ls new file mode 100644 index 0000000..e0dc969 --- /dev/null +++ b/test/tests/whatnow/test-ls @@ -0,0 +1,33 @@ +#!/bin/sh +###################################################### +# +# Test that whatnow's "ls" copes with directory names +# which have spaces and quotes in them (see bug #23319) +# +###################################################### + +set -e +SPDIR="$MH_TEST_DIR/foo's bar" +rm -rf "$SPDIR" +mkdir "$SPDIR" +cd "$SPDIR" +touch baz boz + +expectederr=$MH_TEST_DIR/$$.expectederr +actualerr=$MH_TEST_DIR/$$.actualerr +expected=$MH_TEST_DIR/$$.expected +actual=$MH_TEST_DIR/$$.actual + +cat > $expected < $expectederr < "$actualerr" | sort > "$actual" + +diff -u $expectederr $actualerr +diff -u $expected $actual diff --git a/uip/whatnowsbr.c b/uip/whatnowsbr.c index 72e9d1f..a719431 100644 --- a/uip/whatnowsbr.c +++ b/uip/whatnowsbr.c @@ -120,7 +120,11 @@ static int buildfile (char **, char *); static int check_draft (char *); static int whomfile (char **, char *); static int removefile (char *); -static void writelscmd(char *, int, char *, char **); +static void writelscmd(char *, int, char **); +static void writesomecmd(char *buf, int bufsz, char *cmd, char *trailcmd, char **argp); +static FILE* popen_in_dir(const char *dir, const char *cmd, const char *type); +static int system_in_dir(const char *dir, const char *cmd); + #ifdef HAVE_LSTAT static int copyf (char *, char *); @@ -320,16 +324,13 @@ WhatNow (int argc, char **argv) * is accustomed to. Read back the absolute path. */ - if (*++argp == (char *)0) { + if (*(argp+1) == (char *)0) { (void)sprintf(buf, "$SHELL -c \"cd;pwd\""); } - else if (strlen(*argp) >= BUFSIZ) { - adios((char *)0, "arguments too long"); - } else { - (void)sprintf(buf, "$SHELL -c \"cd %s;cd %s;pwd\"", cwd, *argp); + writesomecmd(buf, BUFSIZ, "cd", "pwd", argp); } - if ((f = popen(buf, "r")) != (FILE *)0) { + if ((f = popen_in_dir(cwd, buf, "r")) != (FILE *)0) { fgets(cwd, sizeof (cwd), f); if (strchr(cwd, '\n') != (char *)0) @@ -354,8 +355,8 @@ WhatNow (int argc, char **argv) * Use the user's shell so that we can take advantage of any * syntax that the user is accustomed to. */ - writelscmd(buf, sizeof(buf), cwd, argp); - (void)system(buf); + writelscmd(buf, sizeof(buf), argp); + (void)system_in_dir(cwd, buf); break; case ALISTCMDSW: @@ -414,7 +415,7 @@ WhatNow (int argc, char **argv) * Build a command line that causes the user's shell to list the file name * arguments. This handles and wildcard expansion, tilde expansion, etc. */ - writelscmd(buf, sizeof(buf), cwd, argp); + writelscmd(buf, sizeof(buf), argp); /* * Read back the response from the shell, which contains a number of lines @@ -424,7 +425,7 @@ WhatNow (int argc, char **argv) * draft. */ - if ((f = popen(buf, "r")) != (FILE *)0) { + if ((f = popen_in_dir(cwd, buf, "r")) != (FILE *)0) { while (fgets(shell, sizeof (shell), f) != (char *)0) { *(strchr(shell, '\n')) = '\0'; @@ -499,35 +500,19 @@ WhatNow (int argc, char **argv) * user's shell for wildcard expansion and other goodies. Do this from * the current working directory if the argument is not an absolute path * name (does not begin with a /). + * + * We feed all the file names to the shell at once, otherwise you can't + * provide a file name with a space in it. */ - - else { - for (arguments = argp + 1; *arguments != (char *)0; arguments++) { - if (**arguments == '/') { - if (strlen(*arguments) + sizeof ("$SHELL -c \"ls \"") >= sizeof (buf)) - adios((char *)0, "arguments too long"); - - (void)sprintf(buf, "$SHELL -c \"ls %s\"", *arguments); - } - else { - if (strlen(cwd) + strlen(*arguments) + sizeof ("$SHELL -c \" cd ; ls \"") >= sizeof (buf)) - adios((char *)0, "arguments too long"); - - (void)sprintf(buf, "$SHELL -c \" cd %s;ls %s\"", cwd, *arguments); - } - - if ((f = popen(buf, "r")) != (FILE *)0) { - while (fgets(shell, sizeof (cwd), f) != (char *)0) { - *(strchr(shell, '\n')) = '\0'; - (void)annotate(drft, attach, shell, 1, 0, 0, 1); - } - - pclose(f); - } - else { - advise("popen", "could not get file from shell"); - } + writelscmd(buf, sizeof(buf), argp); + if ((f = popen_in_dir(cwd, buf, "r")) != (FILE *)0) { + while (fgets(shell, sizeof (shell), f) != (char *)0) { + *(strchr(shell, '\n')) = '\0'; + (void)annotate(drft, attach, shell, 1, 0, 0, 1); } + pclose(f); + } else { + advise("popen", "could not get file from shell"); } break; @@ -542,40 +527,96 @@ WhatNow (int argc, char **argv) } -/* - * Build a command line that causes the user's shell to list the file name - * arguments. This handles and wildcard expansion, tilde expansion, etc. - */ + +/* Build a command line of the form $SHELL -c "cd 'cwd'; cmd argp ... ; trailcmd". */ static void -writelscmd(char *buf, int bufsz, char *cwd, char **argp) +writesomecmd(char *buf, int bufsz, char *cmd, char *trailcmd, char **argp) { char *cp; - int ln = snprintf(buf, bufsz, "$SHELL -c \" cd %s;ls", cwd); + /* Note that we do not quote -- the argp from the user + * is assumed to be quoted as they desire. (We can't treat + * it as pure literal as that would prevent them using ~, + * wildcards, etc.) The buffer produced by this function + * should be given to popen_in_dir() or system_in_dir() so + * that the current working directory is set correctly. + */ + int ln = snprintf(buf, bufsz, "$SHELL -c \"%s", cmd); /* NB that some snprintf() return -1 on overflow rather than the * new C99 mandated 'number of chars that would have been written' */ /* length checks here and inside the loop allow for the - * trailing " and NUL + * trailing ';', trailcmd, '"' and NUL */ - if (ln < 0 || ln + 2 > bufsz) + int trailln = strlen(trailcmd) + 3; + if (ln < 0 || ln + trailln > bufsz) adios((char *)0, "arguments too long"); cp = buf + ln; while (*++argp != (char *)0) { ln = strlen(*argp); - /* +3 for leading space and trailing quote and NUL */ - if (ln + 3 > bufsz - (cp-buf)) + /* +1 for leading space */ + if (ln + trailln + 1 > bufsz - (cp-buf)) adios((char *)0, "arguments too long"); *cp++ = ' '; memcpy(cp, *argp, ln+1); cp += ln; } + if (*trailcmd) { + *cp++ = ';'; + strcpy(cp, trailcmd); + cp += trailln - 3; + } *cp++ = '"'; *cp = 0; } /* + * Build a command line that causes the user's shell to list the file name + * arguments. This handles and wildcard expansion, tilde expansion, etc. + */ +static void +writelscmd(char *buf, int bufsz, char **argp) +{ + writesomecmd(buf, bufsz, "ls", "", argp); +} + +/* Like system(), but run the command in directory dir. + * This assumes the program is single-threaded! + */ +static int +system_in_dir(const char *dir, const char *cmd) +{ + char olddir[BUFSIZ]; + int r; + if (getcwd(olddir, sizeof(olddir)) == 0) + adios("getcwd", "could not get working directory"); + if (chdir(dir) != 0) + adios("chdir", "could not change working directory"); + r = system(cmd); + if (chdir(olddir) != 0) + adios("chdir", "could not change working directory"); + return r; +} + +/* ditto for popen() */ +static FILE* +popen_in_dir(const char *dir, const char *cmd, const char *type) +{ + char olddir[BUFSIZ]; + FILE *f; + if (getcwd(olddir, sizeof(olddir)) == 0) + adios("getcwd", "could not get working directory"); + if (chdir(dir) != 0) + adios("chdir", "could not change working directory"); + f = popen(cmd, type); + if (chdir(olddir) != 0) + adios("chdir", "could not change working directory"); + return f; +} + + +/* * EDIT */ -- 1.7.10.4