Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

[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
  • Loading branch information...
commit 30a76e2eb1b8f2aa7444dcb1ffd0a4ce99016219 1 parent f7457f5
myang authored scottmac committed
View
6 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);
View
15 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);
View
75 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;
View
15 src/runtime/eval/runtime/file_repository.h
@@ -86,6 +86,16 @@ 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.
@@ -93,8 +103,9 @@ class FileRepository {
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:
Please sign in to comment.
Something went wrong with that request. Please try again.