Page MenuHomePhabricator

Phabricator fails to parse unusual protocol requests emitted by Subversion
Closed, ResolvedPublic

Description

See PHI660.

An install reported an issue with certain operations in TortoiseSVN. I made some kind of effort to reproduce this on a Windows 7 machine:

  • Install TortoiseSVN 1.10
  • Install PuTTY 0.70
  • Generate a keypair with PuTTY.
  • Add the public key to Phabricator.
  • Launch Pageant and add the private key.
  • Use TortoiseSVN to checkout an SVN working copy from Phacility.

I was able to get a possible hang with Right ClickTortoiseSVNRepo-browserRight click a fileLogRight click an entryCompare to previous version.

By enabling DebugOutputString and Debug in TortoiseSVN and downloading a tool called DebugView, I got a log pointing at SVNReadProperties.

With some more meddling, I was able to get a hang from:

svn proplist svn+ssh://meta@vault.phacility.com/diffusion/10/README

I can only get this from my Windows machine, but I think this is a Subversion version issue, not a Mac/Windows issue. Things work on 1.7.20 locally but fail on 1.10.0-dev on Windows.

I pointed at my laptop instead and got a reproduction case after a bit more digging. Logging the protocol frames, the principal difference is that the good version sends this:

( get-file ( 0: ( 3 ) true false ) )

...while the bad version of Subversion sends this:

( get-file ( 0: ( 3 ) true false  false ) )

Note this extra space in the protocol frame:

( get-file ( 0: ( 3 ) true false  false ) )
                                 ^

The Subversion wire protocol grammar here:

https://svn.apache.org/repos/asf/subversion/trunk/subversion/libsvn_ra_svn/protocol

...specifies whitespace as:

space  = 1*(SP / LF)

I incorrectly interpreted this as "exactly one space", but RFC2234 says this actually means "one or more spaces":

Default values are 0 and infinity so that *<element> allows any
number, including zero; 1*<element> requires at  least  one;
3*3<element> allows exactly 3 and 1*2<element> allows one or two.

In all other cases I believe Subversion actually sends exactly one space, but it sends two spaces in this case because it builds the protocol frame manually and there's an extra space in the string:

marshal.c
/* Always send the, nominally optional, want-iprops as "false" to
   workaround a bug in svnserve 1.8.0-1.8.8 that causes the server
   to see "true" if it is omitted. */
SVN_ERR(writebuf_write_literal(conn, pool, " false ) ) "));

Our parser should accept more than one space, and ideally be more robust against irregularities in protocol frames.