Skip to content

Commit

Permalink
Fix a possible crash in zai config (#2906)
Browse files Browse the repository at this point in the history 
We might leave a dangling pointer when we update orig_value, but not an identical value.

Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>
  • Loading branch information
@bwoebi
bwoebi committed Oct 21, 2024
1 parent 46173ca commit b7a0f6f
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 16 deletions.
2 changes: 1 addition & 1 deletion Makefile
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ RUN_TESTS_CMD := REPORT_EXIT_STATUS=1 TEST_PHP_SRCDIR=$(PROJECT_ROOT) USE_TRACKE

C_FILES = $(shell find components components-rs ext src/dogstatsd zend_abstract_interface -name '*.c' -o -name '*.h' | awk '{ printf "$(BUILD_DIR)/%s\n", $$1 }' )
TEST_FILES = $(shell find tests/ext -name '*.php*' -o -name '*.inc' -o -name '*.json' -o -name 'CONFLICTS' | awk '{ printf "$(BUILD_DIR)/%s\n", $$1 }' )
RUST_FILES = $(BUILD_DIR)/Cargo.toml $(shell find components-rs -name '*.c' -o -name '*.rs' -o -name 'Cargo.toml' | awk '{ printf "$(BUILD_DIR)/%s\n", $$1 }' ) $(shell find libdatadog/{alloc,build-common,crashtracker,crashtracker-ffi,ddcommon,ddcommon-ffi,ddsketch,ddtelemetry,ddtelemetry-ffi,dogstatsd-client,dynamic-configuration,ipc,live-debugger,live-debugger-ffi,remote-config,sidecar,sidecar-ffi,spawn_worker,tinybytes,tools/{cc_utils,sidecar_mockgen},trace-*,Cargo.toml} -type f \( -path "*/src*" -o -path "*/examples*" -o -path "*Cargo.toml" -o -path "*/build.rs" -o -path "*/tests/dataservice.rs" -o -path "*/tests/service_functional.rs" \) -not -path "*/ipc/build.rs" -not -path "*/sidecar-ffi/build.rs")
RUST_FILES = $(BUILD_DIR)/Cargo.toml $(BUILD_DIR)/Cargo.lock $(shell find components-rs -name '*.c' -o -name '*.rs' -o -name 'Cargo.toml' | awk '{ printf "$(BUILD_DIR)/%s\n", $$1 }' ) $(shell find libdatadog/{alloc,build-common,crashtracker,crashtracker-ffi,ddcommon,ddcommon-ffi,ddsketch,ddtelemetry,ddtelemetry-ffi,dogstatsd-client,dynamic-configuration,ipc,live-debugger,live-debugger-ffi,remote-config,sidecar,sidecar-ffi,spawn_worker,tinybytes,tools/{cc_utils,sidecar_mockgen},trace-*,Cargo.toml} -type f \( -path "*/src*" -o -path "*/examples*" -o -path "*Cargo.toml" -o -path "*/build.rs" -o -path "*/tests/dataservice.rs" -o -path "*/tests/service_functional.rs" \) -not -path "*/ipc/build.rs" -not -path "*/sidecar-ffi/build.rs")
ALL_OBJECT_FILES = $(C_FILES) $(RUST_FILES) $(BUILD_DIR)/Makefile
TEST_OPCACHE_FILES = $(shell find tests/opcache -name '*.php*' -o -name '.gitkeep' | awk '{ printf "$(BUILD_DIR)/%s\n", $$1 }' )
TEST_STUB_FILES = $(shell find tests/ext -type d -name 'stubs' -exec find '{}' -type f \; | awk '{ printf "$(BUILD_DIR)/%s\n", $$1 }' )
Expand Down
33 changes: 18 additions & 15 deletions zend_abstract_interface/config/config_ini.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ int16_t zai_config_initialize_ini_value(zend_ini_entry **entries,
}
zai_json_dtor_pzval(&new_zv);

parsed_ini_value = zend_string_copy(ini_str);
parsed_ini_value = ini_str;
name_index = i;
break;
}
Expand All @@ -93,7 +93,7 @@ int16_t zai_config_initialize_ini_value(zend_ini_entry **entries,

for (int16_t i = 0; i < ini_count; ++i) {
if (entries[i]->modified && !runtime_value && (is_minit || entries[i]->modifiable == PHP_INI_SYSTEM)) {
runtime_value = zend_string_copy(entries[i]->value);
runtime_value = entries[i]->value;
}
zval *inizv = cfg_get_entry(ZSTR_VAL(entries[i]->name), ZSTR_LEN(entries[i]->name));
if (inizv != NULL && !parsed_ini_value) {
Expand All @@ -105,7 +105,7 @@ int16_t zai_config_initialize_ini_value(zend_ini_entry **entries,
}
zai_json_dtor_pzval(&new_zv);

parsed_ini_value = zend_string_copy(Z_STR_P(inizv));
parsed_ini_value = Z_STR_P(inizv);
name_index = i;
break;
}
Expand All @@ -123,19 +123,27 @@ int16_t zai_config_initialize_ini_value(zend_ini_entry **entries,
continue;
}

zend_string **target = entries[i]->modified ? &entries[i]->orig_value : &entries[i]->value;
zend_string *new_value = NULL;
if (i > 0) {
zend_string_release(*target);
*target = zend_string_copy(entries[0]->modified ? entries[0]->orig_value : entries[0]->value);
new_value = zend_string_copy(entries[0]->modified ? entries[0]->orig_value : entries[0]->value);
} else if (zai_option_str_is_some(*buf)) {
zend_string_release(*target);
*target = zend_string_init(buf->ptr, buf->len, 1);
new_value = zend_string_init(buf->ptr, buf->len, 1);
} else if (parsed_ini_value != NULL) {
new_value = zend_string_copy(parsed_ini_value);
}
if (new_value) {
zend_string **target = entries[i]->modified ? &entries[i]->orig_value : &entries[i]->value;
zend_string_release(*target);
*target = zend_string_copy(parsed_ini_value);
if (entries[i]->modified && entries[i]->orig_value == entries[i]->value) {
// Handle edge case where a value was previously attempted to be modified, but on_modify failed.
// In that case orig_value == value, but without a refcount increase. Update it to avoid running into a use after free.
entries[i]->value = new_value;
}
*target = new_value;
}

if (runtime_value) {
new_value = zend_string_copy(runtime_value);
if (entries[i]->modified) {
if (entries[i]->value != entries[i]->orig_value) {
zend_string_release(entries[i]->value);
Expand All @@ -146,24 +154,19 @@ int16_t zai_config_initialize_ini_value(zend_ini_entry **entries,
entries[i]->orig_modifiable = entries[i]->modifiable;
zend_hash_add_ptr(EG(modified_ini_directives), entries[i]->name, entries[i]);
}
entries[i]->value = zend_string_copy(runtime_value);
entries[i]->value = new_value;
}
}
}

if (runtime_value) {
buf->ptr = ZSTR_VAL(runtime_value);
buf->len = ZSTR_LEN(runtime_value);
zend_string_release(runtime_value);
} else if (parsed_ini_value && zai_option_str_is_none(*buf)) {
buf->ptr = ZSTR_VAL(parsed_ini_value);
buf->len = ZSTR_LEN(parsed_ini_value);
}

if (parsed_ini_value) {
zend_string_release(parsed_ini_value);
}

#if ZTS
#ifndef _WIN32
pthread_rwlock_unlock(&lock_ini_init_rw);
Expand Down

0 comments on commit b7a0f6f

Please sign in to comment.