Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

appsec: fix recv/writev calls in the face of interrupting signals #3008

Merged
merged 2 commits into from
Jan 6, 2025
Merged

appsec: fix recv/writev calls in the face of interrupting signals #3008

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cf7254e
appsec: fix recv/writev calls in the face of interrupting signals
cataphract Dec 19, 2024
41d91c4
fix leak on one of the error branches of _dd_command_exec
cataphract Dec 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 16 additions & 45 deletions appsec/src/extension/commands_helpers.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,6 @@ static inline ATTR_WARN_UNUSED mpack_error_t _omsg_finish(
static inline void _omsg_destroy(dd_omsg *nonnull omsg);
static inline dd_result _omsg_send(
dd_conn *nonnull conn, dd_omsg *nonnull omsg);
static inline dd_result _omsg_send_cred(
dd_conn *nonnull conn, dd_omsg *nonnull omsg);
static void _dump_in_msg(
dd_log_level_t lvl, const char *nonnull data, size_t data_len);
static void _dump_out_msg(dd_log_level_t lvl, zend_llist *iovecs);
Expand All @@ -42,16 +40,14 @@ typedef struct _dd_imsg {
mpack_node_t root;
} dd_imsg;

// iif these two return success, _imsg_destroy must be called
static inline dd_result _imsg_recv(
dd_imsg *nonnull imsg, dd_conn *nonnull conn);
static inline ATTR_WARN_UNUSED dd_result _imsg_recv_cred(
// iif this returns success, _imsg_destroy must be called
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, it's correct, _imsg_destroy should be called if and only if _imsg_recv returns success. I did spot a branch where it was not being called, though, so I added a new commit to use a gcc/clang extension to prevent this.

static dd_result ATTR_WARN_UNUSED _imsg_recv(
dd_imsg *nonnull imsg, dd_conn *nonnull conn);

static inline ATTR_WARN_UNUSED mpack_error_t _imsg_destroy(
dd_imsg *nonnull imsg);

static dd_result _dd_command_exec(dd_conn *nonnull conn, bool check_cred,
static dd_result _dd_command_exec(dd_conn *nonnull conn,
const dd_command_spec *nonnull spec, void *unspecnull ctx)
{
#define NAME_L (int)spec->name_len, spec->name
Expand All @@ -78,11 +74,7 @@ static dd_result _dd_command_exec(dd_conn *nonnull conn, bool check_cred,
return dd_error;
}

if (check_cred) {
res = _omsg_send_cred(conn, &omsg);
} else {
res = _omsg_send(conn, &omsg);
}
res = _omsg_send(conn, &omsg);
_dump_out_msg(dd_log_trace, &omsg.iovecs);
_omsg_destroy(&omsg);
if (res) {
Expand All @@ -96,11 +88,7 @@ static dd_result _dd_command_exec(dd_conn *nonnull conn, bool check_cred,
dd_result res;
{
dd_imsg imsg = {0};
if (check_cred) {
res = _imsg_recv_cred(&imsg, conn);
} else {
res = _imsg_recv(&imsg, conn);
}
res = _imsg_recv(&imsg, conn);
if (res) {
if (res != dd_helper_error) {
mlog(dd_log_warning,
Expand Down Expand Up @@ -194,20 +182,25 @@ static dd_result _dd_command_exec(dd_conn *nonnull conn, bool check_cred,
dd_result ATTR_WARN_UNUSED dd_command_exec(dd_conn *nonnull conn,
const dd_command_spec *nonnull spec, void *unspecnull ctx)
{
return _dd_command_exec(conn, false, spec, ctx);
return _dd_command_exec(conn, spec, ctx);
}

dd_result ATTR_WARN_UNUSED dd_command_exec_req_info(dd_conn *nonnull conn,
const dd_command_spec *nonnull spec, struct req_info *nonnull ctx)
{
ctx->command_name = spec->name;
return _dd_command_exec(conn, false, spec, ctx);
return _dd_command_exec(conn, spec, ctx);
}

dd_result ATTR_WARN_UNUSED dd_command_exec_cred(dd_conn *nonnull conn,
const dd_command_spec *nonnull spec, void *unspecnull ctx)
{
return _dd_command_exec(conn, true, spec, ctx);
dd_result res = dd_conn_check_credentials(conn);
if (res) {
return res;
}

return _dd_command_exec(conn, spec, ctx);
}

// outgoing
Expand Down Expand Up @@ -247,24 +240,13 @@ static inline dd_result _omsg_send(dd_conn *nonnull conn, dd_omsg *nonnull omsg)
return dd_conn_sendv(conn, &omsg->iovecs);
}

static inline dd_result _omsg_send_cred(
dd_conn *nonnull conn, dd_omsg *nonnull omsg)
{
return dd_conn_sendv_cred(conn, &omsg->iovecs);
}

// incoming
static inline dd_result _dd_imsg_recv(
dd_imsg *nonnull imsg, dd_conn *nonnull conn, bool check_cred)
static ATTR_WARN_UNUSED dd_result _imsg_recv(
dd_imsg *nonnull imsg, dd_conn *nonnull conn)
{
mlog(dd_log_debug, "Will receive response from helper");

dd_result res;
if (check_cred) {
res = dd_conn_recv_cred(conn, &imsg->_data, &imsg->_size);
} else {
res = dd_conn_recv(conn, &imsg->_data, &imsg->_size);
}
dd_result res = dd_conn_recv(conn, &imsg->_data, &imsg->_size);
if (res) {
return res;
}
Expand All @@ -290,17 +272,6 @@ static inline dd_result _dd_imsg_recv(
return dd_success;
}

ATTR_WARN_UNUSED dd_result _imsg_recv(
dd_imsg *nonnull imsg, dd_conn *nonnull conn)
{
return _dd_imsg_recv(imsg, conn, false);
}
ATTR_WARN_UNUSED dd_result _imsg_recv_cred(
dd_imsg *nonnull imsg, dd_conn *nonnull conn)
{
return _dd_imsg_recv(imsg, conn, true);
}

static inline ATTR_WARN_UNUSED mpack_error_t _imsg_destroy(
dd_imsg *nonnull imsg)
{
Expand Down
Loading
Loading