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 :)
