Fix unhandled dd_network; fix log message; fix handling of helper errors

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);

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;

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));

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);

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--
+<?php
+use function datadog\appsec\testing\rinit;
+
+include __DIR__ . '/inc/mock_helper.php';
+require __DIR__ . '/inc/logging.php';
+
+$helper = Helper::createInitedRun([
+    response_list(response("error", []))
+]);
+
+var_dump(rinit());
+
+match_log("/Helper responded with an error\. Check helper logs/");
+match_log("/request init failed: dd_helper_error/");
+
+?>
+--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/
+