Skip to content

Commit

Permalink
migrate ini parsing away from legacy arrays - try 2
Browse files Browse the repository at this point in the history 
Summary:
This is a redo of D20805919 with a fix for the crash, described in T65255134.

Crash stack:

```
#0  0x0000000006566dbe in HPHP::arrprov::tagFromPC () at hphp/runtime/base/array-provenance.cpp:343
#1  0x000000000670b2d5 in HPHP::arrprov::tagStaticArr (tag=..., ad=<optimized out>) at hphp/runtime/base/array-provenance.cpp:291
#2  HPHP::ArrayData::CreateDict (tag=..., tag=...) at buck-out/opt-hhvm-lto/gen/hphp/runtime/headers#header-mode-symlink-tree-only,headers,v422239b/hphp/runtime/base/array-data-inl.h:105
#3  HPHP::Array::CreateDict () at buck-out/opt-hhvm-lto/gen/hphp/runtime/headers#header-mode-symlink-tree-only,headers,v422239b/hphp/runtime/base/type-array.h:97
#4  HPHP::IniSettingMap::IniSettingMap (this=<optimized out>, this=<optimized out>) at hphp/runtime/base/ini-setting.cpp:517
#5  0x00000000068835bf in HPHP::IniSettingMap::IniSettingMap (this=0x7fff36f4d8f0) at hphp/runtime/base/config.cpp:370
#6  HPHP::Config::Iterate(std::function<void (HPHP::IniSettingMap const&, HPHP::Hdf const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)>, HPHP::IniSettingMap const&, HPHP::Hdf const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, bool) (cb=..., ini=..., config=..., name=..., prepend_hhvm=prepend_hhvm@entry=true) at hphp/runtime/base/config.cpp:370
#7  0x0000000006927373 in HPHP::RuntimeOption::Load (ini=..., config=..., iniClis=..., hdfClis=..., messages=messages@entry=0x7fff36f4e170, cmd=...)
    at third-party-buck/platform007/build/libgcc/include/c++/trunk/bits/std_function.h:106
#8  0x000000000695aaee in HPHP::execute_program_impl (argc=argc@entry=21, argv=argv@entry=0x7fff36f4fac8) at hphp/runtime/base/program-functions.cpp:1716
```

The immediate cause of the crash is trying to access vmfp() inside tagFromPC before VM was initialized, resulting in null pointer dereference.
However, provenance tagging should have actually used runtime tag override, set in RuntimeOption::Load instead of trying to compute tag off PC.
The problem is: TagOverride short-circuits itself if Eval.ArrayProvenance is disabled.

So we run into the following sequence of events:
1. we start with EvalArrayProvenance=false - default value
2. TagOverride short-circuits and doesn't actually update the override
3. we parse config options and set EvalArrayProvenance=true
4. we try to create dict, decide that it needs provenance and try to compute a tag. Since there is no override set we fall back to tagFromPC and crash

To fix this I made TagOverride not short-circuit for this 1 specific call site.

The specific nature of a bug also explains, why it didn't get caught by any of prior testing:
- hphp tests could not catch it, because they run with minimal config, which doesn't exercise Config::Iterate
- I didn't specifically test sandbox with array provenance enabled, which would catch it
- This wouldn't be caught in servicelab, since we enable provenance in SL by changing default values. Also, I didn't run SL with provenance enabled for this.

What would catch this is starting either sandbox or prod-like web server with actual config.hdf or config.hdf.devrs and array provenance enabled via config or commandline options.

Differential Revision: D20974470

fbshipit-source-id: 6474fe4e4cf808c4e8572539119cd57374658877
  • Loading branch information
@dneiter @facebook-github-bot
dneiter authored and facebook-github-bot committed May 1, 2020
1 parent d44db4c commit df1cee0
Show file tree
Hide file tree
Showing 6 changed files with 105 additions and 39 deletions.
9 changes: 9 additions & 0 deletions hphp/runtime/base/array-provenance.cpp
View file
Expand Up @@ -314,6 +314,15 @@ TagOverride::TagOverride(Tag tag)
}
}

TagOverride::TagOverride(Tag tag, TagOverride::ForceTag)
: m_valid(true)
{
if (m_valid) {
m_saved_tag = *rl_tag_override;
*rl_tag_override = tag;
}
}

