diff --git a/appsec/src/extension/commands_helpers.c b/appsec/src/extension/commands_helpers.c index abf21c6fb1..2a8effe534 100644 --- a/appsec/src/extension/commands_helpers.c +++ b/appsec/src/extension/commands_helpers.c @@ -105,6 +105,8 @@ static dd_result _dd_command_exec(dd_conn *nonnull conn, __attribute__((cleanup(_imsg_cleanup))) dd_imsg *nullable destroy_imsg = &imsg; + // TODO: it seems we only look at the first response? Why even support + // several responses then? mpack_node_t first_response = mpack_node_array_at(imsg.root, 0); mpack_error_t err = mpack_node_error(first_response); if (err != mpack_ok) { @@ -142,6 +144,10 @@ static dd_result _dd_command_exec(dd_conn *nonnull conn, res = spec->config_features_cb(first_message, ctx); } else if (dd_mpack_node_str_eq(type, spec->name, spec->name_len)) { res = spec->incoming_cb(first_message, ctx); + } else if (dd_mpack_node_lstr_eq(type, "error")) { + mlog(dd_log_info, + "Helper responded with an error. Check helper logs"); + return dd_helper_error; } else { mlog(dd_log_debug, "Received message for command %.*s unexpected: %.*s\n", NAME_L, @@ -249,12 +255,6 @@ static ATTR_WARN_UNUSED dd_result _imsg_recv( return res; } - if (imsg->_size == 1) { - // The helper process sent an error response, this is a non-fatal - // error to indicate the message could not be processed. - return dd_helper_error; - } - mpack_tree_init(&imsg->_tree, imsg->_data, imsg->_size); mpack_tree_parse(&imsg->_tree); imsg->root = mpack_tree_root(&imsg->_tree); diff --git a/appsec/src/extension/network.c b/appsec/src/extension/network.c index ed74b94691..8df53c2e9a 100644 --- a/appsec/src/extension/network.c +++ b/appsec/src/extension/network.c @@ -282,9 +282,12 @@ dd_result dd_conn_recv(dd_conn *nonnull conn, char *nullable *nonnull data, if (memcmp(h.code, "dds", 4) != 0) { mlog(dd_log_warning, - "Invalid message header from helper. First four bytes are " - "0x%02X%02X%02X%02x", - h.code[0], h.code[1], h.code[2], h.code[3]); + "Invalid message header from helper. First eight bytes are " + "%02X%02X%02X%02x%02X%02X%02X%02x", + ((unsigned char *)&h)[0], ((unsigned char *)&h)[1], + ((unsigned char *)&h)[2], ((unsigned char *)&h)[3], + ((unsigned char *)&h)[4], ((unsigned char *)&h)[5], + ((unsigned char *)&h)[6], ((unsigned char *)&h)[7]); // to force the connection closed. It may be we half-read a previous // message, so a reconnection can help return dd_network; diff --git a/appsec/src/extension/request_lifecycle.c b/appsec/src/extension/request_lifecycle.c index 662db69470..476344ffae 100644 --- a/appsec/src/extension/request_lifecycle.c +++ b/appsec/src/extension/request_lifecycle.c @@ -278,7 +278,12 @@ void dd_req_lifecycle_rshutdown(bool ignore_verdict, bool force) } else { dd_result res = dd_config_sync(conn, &(struct config_sync_data){.rem_cfg_path = _last_rem_cfg_path}); - if (res) { + if (res == dd_network) { + mlog_g(dd_log_info, "request_init/config_sync failed with " + "dd_network; closing " + "connection to helper"); + dd_helper_close_conn(); + } else if (res) { mlog_g(dd_log_info, "Failed to sync remote config path on rshutdown: %s", dd_result_to_string(res)); diff --git a/appsec/src/extension/user_tracking.c b/appsec/src/extension/user_tracking.c index f930293373..badc6e1ab5 100644 --- a/appsec/src/extension/user_tracking.c +++ b/appsec/src/extension/user_tracking.c @@ -132,6 +132,12 @@ void dd_find_and_apply_verdict_for_user(zend_string *nonnull user_id) Z_ARRVAL(data_zv), "usr.id", sizeof("usr.id") - 1, &user_id_zv); dd_result res = dd_request_exec(conn, &data_zv, false); + if (res == dd_network) { + mlog_g(dd_log_info, "request_exec failed with dd_network; closing " + "connection to helper"); + dd_helper_close_conn(); + } + zval_ptr_dtor(&data_zv); dd_tags_set_event_user_id(user_id); diff --git a/appsec/tests/extension/helper_error_message.phpt b/appsec/tests/extension/helper_error_message.phpt new file mode 100644 index 0000000000..1b11aa3485 --- /dev/null +++ b/appsec/tests/extension/helper_error_message.phpt @@ -0,0 +1,27 @@ +--TEST-- +Test when helper responds with error +--INI-- +datadog.appsec.log_file=/tmp/php_appsec_test.log +datadog.appsec.log_level=debug +--FILE-- + +--EXPECTF-- +bool(true) +found message in log matching /Helper responded with an error\. Check helper logs/ +found message in log matching /request init failed: dd_helper_error/ +