From acd68c48993278f8d509900f37ad45e6136ee370 Mon Sep 17 00:00:00 2001
From: Herman Venter <hermanv@fb.com>
Date: Tue, 23 Jul 2013 10:39:02 -0700
Subject: [PATCH] Check that the names used in function name breakpoints are
 valid PHP identifiers

Breakpoints of the form func() used the same routine to scan names as breakpoints of the form filename:lineno. This meant that you could write something like obj->func() and not get an error message. To fix this I've added a validation routine that is invoked when it has been determined that a name is part of a function name.
---
 hphp/runtime/debugger/break_point.cpp       | 26 +++++++++++++++++++--
 hphp/test/quick/debugger/break1.php.expectf |  4 ++++
 hphp/test/quick/debugger/break1.php.in      |  2 ++
 hphp/test/quick/debugger/break3.php.expectf |  4 ++++
 hphp/test/quick/debugger/break3.php.in      |  2 ++
 5 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/hphp/runtime/debugger/break_point.cpp b/hphp/runtime/debugger/break_point.cpp
index e11ee81fc..bb09bd1dc 100644
--- a/hphp/runtime/debugger/break_point.cpp
+++ b/hphp/runtime/debugger/break_point.cpp
@@ -665,6 +665,24 @@ int32_t BreakPointInfo::parseFileLocation(const std::string &str,
   return offset1;
 }
 
+//( letter | underscore ) #( letter | digit | underscore | extended_ascii ),
+//extended_ascii::=char_nbr(127)..char_nbr(255),
+static bool isValidIdentifier(const std::string &str) {
+  auto len = str.length();
+  for (int32_t index = 0; index < len; index++) {
+    char ch = str[index];
+    if (('A' <= ch && ch <= 'Z') || ('a' <= ch && ch <= 'z') || ch == '_') {
+      continue;
+    }
+    if (index == 0) return false;
+    if (('0' <= ch && ch <= '9') || ch >= 127) {
+      continue;
+    }
+    return false;
+  }
+  return true;
+}
+
 /* The parser accepts the following syntax, which harks back to pre VM days
    (all components are optional, as long as there is at least one component):
 
@@ -733,6 +751,7 @@ void BreakPointInfo::parseBreakPointReached(const std::string &exp,
     }
     while (offset1 < len && exp[offset1] == '\\') {
       if (!m_namespace.empty()) m_namespace += "\\";
+      if (!isValidIdentifier(name)) goto returnInvalid;
       m_namespace += name;
       offset1 += 1;
       auto offset2 = scanName(exp, offset1);
@@ -754,9 +773,10 @@ void BreakPointInfo::parseBreakPointReached(const std::string &exp,
     // The namespace only or the namespace and class might be empty.
     DFunctionInfoPtr pfunc(new DFunctionInfo());
     if (m_class.empty()) {
-      if (m_namespace.empty())
+      if (!isValidIdentifier(name)) goto returnInvalid;
+      if (m_namespace.empty()) {
         pfunc->m_function = name;
-      else {
+      } else {
         // Yes this does seem beyond strange, but that is what the PHP parser
         // does when it sees a function declared inside a namespace, so we
         // too have to pretend there is no namespace here. At some point
@@ -766,10 +786,12 @@ void BreakPointInfo::parseBreakPointReached(const std::string &exp,
       }
     } else {
       mangleXhpName(m_class, pfunc->m_class);
+      if (!isValidIdentifier(pfunc->m_class)) goto returnInvalid;
       if (!m_namespace.empty()) {
         // Emulate parser hack. See longer comment above.
         pfunc->m_class = m_namespace + "\\" + pfunc->m_class;
       }
+      if (!isValidIdentifier(name)) goto returnInvalid;
       pfunc->m_function = name;
     }
     m_funcs.insert(m_funcs.begin(), pfunc);
diff --git a/hphp/test/quick/debugger/break1.php.expectf b/hphp/test/quick/debugger/break1.php.expectf
index a68838f52..f8ad0591d 100644
--- a/hphp/test/quick/debugger/break1.php.expectf
+++ b/hphp/test/quick/debugger/break1.php.expectf
@@ -66,6 +66,8 @@ variable
 $x = "test_break3"
 break clear all
 All breakpoints are cleared.
+break break3->pubObj()
+Breakpoint was not set in right format.
 continue
 pubObj:test_break3
 
@@ -81,6 +83,8 @@ variable
 $x = "test_break4"
 break clear all
 All breakpoints are cleared.
+break cls::break4->pubCls()
+Breakpoint was not set in right format.
 continue
 pubCls:test_break4
 
diff --git a/hphp/test/quick/debugger/break1.php.in b/hphp/test/quick/debugger/break1.php.in
index d1ae90358..5efd38d75 100644
--- a/hphp/test/quick/debugger/break1.php.in
+++ b/hphp/test/quick/debugger/break1.php.in
@@ -20,11 +20,13 @@ break cls::pubObj()
 @ $break3->pubObj('test_break3')
 variable
 break clear all
+break break3->pubObj()
 continue
 break cls::pubCls()
 @ cls::pubCls('test_break4')
 variable
 break clear all
+break cls::break4->pubCls()
 continue
 @ $break5=new cls()
 @ $break5->pubHardBreak('test_break5')
diff --git a/hphp/test/quick/debugger/break3.php.expectf b/hphp/test/quick/debugger/break3.php.expectf
index e370b8163..3e5caff0c 100644
--- a/hphp/test/quick/debugger/break3.php.expectf
+++ b/hphp/test/quick/debugger/break3.php.expectf
@@ -31,6 +31,10 @@ variable
 $x = "test_break2"
 break clear all
 All breakpoints are cleared.
+break \TestNs\break8->pubObj()
+Breakpoint was not set in right format.
+break TestNs\cls::break8->pubObj()
+Breakpoint was not set in right format.
 continue
 pubObj:test_break2
 
diff --git a/hphp/test/quick/debugger/break3.php.in b/hphp/test/quick/debugger/break3.php.in
index 9ec453fb6..f06f1fb85 100644
--- a/hphp/test/quick/debugger/break3.php.in
+++ b/hphp/test/quick/debugger/break3.php.in
@@ -9,6 +9,8 @@ break TestNs\cls::pubObj()
 @ $break8->pubObj('test_break2')
 variable
 break clear all
+break \TestNs\break8->pubObj()
+break TestNs\cls::break8->pubObj()
 continue
 break \TestNs\cls::pubCls()
 @ \TestNs\cls::pubCls('test_break3')