[Fix/HPHPi] Fix deadlock when checking out files

Summary:
The file repository uses ReadWrite lock in order to allow multiple read
operation (e.g., parsing files) at the same time. Only when the file
repository metadata needs to be updated, a write lock is needed. However,
an warning/error raised during parsing while a thread holds the read lock
can trigger a recursive parsing. This happens when the user error handler
is invoked. Deadlock happens when the thread already holds a read lock and
tries to acquire a write lock. I noticed that the parsing can be made
lock free rather than under a read lock so I restructured the code to make
it so. I also made the change to use Logger::Warning instead of raise_warning
when the maximum # of user function id is reached because that is an internal
limit and should not be handled by a user handler. Finally, fixed some
off-by-one assertion failures under DEBUG mode.

Test Plan:
make fast_tests
start hphpi in sandbox mode and browse the site
run server mode test to finish with no deadlock or crash
the test case that can lead to deadlock:
[] cat test.php
<?php
function myErrorHandler($errno, $errstr, $errfile, $errline) {
  require_once('junk.php');
  var_dump($errstr, $errline);
}
set_error_handler('myErrorHandler');
include_once('hello.php');
[] cat hello.php
<?php
function f1() {}
function f2() {}
function f3() {}
function f4() {}
function f5() {}
function f6() {}
echo "hello, world\n";
[] cat junk.php
<?php
echo "junk\n";

Start hphpi in sandbox mode with -v Eval.MaxUserFunctionId=5

Before the fix, the command GET http://<sandbox>/test.php will hang and
gdb attach the server confirms the deadlock. After the fix it no longer
deadlock (even without the raise_warning to Logger::Warning change).

Reviewers: qigao, mwilliams

Reviewed By: mwilliams

CC: hphp-diffs@lists, ps, mwilliams, amenghra

Differential Revision: 342493

src/runtime/base/program_functions.cpp

