From 3544620209cf25fc556c74e0df1c681ab4cdd44b Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 15 May 2018 15:37:40 -0700
Subject: [PATCH] Parse unusual Subversion protocol frames which contain extra
 whitespace

Summary:
Fixes T13140. See PHI660.

Recent versions of Subversion can send a `(get-file true false  false )` protocol frame with extra space between "false" and "false". This is allowed by the protocol spec, but never normally happens, and we do not parse it correctly.

Instead, parse it correctly.

Test Plan:
  - Added unit tests.
  - Ran `svn proplist svn+ssh://.../diffusion/X/file.c` under SVN 1.10 before and after the change.
    - Before: indefinite hang.
    - After: completed in finite time.

Reviewers: amckinley, asherkin

Reviewed By: amckinley, asherkin

Maniphest Tasks: T13140

Differential Revision: https://secure.phabricator.com/D19451
---
 .../DiffusionSubversionWireProtocol.php       | 27 ++++++++-
 ...iffusionSubversionWireProtocolTestCase.php | 59 ++++++++++++++++++-
 2 files changed, 82 insertions(+), 4 deletions(-)

diff --git a/src/applications/diffusion/protocol/DiffusionSubversionWireProtocol.php b/src/applications/diffusion/protocol/DiffusionSubversionWireProtocol.php
index 1cb0f107aa..251967f314 100644
--- a/src/applications/diffusion/protocol/DiffusionSubversionWireProtocol.php
+++ b/src/applications/diffusion/protocol/DiffusionSubversionWireProtocol.php
@@ -33,7 +33,24 @@ final class DiffusionSubversionWireProtocol extends Phobject {
 
     $messages = array();
     while (true) {
-      if ($this->state == 'item') {
+      if ($this->state == 'space') {
+        // Consume zero or more extra spaces after matching an item. The
+        // protocol requires at least one space, but allows more than one.
+
+        $matches = null;
+        if (!preg_match('/^(\s*)\S/', $this->buffer, $matches)) {
+         // Wait for more data.
+          break;
+        }
+
+        // We have zero or more spaces and then some other character, so throw
+        // the spaces away and continue parsing frames.
+        if (strlen($matches[1])) {
+          $this->buffer = substr($this->buffer, strlen($matches[1]));
+        }
+
+        $this->state = 'item';
+      } else if ($this->state == 'item') {
         $match = null;
         $result = null;
         $buf = $this->buffer;
@@ -69,6 +86,12 @@ final class DiffusionSubversionWireProtocol extends Phobject {
             );
             $this->raw = '';
           }
+
+          // Consume any extra whitespace after an item. If we're in the
+          // "bytes" state, we aren't looking for whitespace.
+          if ($this->state == 'item') {
+            $this->state = 'space';
+          }
         } else {
           // No matches yet, wait for more data.
           break;
@@ -90,7 +113,7 @@ final class DiffusionSubversionWireProtocol extends Phobject {
           // Strip off the terminal space.
           $this->pushItem(substr($this->byteBuffer, 0, -1), 'string');
           $this->byteBuffer = '';
-          $this->state = 'item';
+          $this->state = 'space';
         }
       } else {
         throw new Exception(pht("Invalid state '%s'!", $this->state));
diff --git a/src/applications/diffusion/protocol/__tests__/DiffusionSubversionWireProtocolTestCase.php b/src/applications/diffusion/protocol/__tests__/DiffusionSubversionWireProtocolTestCase.php
index bd99b82035..f4309b0e7b 100644
--- a/src/applications/diffusion/protocol/__tests__/DiffusionSubversionWireProtocolTestCase.php
+++ b/src/applications/diffusion/protocol/__tests__/DiffusionSubversionWireProtocolTestCase.php
@@ -59,6 +59,50 @@ final class DiffusionSubversionWireProtocolTestCase
           ),
         ),
       ));
+
+    // This is testing that multiple spaces are parsed correctly. See T13140
+    // for discussion.
+    $this->assertSameSubversionMessages(
+      '( get-file true  false ) ',
+      //               ^-- Note extra space!
+      array(
+        array(
+          array(
+            'type' => 'word',
+            'value' => 'get-file',
+          ),
+          array(
+            'type' => 'word',
+            'value' => 'true',
+          ),
+          array(
+            'type' => 'word',
+            'value' => 'false',
+          ),
+        ),
+      ),
+      '( get-file true false ) ');
+
+    $this->assertSameSubversionMessages(
+      '( duck 5:quack    moo  ) ',
+      array(
+        array(
+          array(
+            'type' => 'word',
+            'value' => 'duck',
+          ),
+          array(
+            'type' => 'string',
+            'value' => 'quack',
+          ),
+          array(
+            'type' => 'word',
+            'value' => 'moo',
+          ),
+        ),
+      ),
+      '( duck 5:quack moo ) ');
+
   }
 
   public function testSubversionWireProtocolPartialFrame() {
@@ -86,7 +130,11 @@ final class DiffusionSubversionWireProtocolTestCase
       ipull($msg2, 'structure'));
   }
 
-  private function assertSameSubversionMessages($string, array $structs) {
+  private function assertSameSubversionMessages(
+    $string,
+    array $structs,
+    $serial_string = null) {
+
     $proto = new DiffusionSubversionWireProtocol();
 
     // Verify that the wire message parses into the structs.
@@ -100,6 +148,13 @@ final class DiffusionSubversionWireProtocolTestCase
       $serial[] = $proto->serializeStruct($struct);
     }
     $serial = implode('', $serial);
-    $this->assertEqual($string, $serial, 'serialize<'.$string.'>');
+
+    if ($serial_string === null) {
+      $expect_serial = $string;
+    } else {
+      $expect_serial = $serial_string;
+    }
+
+    $this->assertEqual($expect_serial, $serial, 'serialize<'.$string.'>');
   }
 }