Stefan Sperling wrote on Tue, Jan 27, 2015 at 11:39:26 +0100: > On Tue, Jan 27, 2015 at 10:14:50AM +0000, Daniel Shahaf wrote: > > +/* Attempt to strip double quotes from a string. > > + * > > + * The string considered is (STR->DATA + OFFSET). If it starts > > + * and ends with double quotes, and has no escape sequences, return > > + * the string delimited by the double quotes. Otherwise, return > > + * the original string verbatim. > > + */ > > +static const char * > > +remove_shell_quoting(svn_stringbuf_t *buf, > > + int offset, > > + apr_pool_t *result_pool) > > +{ > > + const char *str = buf->data + offset; > > + const char *second_double_quote; > > + > > Perhaps also trim leading and trailing whitespace before checking for quotes? >
Okay. > > + /* Are there exactly two double quotes, at the first and last positions? > > */ > > What about single quotes? The spec allows single quotes: > "Variable assignment values must be enclosed in double or single quotes if > they > include spaces, semicolons or other special characters outside of A-Z, a-z, > 0-9" > Missed that, will fix. > > + if (str[0] == '"' && (second_double_quote = strchr(str+1, '"')) > > + && second_double_quote[1] == '\0') > > + /* Are there any backslashes? */ > > + if (!strchr(str, '\\')) > > As I understand it backslashes used for escaping should be stripped so > that e.g. \$ becomes $ and \\ becomes \. > I'm not completely sure. The spec says: Shell special characters ("$", quotes, backslash, backtick) must be escaped with backslashes, following shell style. To me, the following points are not answered by that specification: - Is \ special inside single quotes, or only inside double quotes? [ Example: '\t' is two characters, "\t" is a tab in bash ] - Is \ special when it is followed by something other than one of the five characters $"'`\\ ? [ Example: "\t" is two characters in dash, but a tab in bash] Some shells also have $'...' style strings, but I don't think we need to support those. So, I agree with you that \\ and \$ should become backslash and dollar, when enclosed by double quotes. But I am not sure how "\t" "\011" '\\' should render. > I'd suggest if we're going to go through the trouble of parsing quotes and > escapes it should be done properly. Alternatively we could just always use > the string verbatim. > That's what my first version of the patch did, but it resulted in this: System information: * running on x86_64-unknown-linux-gnu - "Debian GNU/Linux 7 (wheezy)" [Linux 3.2.0-4-amd64] and I figured it was too inelegant, so I went back and added the dquote-stripping... Thanks for the review. Daniel P.S. I really like the "available authz backends" feature. I saw your name is on it, so thanks :)