TagOverride::~TagOverride() {
if (m_valid) {
*rl_tag_override = m_saved_tag;
Expand Down
12 changes: 12 additions & 0 deletions hphp/runtime/base/array-provenance.h
View file
Expand Up @@ -257,7 +257,10 @@ Tag tagFromSK(SrcKey sk);
* backtrace
*/
struct TagOverride {
enum class ForceTag {};

explicit TagOverride(Tag tag);
TagOverride(Tag tag, ForceTag);
~TagOverride();

TagOverride(TagOverride&&) = delete;
Expand All @@ -281,6 +284,15 @@ struct TagOverride {
#define ARRPROV_USE_RUNTIME_LOCATION() \
::HPHP::arrprov::TagOverride ap_override(ARRPROV_HERE())

// Set tag even if provenanance is currently disabled.
// This is useful for runtime initialization and config parsing code, where
// Eval.ArrayProvenance may change as result of config parsing.
#define ARRPROV_USE_RUNTIME_LOCATION_FORCE() \
::HPHP::arrprov::TagOverride ap_override( \
ARRPROV_HERE(), \
::HPHP::arrprov::TagOverride::ForceTag{} \
)

#define ARRPROV_USE_POISONED_LOCATION() \
::HPHP::arrprov::TagOverride ap_override( \
::HPHP::arrprov::Tag::RuntimeLocationPoison( \
Expand Down
103 changes: 65 additions & 38 deletions hphp/runtime/base/ini-setting.cpp
View file
Expand Up @@ -370,7 +370,7 @@ Variant ini_get(String& p) {
namespace {

template<typename T>
void add(ArrayInit& arr, const std::string& key, const T& value) {
void add(DArrayInit& arr, const std::string& key, const T& value) {
auto keyStr = String(key);
int64_t n;
if (keyStr.get()->isStrictlyInteger(n)) {
Expand All @@ -383,31 +383,31 @@ void add(ArrayInit& arr, const std::string& key, const T& value) {
}

Variant ini_get(std::unordered_map<std::string, int>& p) {
ArrayInit ret(p.size(), ArrayInit::Map{});
DArrayInit ret(p.size());
for (auto& pair : p) {
add(ret, pair.first, pair.second);
}
return ret.toArray();
}

Variant ini_get(std::map<std::string, std::string>& p) {
ArrayInit ret(p.size(), ArrayInit::Map{});
DArrayInit ret(p.size());
for (auto& pair : p) {
add(ret, pair.first, pair.second);
}
return ret.toArray();
}

Variant ini_get(std::map<std::string, std::string, stdltistr>& p) {
ArrayInit ret(p.size(), ArrayInit::Map{});
DArrayInit ret(p.size());
for (auto& pair : p) {
add(ret, pair.first, pair.second);
}
return ret.toArray();
}

Variant ini_get(hphp_string_imap<std::string>& p) {
ArrayInit ret(p.size(), ArrayInit::Map{});
DArrayInit ret(p.size());
for (auto& pair : p) {
add(ret, pair.first, pair.second);
}
Expand All @@ -419,38 +419,34 @@ Variant ini_get(Array& p) {
}

Variant ini_get(std::set<std::string>& p) {
ArrayInit ret(p.size(), ArrayInit::Map{});
auto idx = 0;
VArrayInit ret(p.size());
for (auto& s : p) {
ret.add(idx++, s);
ret.append(s);
}
return ret.toArray();
}

Variant ini_get(std::set<std::string, stdltistr>& p) {
ArrayInit ret(p.size(), ArrayInit::Map{});
auto idx = 0;
VArrayInit ret(p.size());
for (auto& s : p) {
ret.add(idx++, s);
ret.append(s);
}
return ret.toArray();
}

Variant ini_get(boost::container::flat_set<std::string>& p) {
ArrayInit ret(p.size(), ArrayInit::Map{});
auto idx = 0;
VArrayInit ret(p.size());
for (auto& s : p) {
ret.add(idx++, s);
ret.append(s);
}
return ret.toArray();
}

template<typename T>
Variant ini_get(std::vector<T>& p) {
ArrayInit ret(p.size(), ArrayInit::Map{});
auto idx = 0;
VArrayInit ret(p.size());
for (auto& s : p) {
ret.add(idx++, s);
ret.append(s);
}
return ret.toArray();
}
Expand Down Expand Up @@ -518,7 +514,7 @@ const IniSettingMap ini_iterate(const IniSettingMap &ini,
// IniSettingMap

IniSettingMap::IniSettingMap() {
m_map = Variant(Array::Create());
m_map = Variant(Array::CreateDict());
}

IniSettingMap::IniSettingMap(Type /*t*/) : IniSettingMap() {}
Expand Down Expand Up @@ -555,7 +551,7 @@ void mergeSettings(tv_lval curval, TypedValue v) {
for (auto i = ArrayIter(v.m_data.parr); !i.end(); i.next()) {
auto& cur_inner_ref = asArrRef(curval);
if (!cur_inner_ref.exists(i.first())) {
cur_inner_ref.set(i.first(), empty_array());
cur_inner_ref.set(i.first(), Array::CreateDict());
}
mergeSettings(cur_inner_ref.lval(i.first()), i.secondVal());
}
Expand All @@ -569,7 +565,7 @@ void IniSettingMap::set(const String& key, const Variant& v) {
assertx(this->isArray());
auto& mapref = m_map.asArrRef();
if (!mapref.exists(key)) {
mapref.set(key, empty_array());
mapref.set(key, Array::CreateDict());
}
auto const curval = mapref.lval(key);
mergeSettings(curval, *v.asTypedValue());
Expand All @@ -578,6 +574,36 @@ void IniSettingMap::set(const String& key, const Variant& v) {
///////////////////////////////////////////////////////////////////////////////
// callbacks for creating arrays out of ini

Array IniSetting::ParserCallback::emptyArrayForMode() const {
switch (mode_) {
case ParserCallbackMode::DARRAY:
return Array::CreateDArray();
case ParserCallbackMode::DICT:
return Array::CreateDict();
}
not_reached();
}

Array& IniSetting::ParserCallback::forceToArrayForMode(Variant& var) const {
switch (mode_) {
case ParserCallbackMode::DARRAY:
return forceToDArray(var);
case ParserCallbackMode::DICT:
return forceToDict(var);
}
not_reached();
}

Array& IniSetting::ParserCallback::forceToArrayForMode(tv_lval var) const {
switch (mode_) {
case ParserCallbackMode::DARRAY:
return forceToDArray(var);
case ParserCallbackMode::DICT:
return forceToDict(var);
}
not_reached();
}

void IniSetting::ParserCallback::onSection(const std::string& /*name*/,
void* /*arg*/) {
// do nothing
Expand All @@ -593,7 +619,7 @@ void IniSetting::ParserCallback::onEntry(
auto arr = static_cast<Variant*>(arg);
String skey(key);
Variant sval(value);
forceToArray(*arr).set(skey, sval);
forceToArrayForMode(*arr).set(skey, sval);
}

void IniSetting::ParserCallback::onPopEntry(
Expand All @@ -602,15 +628,14 @@ void IniSetting::ParserCallback::onPopEntry(
const std::string &offset,
void *arg) {
auto arr = static_cast<Variant*>(arg);
forceToArray(*arr);

String skey(key);
auto& arr_ref = arr->asArrRef();
auto& arr_ref = forceToArrayForMode(*arr);
if (!arr_ref.exists(skey)) {
arr_ref.set(skey, empty_array());
arr_ref.set(skey, emptyArrayForMode());
}
auto const hash = arr_ref.lval(skey);
forceToArray(hash);
forceToArrayForMode(hash);
if (!offset.empty()) { // a[b]
makeArray(hash, offset, value);
} else { // a[]
Expand All @@ -633,17 +658,19 @@ void IniSetting::ParserCallback::makeArray(tv_lval val,
// hhvm.a[b][c][d]
// b will be hash and an array already, but c and d might
// not exist and will need to be made an array
auto& arr = forceToArray(val);
const auto key = arr.convertKey<IntishCast::Cast>(index);
auto& arr = forceToArrayForMode(val);
Variant key = index;
if (auto const intish = tryIntishCast<IntishCast::Cast>(index.get())) {
key = *intish;
}
if (last) {
String s{value};
arr.set(key, make_tv<KindOfString>(s.get()));
arr.set(key, value);
break;
}
// Similar to the above, in the case of a nested array we need to ensure
// that we create empty arrays on the way down if they don't already exist.
if (!arr.exists(key)) {
arr.set(key, make_array_like_tv(ArrayData::Create()));
arr.set(key, emptyArrayForMode());
}
val = arr.lval(key);
p += index.size() + 1;
Expand Down Expand Up @@ -697,7 +724,7 @@ void IniSetting::SectionParserCallback::onSection(
data->arr.asArrRef().set(data->active_name, data->active_section);
data->active_section.unset();
}
data->active_section = Array::Create();
data->active_section = emptyArrayForMode();
data->active_name = String(name);
}

Expand Down Expand Up @@ -776,17 +803,17 @@ Variant IniSetting::FromString(const String& ini, const String& filename,
Variant ret = false;
if (process_sections) {
CallbackData data;
SectionParserCallback cb;
data.arr = Array::Create();
SectionParserCallback cb(ParserCallbackMode::DARRAY);
data.arr = Array::CreateDArray();
if (zend_parse_ini_string(ini_cpp, filename_cpp, scanner_mode, cb, &data)) {
if (!data.active_name.isNull()) {
data.arr.asArrRef().set(data.active_name, data.active_section);
}
ret = data.arr;
}
} else {
ParserCallback cb;
Variant arr = Array::Create();
ParserCallback cb(ParserCallbackMode::DARRAY);
Variant arr = Array::CreateDArray();
if (zend_parse_ini_string(ini_cpp, filename_cpp, scanner_mode, cb, &arr)) {
ret = arr;
}
Expand All @@ -799,7 +826,7 @@ IniSettingMap IniSetting::FromStringAsMap(const std::string& ini,
Lock lock(s_mutex); // ini parser is not thread-safe
// We are parsing something new, so reset this flag
s_config_is_a_constant = false;
SystemParserCallback cb;
SystemParserCallback cb(ParserCallbackMode::DICT);
Variant parsed;
zend_parse_ini_string(ini, filename, NormalScanner, cb, &parsed);
if (parsed.isNull()) {
Expand Down Expand Up @@ -1117,7 +1144,7 @@ bool IniSetting::GetMode(const std::string& name, Mode& mode) {
}

Array IniSetting::GetAll(const String& ext_name, bool details) {
Array r = Array::Create();
Array r = Array::CreateDArray();

const Extension* ext = nullptr;
if (!ext_name.empty()) {
Expand Down Expand Up @@ -1147,7 +1174,7 @@ Array IniSetting::GetAll(const String& ext_name, bool details) {
value = value.toString();
}
if (details) {
Array item = Array::Create();
Array item = Array::CreateDArray();
item.set(s_global_value, value);
item.set(s_local_value, value);
if (iter.second.mode == PHP_INI_ALL) {
Expand Down
17 changes: 17 additions & 0 deletions hphp/runtime/base/ini-setting.h
View file
Expand Up @@ -172,7 +172,13 @@ struct IniSetting {
RawScanner,
};

enum struct ParserCallbackMode {
DARRAY = 0,
DICT = 1,
};

struct ParserCallback {
explicit ParserCallback(ParserCallbackMode mode) : mode_(mode) {}
virtual ~ParserCallback() {};
virtual void onSection(const std::string &name, void *arg);
virtual void onLabel(const std::string &name, void *arg);
Expand All @@ -187,15 +193,24 @@ struct IniSetting {
protected:
void makeArray(tv_lval hash, const std::string &offset,
const std::string &value);

Array emptyArrayForMode() const;
Array& forceToArrayForMode(Variant& var) const;
Array& forceToArrayForMode(tv_lval var) const;

private:
// Substitution copy or symlink via @ or : markers in the config line
void makeSettingSub(const String &key, const std::string &offset,
const std::string &value, Variant& cur_settings);
void traverseToSet(const String &key, const std::string& offset,
tv_lval value, Variant& cur_settings,
const std::string& stopChar);

ParserCallbackMode mode_;
};
struct SectionParserCallback : ParserCallback {
using ParserCallback::ParserCallback;

void onSection(const std::string& name, void* arg) override;
void onLabel(const std::string& name, void* arg) override;
void onEntry(const std::string& key, const std::string& value, void* arg)
Expand All @@ -210,6 +225,8 @@ struct IniSetting {
Variant* activeArray(CallbackData* data);
};
struct SystemParserCallback : ParserCallback {
using ParserCallback::ParserCallback;

void onEntry(const std::string& key, const std::string& value, void* arg)
override;
void onPopEntry(
Expand Down
2 changes: 1 addition & 1 deletion hphp/runtime/base/runtime-option.cpp
View file
Expand Up @@ -1396,7 +1396,7 @@ void RuntimeOption::Load(
const std::vector<std::string>& hdfClis /* = std::vector<std::string>() */,
std::vector<std::string>* messages /* = nullptr */,
std::string cmd /* = "" */) {
ARRPROV_USE_RUNTIME_LOCATION();
ARRPROV_USE_RUNTIME_LOCATION_FORCE();

// Intialize the memory manager here because various settings and
// initializations that we do here need it
Expand Down
1 change: 1 addition & 0 deletions hphp/runtime/vm/extern-compiler.cpp
View file
Expand Up @@ -886,6 +886,7 @@ void ExternCompiler::writeConfigs() {
// necessary to initialize zend-strtod, which is used to serialize
// boundConfig to JSON (!)
zend_get_bigint_data();
ARRPROV_USE_RUNTIME_LOCATION();
return IniSetting::GetAllAsJSON();
}
return "";
Expand Down

0 comments on commit df1cee0

Please sign in to comment.