Skip to content

Commit 8f21029

Browse files
bwoebibwoebi
authored
Mark config as immutable and interned to avoid refcounting race conditions (#2516)
* Mark config as immutable and interned to avoid refcounting race conditions Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com> * Use #if PHP_VERSION_ID around immutable typeflags Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com> * Fix PHP 7.2 compatibility Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com> --------- Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>
1 parent 2df1f16 commit 8f21029

8 files changed

Lines changed: 89 additions & 11 deletions

File tree

‎appsec/src/extension/configuration.c‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -226,7 +226,7 @@ bool dd_config_minit(int module_number)
226226
// places wishing to use values pre-RINIT do have to explicitly opt in by
227227
// using the arduous way of accessing the decoded_value directly from
228228
// zai_config_memoized_entries.
229-
zai_config_first_time_rinit();
229+
zai_config_first_time_rinit(false);
230230
#ifdef TESTING
231231
_register_testing_objects();
232232
#endif
@@ -236,7 +236,7 @@ bool dd_config_minit(int module_number)
236236

237237
void dd_config_first_rinit()
238238
{
239-
zai_config_first_time_rinit();
239+
zai_config_first_time_rinit(true);
240240
zai_config_rinit();
241241

242242
runtime_config_first_init = true;

‎ext/configuration.c‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -140,7 +140,7 @@ bool ddtrace_config_minit(int module_number) {
140140
// Note that we are not calling zai_config_rinit(), i.e. the get_...() functions will not work.
141141
// This is intentional, so that places wishing to use values pre-RINIT do have to explicitly opt in by using the
142142
// arduous way of accessing the decoded_value directly from zai_config_memoized_entries.
143-
zai_config_first_time_rinit();
143+
zai_config_first_time_rinit(false);
144144

145145
ddtrace_log_ginit();
146146
return true;
@@ -152,7 +152,7 @@ void ddtrace_config_first_rinit() {
152152
zend_string *internal_functions_old = zend_string_copy(
153153
internal_functions_ini->modified ? internal_functions_ini->orig_value : internal_functions_ini->value);
154154

155-
zai_config_first_time_rinit();
155+
zai_config_first_time_rinit(true);
156156
zai_config_rinit();
157157

158158
zend_string *internal_functions_new =

‎profiling/src/lib.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -399,7 +399,7 @@ extern "C" fn rinit(_type: c_int, _module_number: c_int) -> ZendResult {
399399

400400
// SAFETY: not being mutated during rinit.
401401
unsafe { &ZAI_CONFIG_ONCE }.call_once(|| unsafe {
402-
bindings::zai_config_first_time_rinit();
402+
bindings::zai_config_first_time_rinit(true);
403403
config::first_rinit();
404404
});
405405

‎zend_abstract_interface/config/config.c‎

Lines changed: 65 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -140,11 +140,75 @@ void zai_config_mshutdown(void) {
140140
void zai_config_runtime_config_ctor(void);
141141
void zai_config_runtime_config_dtor(void);
142142

143-
void zai_config_first_time_rinit(void) {
143+
#if PHP_VERSION_ID < 70300
144+
#define GC_ADD_FLAGS(c, flag) GC_FLAGS(c) |= flag
145+
#define GC_ADDREF(p) ++GC_REFCOUNT(p)
146+
#endif
147+
148+
static void zai_config_intern_zval(zval *pzval) {
149+
if (Z_TYPE_P(pzval) == IS_STRING) {
150+
#if PHP_VERSION_ID >= 70400
151+
ZVAL_INTERNED_STR(pzval, zend_new_interned_string(Z_STR_P(pzval)));
152+
#else
153+
GC_ADD_FLAGS(Z_STR_P(pzval), IS_STR_INTERNED);
154+
Z_TYPE_INFO_P(pzval) = IS_INTERNED_STRING_EX;
155+
#endif
156+
}
157+
if (Z_TYPE_P(pzval) == IS_ARRAY) {
158+
GC_ADDREF(Z_ARR_P(pzval));
159+
GC_ADD_FLAGS(Z_ARR_P(pzval), IS_ARRAY_IMMUTABLE);
160+
#if PHP_VERSION_ID < 70200
161+
Z_TYPE_FLAGS_P(pzval) = IS_TYPE_IMMUTABLE;
162+
#elif PHP_VERSION_ID < 70300
163+
Z_TYPE_FLAGS_P(pzval) = IS_TYPE_COPYABLE;
164+
#else
165+
Z_TYPE_FLAGS_P(pzval) = 0;
166+
#endif
167+
168+
#if PHP_VERSION_ID >= 80200
169+
if (HT_IS_PACKED(Z_ARR_P(pzval))) {
170+
zval *zv;
171+
ZEND_HASH_FOREACH_VAL(Z_ARR_P(pzval), zv) {
172+
zai_config_intern_zval(zv);
173+
} ZEND_HASH_FOREACH_END();
174+
} else
175+
#endif
176+
{
177+
Bucket *bucket;
178+
ZEND_HASH_FOREACH_BUCKET(Z_ARR_P(pzval), bucket) {
179+
if (bucket->key) {
180+
#if PHP_VERSION_ID >= 70400
181+
bucket->key = zend_new_interned_string(bucket->key);
182+
#else
183+
GC_ADD_FLAGS(bucket->key, IS_STR_INTERNED);
184+
#endif
185+
}
186+
zai_config_intern_zval(&bucket->val);
187+
} ZEND_HASH_FOREACH_END();
188+
}
189+
}
190+
}
191+
192+
void zai_config_first_time_rinit(bool in_request) {
193+
#if PHP_VERSION_ID >= 70400
194+
if (in_request) {
195+
zend_interned_strings_switch_storage(0);
196+
}
197+
#endif
198+
144199
for (uint8_t i = 0; i < zai_config_memoized_entries_count; i++) {
145200
zai_config_memoized_entry *memoized = &zai_config_memoized_entries[i];
146201
zai_config_find_and_set_value(memoized, i);
202+
if (in_request) {
203+
zai_config_intern_zval(&memoized->decoded_value);
204+
}
205+
}
206+
207+
#if PHP_VERSION_ID >= 70400
208+
if (in_request) {
209+
zend_interned_strings_switch_storage(1);
147210
}
211+
#endif
148212
}
149213

150214
void zai_config_rinit(void) {

‎zend_abstract_interface/config/config.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,7 @@ void zai_config_mshutdown(void);
8080
// Not thread-safe; must block (Use pthread_once)
8181
// Must be called before zai_config_rinit()
8282
// Update decoded_value with env/ini value if exists
83-
void zai_config_first_time_rinit(void);
83+
void zai_config_first_time_rinit(bool in_request);
8484

8585
// Runtime config ctor (++rc)
8686
void zai_config_rinit(void);

‎zend_abstract_interface/config/tests/ext_zai_config.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ static PHP_RINIT_FUNCTION(zai_config) {
4646

4747
int expected_first_rinit = 1;
4848
if (atomic_compare_exchange_strong(&ext_first_rinit, &expected_first_rinit, 0)) {
49-
zai_config_first_time_rinit();
49+
zai_config_first_time_rinit(true);
5050
}
5151

5252
zai_config_rinit();

‎zend_abstract_interface/config/tests/ini.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -528,7 +528,7 @@ TEST_INI("setting perdir INI setting for multiple ZAI config users", {
528528
zai_config_memoized_entry *entry = &zai_config_memoized_entries[EXT_CFG_INI_FOO_STRING];
529529
entry->original_on_modify = dummy;
530530

531-
zai_config_first_time_rinit();
531+
zai_config_first_time_rinit(true);
532532
REQUEST_BEGIN();
533533

534534
zval *value = zai_config_get_value(EXT_CFG_INI_FOO_STRING);

‎zend_abstract_interface/json/json.c‎

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -150,10 +150,11 @@ void zai_json_shutdown_bindings(void) {
150150
}
151151

152152
void zai_json_release_persistent_array(HashTable *ht) {
153+
uint32_t immutable = (GC_FLAGS(ht) & IS_ARRAY_IMMUTABLE) != 0;
153154
#if PHP_VERSION_ID < 70300
154-
if (--GC_REFCOUNT(ht) == 0)
155+
if (--GC_REFCOUNT(ht) == immutable)
155156
#else
156-
if (GC_DELREF(ht) == 0)
157+
if (GC_DELREF(ht) == immutable)
157158
#endif
158159
{
159160
zend_hash_destroy(ht);
@@ -164,7 +165,20 @@ void zai_json_release_persistent_array(HashTable *ht) {
164165
void zai_json_dtor_pzval(zval *pval) {
165166
if (Z_TYPE_P(pval) == IS_ARRAY) {
166167
zai_json_release_persistent_array(Z_ARR_P(pval));
168+
#if PHP_VERSION_ID >= 70400
169+
} else if (Z_TYPE_P(pval) == IS_STRING && ZSTR_IS_INTERNED(Z_STR_P(pval))) {
170+
// nothing
171+
#endif
167172
} else {
173+
#if PHP_VERSION_ID >= 70200 && PHP_VERSION_ID < 70400
174+
if (Z_TYPE_P(pval) == IS_STRING && ZSTR_IS_INTERNED(Z_STR_P(pval)) && !zend_interned_string_find_permanent(Z_STR_P(pval))) {
175+
#if PHP_VERSION_ID >= 70300
176+
GC_DEL_FLAGS(Z_STR_P(pval), IS_STR_INTERNED);
177+
#else
178+
GC_FLAGS(Z_STR_P(pval)) &= ~IS_STR_INTERNED;
179+
#endif
180+
}
181+
#endif
168182
zval_internal_ptr_dtor(pval);
169183
}
170184
// Prevent an accidental use after free

0 commit comments

Comments
 (0)