@@ -852,9 +852,9 @@ static int execute_program_impl(int argc, char **argv) {
     struct stat s;
     if (!Eval::FileRepository::findFile(fileName, &s)) return 0;
     Eval::Parser::Reset();
-    bool created;
-    Eval::PhpFile *f =
-      Eval::FileRepository::readFile(fileName, s, created);
+    Eval::FileRepository::FileInfo fileInfo;
+    if (!Eval::FileRepository::readFile(fileName, s, fileInfo)) return 0;
+    Eval::PhpFile *f = Eval::FileRepository::parseFile(fileName, fileInfo);
     if (!f) return 0;
     const Eval::StatementPtr &tree = f->getTree();
     tree->dump(cout);

src/runtime/eval/ast/function_statement.cpp

@@ -37,6 +37,7 @@
 #include <system/lib/systemlib.h>
 
 #include <util/parser/parser.h>
+#include <util/logger.h>
 #include <tbb/concurrent_hash_map.h>
 
 namespace HPHP {
@@ -65,16 +66,16 @@ UserFunctionIdTable::UserFunctionIdTable() : m_size(0) {
 }
 
 void UserFunctionIdTable::requestInit() {
-  ASSERT(s_id < RuntimeOption::MaxUserFunctionId);
+  ASSERT(s_id <= RuntimeOption::MaxUserFunctionId);
   memset(m_funcStmts, 0, sizeof(FunctionStatement *) * m_size);
   grow(atomic_add(s_id, 0) + 1);
 }
 
-bool UserFunctionIdTable::grow(int id) {
-  ASSERT(id < RuntimeOption::MaxUserFunctionId);
-  int inc = id - m_size;
+bool UserFunctionIdTable::grow(int size) {
+  ASSERT(size <= RuntimeOption::MaxUserFunctionId + 1);
+  int inc = size - m_size;
   if (inc <= 0) return false;
-  if (id >= m_alloc) {
+  if (size >= m_alloc) {
     m_alloc += INC_FUNC_ID_TABLE_SIZE;
     if (inc > INC_FUNC_ID_TABLE_SIZE) {
       m_alloc += inc - inc % INC_FUNC_ID_TABLE_SIZE;
@@ -83,7 +84,7 @@ bool UserFunctionIdTable::grow(int id) {
       realloc(m_funcStmts, m_alloc * sizeof(FunctionStatement *));
   }
   memset(m_funcStmts + m_size, 0, sizeof(FunctionStatement *) * inc);
-  m_size = id;
+  m_size = size;
   return true;
 }
 
@@ -100,7 +101,7 @@ int UserFunctionIdTable::getUserFunctionId(CStrRef func) {
 
 int UserFunctionIdTable::GetUserFunctionId(CStrRef func) {
   if (s_id >= RuntimeOption::MaxUserFunctionId) {
-    raise_warning("Maximum function id reached: %d", s_id);
+    Logger::Warning("Maximum function id reached: %d", s_id);
     return -1;
   }
   return s_userFunctionIdTable->getUserFunctionId(func);

src/runtime/eval/runtime/file_repository.cpp

@@ -125,6 +125,7 @@ void FileRepository::onZeroRef(PhpFile *f) {
 
 PhpFile *FileRepository::checkoutFile(const string &rname,
                                       const struct stat &s) {
+  FileInfo fileInfo;
   PhpFile *ret = NULL;
   string name;
 
@@ -136,18 +137,17 @@ PhpFile *FileRepository::checkoutFile(const string &rname,
     name = RuntimeOption::SourceRoot + "/" + rname;
   }
 
-  bool created = false;
   bool changed = false;
   {
     ReadLock lock(s_lock);
     hphp_hash_map<string, PhpFileWrapper*, string_hash>::iterator it =
       s_files.find(name);
     if (it == s_files.end()) {
-      ret = readFile(name, s, created);
-      if (!ret) return NULL;
+      if (!readFile(name, s, fileInfo)) return NULL;
+      ret = fileInfo.m_phpFile;
     } else if (it->second->isChanged(s)) {
-      ret = readFile(name, s, created);
-      if (!ret) return NULL;
+      if (!readFile(name, s, fileInfo)) return NULL;
+      ret = fileInfo.m_phpFile;
       PhpFile *f = it->second->getPhpFile();
       if (ret == f) {
         if (RuntimeOption::SandboxCheckMd5) {
@@ -165,6 +165,14 @@ PhpFile *FileRepository::checkoutFile(const string &rname,
     }
   }
 
+  // parseFile is lock free
+  bool created = false;
+  if (!ret) {
+    ret = parseFile(name, fileInfo);
+    if (!ret) return NULL;
+    created = true;
+  }
+
   WriteLock lock(s_lock);
   hphp_hash_map<string, PhpFileWrapper*, string_hash>::iterator it =
     s_files.find(name);
@@ -229,54 +237,59 @@ bool FileRepository::findFile(const string &path, struct stat *s) {
   return fileStat(path.c_str(), s) && !S_ISDIR(s->st_mode);
 }
 
-PhpFile *FileRepository::readFile(const string &name,
-                                  const struct stat &s,
-                                  bool &created) {
-  created = false;
-  vector<StaticStatementPtr> sts;
-  Block::VariableIndices variableIndices;
+bool FileRepository::readFile(const string &name,
+                              const struct stat &s,
+                              FileInfo &fileInfo) {
   if (s.st_size > StringData::LenMask) {
     throw FatalErrorException(0, "file %s is too big", name.c_str());
   }
   int fileSize = s.st_size;
-  if (!fileSize) return NULL;
+  if (!fileSize) return false;
   int fd = open(name.c_str(), O_RDONLY);
-  if (!fd) return NULL; // ignore file open exception
+  if (!fd) return false; // ignore file open exception
   char *input = (char *)malloc(fileSize + 1);
-  if (!input) return NULL;
+  if (!input) return false;
   int nbytes = read(fd, input, fileSize);
   close(fd);
   input[fileSize] = 0;
-  String strHelper(input, fileSize, AttachString);
-  if (nbytes != fileSize) return NULL;
+  fileInfo.m_inputString = String(input, fileSize, AttachString);
+  if (nbytes != fileSize) return false;
 
-  string md5;
-  string relPath;
-  string srcRoot;
   if (RuntimeOption::SandboxCheckMd5) {
-    md5 = StringUtil::MD5(strHelper).c_str();
-    srcRoot = SourceRootInfo::GetCurrentSourceRoot();
-    if (srcRoot.empty()) srcRoot = RuntimeOption::SourceRoot;
-    int srcRootLen = srcRoot.size();
+    fileInfo.m_md5 = StringUtil::MD5(fileInfo.m_inputString).c_str();
+    fileInfo.m_srcRoot = SourceRootInfo::GetCurrentSourceRoot();
+    if (fileInfo.m_srcRoot.empty()) {
+      fileInfo.m_srcRoot = RuntimeOption::SourceRoot;
+    }
+    int srcRootLen = fileInfo.m_srcRoot.size();
     if (srcRootLen) {
-      if (!strncmp(name.c_str(), srcRoot.c_str(), srcRootLen)) {
-        relPath = string(name.c_str() + srcRootLen);
+      if (!strncmp(name.c_str(), fileInfo.m_srcRoot.c_str(), srcRootLen)) {
+        fileInfo.m_relPath = string(name.c_str() + srcRootLen);
       }
     }
     hphp_hash_map<string, PhpFile*, string_hash>::iterator it =
-      s_md5Files.find(md5);
+      s_md5Files.find(fileInfo.m_md5);
     if (it != s_md5Files.end()) {
       PhpFile *f = it->second;
-      if (!relPath.empty() && relPath == f->getRelPath()) {
-        return it->second;
+      if (!fileInfo.m_relPath.empty() &&
+          fileInfo.m_relPath == f->getRelPath()) {
+        fileInfo.m_phpFile = f;
       }
     }
   }
+  return true;
+}
+
+PhpFile *FileRepository::parseFile(const std::string &name,
+                                   const FileInfo &fileInfo) {
+  vector<StaticStatementPtr> sts;
+  Block::VariableIndices variableIndices;
   if (RuntimeOption::EnableEvalOptimization) {
     ScalarValueExpression::initScalarValues();
   }
   StatementPtr stmt =
-    Parser::ParseString(strHelper, name.c_str(), sts, variableIndices);
+    Parser::ParseString(fileInfo.m_inputString, name.c_str(),
+                        sts, variableIndices);
   if (stmt && RuntimeOption::EnableEvalOptimization) {
     DummyVariableEnvironment env;
     stmt->optimize(env);
@@ -285,9 +298,9 @@ PhpFile *FileRepository::readFile(const string &name,
     if (RuntimeOption::EnableEvalOptimization) {
       ScalarValueExpression::registerScalarValues();
     } 
-    created = true;
     PhpFile *p = new PhpFile(stmt, sts, variableIndices,
-                             name, srcRoot, relPath, md5);
+                             name, fileInfo.m_srcRoot,
+                             fileInfo.m_relPath, fileInfo.m_md5);
     return p;
   }
   return NULL;

src/runtime/eval/runtime/file_repository.h

@@ -86,15 +86,26 @@ class PhpFileWrapper {
  */
 class FileRepository {
 public:
+  class FileInfo {
+  public:
+    FileInfo() : m_phpFile(NULL) {}
+    PhpFile *m_phpFile;
+    String m_inputString;
+    std::string m_md5;
+    std::string m_srcRoot;
+    std::string m_relPath;
+  };
+
   /**
    * The first time you attempt to invoke a file in a request, this is called.
    * From then on, invoke_file will store the PhpFile and use that.
    */
   static PhpFile *checkoutFile(const std::string &name, const struct stat &s);
   static bool findFile(const std::string &path, struct stat *s);
   static bool fileDump(const char *filename);
-  static PhpFile *readFile(const std::string &name, const struct stat &s,
-                           bool &created);
+  static bool readFile(const std::string &name, const struct stat &s,
+                       FileInfo &fileInfo);
+  static PhpFile *parseFile(const std::string &name, const FileInfo &fileInfo);
   static String translateFileName(const std::string &file);
   static void onZeroRef(PhpFile *f);
 private: