Skip to content

Commit

Permalink
Make DummySandbox shutdown always call execute_command_line_end
Browse files Browse the repository at this point in the history
Failing to call this when there are live RequestLocal objects
will can cause them to access deleted objects during pthread's
shutdown (calling OnThreadExit).  This fixes and adds assertions about
ref counts that makes this easier to hit this in a debug build.
  • Loading branch information
jdelong authored and sgolemon committed Sep 13, 2012
1 parent 9db1d12 commit 28e4699
Show file tree
Hide file tree
Showing 4 changed files with 92 additions and 38 deletions.
4 changes: 0 additions & 4 deletions src/runtime/base/file/plain_file.h
Expand Up @@ -55,10 +55,6 @@ class PlainFile : public File {
FILE *getStream() { return m_stream;}
virtual const char *getStreamType() const { return "STDIO";}

static CVarRef getStdIn();
static CVarRef getStdOut();
static CVarRef getStdErr();

protected:
FILE *m_stream;
bool m_eof;
Expand Down
2 changes: 1 addition & 1 deletion src/runtime/base/memory/smart_allocator.h
Expand Up @@ -294,7 +294,7 @@ class SmartAllocatorImpl : boost::noncopyable {
void* alloc(size_t size);
void dealloc(void *obj) {
ASSERT(assertValidHelper(obj));
ASSERT(memset(obj, 0x5a, m_itemSize));
ASSERT(memset(obj, 0x6a, m_itemSize));
m_freelist.push(obj);
if (hhvm) {
int tomb = RefCountTombstoneValue;
Expand Down
98 changes: 71 additions & 27 deletions src/runtime/base/util/countable.h
Expand Up @@ -18,36 +18,73 @@
#define __HPHP_COUNTABLE_H__

#include <util/base.h>
#include <util/util.h>
#include <util/trace.h>
#include <util/atomic.h>

namespace HPHP {
///////////////////////////////////////////////////////////////////////////////

/**
* This is a special value for _count used to indicate objects that
* are already deallocated. (See smart_allocator.h.)
*/
const int32_t RefCountTombstoneValue = 0xde1ee7ed;
static_assert(RefCountTombstoneValue < 0,
"RefCountTombstoneValue should be negative to aid assertions");

/*
* This bit flags a reference count as "static". If a reference count
* is static, it means we should never increment or decrement it: the
* object lives across requests and may be accessed by multiple
* threads.
*/
const int32_t RefCountStaticValue = (1 << 30);

// Used for assertions.
inline DEBUG_ONLY bool is_refcount_realistic(int32_t count) {
return count != RefCountTombstoneValue &&
count != 0x5a5a5a5a; /* debug freed mem in malloc */
}

/**
* StringData and Variant do not formally derive from Countable, but they
* have a _count field and define all of the methods from Countable. These
* macros are provided to avoid code duplication.
*/
#define IMPLEMENT_COUNTABLE_METHODS_NO_STATIC \
int32_t getCount() const { return _count; } \
bool isRefCounted() const { return _count != Countable::STATIC_FLAG; } \
void incRefCount() const { if (isRefCounted()) { ++_count; } } \
int32_t decRefCount() const { \
ASSERT(_count > 0); \
return isRefCounted() ? --_count : _count; \
#define IMPLEMENT_COUNTABLE_METHODS_NO_STATIC \
int32_t getCount() const { \
ASSERT(is_refcount_realistic(_count)); \
return _count; \
} \
\
bool isRefCounted() const { \
ASSERT(is_refcount_realistic(_count)); \
return _count != RefCountStaticValue; \
} \
\
void incRefCount() const { \
ASSERT(is_refcount_realistic(_count)); \
if (isRefCounted()) { ++_count; } \
} \
\
int32_t decRefCount() const { \
ASSERT(_count > 0); \
ASSERT(is_refcount_realistic(_count)); \
return isRefCounted() ? --_count : _count; \
}

#define IMPLEMENT_COUNTABLE_METHODS \
/* setStatic() is used by StaticString and StaticArray to make */ \
/* sure ref count is "never" going to reach 0, even if multiple */ \
/* threads modify it in a non-thread-safe fashion. */ \
void setStatic() const { _count = Countable::STATIC_FLAG; } \
bool isStatic() const { return _count == STATIC_FLAG; } \
#define IMPLEMENT_COUNTABLE_METHODS \
void setStatic() const { \
ASSERT(is_refcount_realistic(_count)); \
_count = RefCountStaticValue; \
} \
bool isStatic() const { \
ASSERT(is_refcount_realistic(_count)); \
return _count == STATIC_FLAG; \
} \
IMPLEMENT_COUNTABLE_METHODS_NO_STATIC

const int32_t RefCountStaticValue = (1 << 30);

/**
* Implements reference counting. We chose to roll our own SmartPtr type
* instead of using boost::shared_ptr<T> so that we can achieve better
Expand All @@ -62,17 +99,24 @@ class Countable {
mutable int32_t _count;
};

/**
* This is a special value for _count used to indicate objects that
* are already deallocated. (See smart_allocator.h.)
*/
const int32_t RefCountTombstoneValue = 0xde1ee7ed;

#define IMPLEMENT_COUNTABLENF_METHODS_NO_STATIC \
int32_t getCount() const { return _count; } \
bool isRefCounted() const { return true; } \
void incRefCount() const { ++_count; } \
int32_t decRefCount() const { ASSERT(_count > 0); return --_count; }
#define IMPLEMENT_COUNTABLENF_METHODS_NO_STATIC \
int32_t getCount() const { \
ASSERT(is_refcount_realistic(_count)); \
return _count; \
} \
\
bool isRefCounted() const { return true; } \
\
void incRefCount() const { \
ASSERT(is_refcount_realistic(_count)); \
++_count; \
} \
\
int32_t decRefCount() const { \
ASSERT(_count > 0); \
ASSERT(is_refcount_realistic(_count)); \
return --_count; \
}

/**
* CountableNF : countable no flags
Expand Down Expand Up @@ -106,7 +150,7 @@ class CountableHelper {
class AtomicCountable {
public:
AtomicCountable() : _count(0) {}
int32_t getCount() const { return _count; }
int32_t getCount() const { return _count; }
void incAtomicCount() const { atomic_inc(_count); }
int decAtomicCount() const { return atomic_dec(_count); }
protected:
Expand Down
26 changes: 20 additions & 6 deletions src/runtime/eval/debugger/dummy_sandbox.cpp
Expand Up @@ -14,6 +14,8 @@
+----------------------------------------------------------------------+
*/

#include <boost/noncopyable.hpp>

#include <runtime/eval/debugger/dummy_sandbox.h>
#include <runtime/eval/debugger/debugger.h>
#include <runtime/eval/debugger/cmd/cmd_signal.h>
Expand Down Expand Up @@ -63,15 +65,30 @@ void DummySandbox::stop() {
}
}

namespace {

struct CLISession : boost::noncopyable {
CLISession() {
char *argv[] = {"", NULL};
execute_command_line_begin(1, argv, 0);
}
~CLISession() {
Debugger::UnregisterSandbox(g_context->getSandboxId());
ThreadInfo::s_threadInfo.getNoCheck()->
m_reqInjectionData.debugger = false;
execute_command_line_end(0, false, NULL);
}
};

}

void DummySandbox::run() {
ThreadInfo *ti = ThreadInfo::s_threadInfo.getNoCheck();
Debugger::RegisterThread();
ti->m_reqInjectionData.dummySandbox = true;
while (!m_stopped) {
try {
char *argv[] = {"", NULL};
execute_command_line_begin(1, argv, 0);

CLISession hphpSession;
FUNCTION_INJECTION_FS("_", FrameInjection::PseudoMain);

DSandboxInfo sandbox = m_proxy->getSandbox();
Expand Down Expand Up @@ -132,10 +149,7 @@ void DummySandbox::run() {
break;
} catch (const DebuggerException &e) {
}
ti->m_reqInjectionData.debugger = false;
execute_command_line_end(0, false, NULL);
}
Debugger::UnregisterSandbox(g_context->getSandboxId());
}

void DummySandbox::notifySignal(int signum) {
Expand Down

0 comments on commit 28e4699

Please sign in to comment.