Re: [libvirt] [PATCH v3] storage: Sanitize pool target paths
On 05/25/2010 10:54 AM, Daniel P. Berrange wrote:
On Tue, May 25, 2010 at 08:42:31AM -0600, Eric Blake wrote:
> On 05/24/2010 12:52 PM, Cole Robinson wrote:
>> +
>> + /* Need to sanitize:
>> + * // -> //
>> + * /// -> /
>> + * /../foo -> /../foo
>> + * /.//foo -> /foo
>> + * /foo///bar/ -> /foo/bar
>> + * ./foo/./. -> /foo
>> + */
>> +
>
> For my second attempt at a valid review, I actually compiled the
> function, and threw the above inputs at it. /../foo -> /./foo (oops,
> didn't match documentation), and ./foo/./. -> /foo (oops, matched
> documentation, but turned a relative path into absolute), so we do need
> a v4, but not for the original reasons in my first NAK where I mis-read
> the do-while loop.
This function is crying out for a real test case to be written and put
under tests/, feeding it all sorts of evil input.
Agreed, however I'm backtracking a bit on this, my next post is only
going to sanitize spurious /, like my original posting (but with the
original comments addressed).
A user can accidentally add an extra slash to a pool target, and its
reasonable to expect it won't cause cause problems, but the same can't
be said for relative path characters. This function is becoming
impossible to review (the current posting goes into infinite loop with a
path like /foo./bar), and most of the logic is not helping solve a real
world problem.
This patch is going to be backported to a few places: less complex the
better. Oh, and I'm lazy :)
- Cole