validate trigger registration lengths - #6184
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens trigger registration/unregistration message handling by validating NET_TRIGGER_REGISTER / NET_TRIGGER_UNREGISTER payload lengths before parsing and by removing an unbounded strcpy() into the stored procedure name buffer.
Changes:
- Add
valid_trigger_reg_payload()to validate incoming trigger register/unregister packets againstdtalenbefore invoking trigger logic. - Replace
strcpy(info->spname, ...)with boundedmemcpy(..., spname_len)plus explicit NUL termination. - Add early rejection/logging for malformed trigger registration/unregistration packets.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| db/trigger.c | Replaces unbounded string copy when storing trigger SP name. |
| db/glue.c | Adds payload-length validation for trigger register/unregister network handlers. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (!valid_trigger_reg_payload(dtap, dtalen)) { | ||
| logmsg(LOGMSG_ERROR, | ||
| "%s: invalid trigger registration payload length %d\n", | ||
| __func__, dtalen); | ||
| return; | ||
| } |
| if (!valid_trigger_reg_payload(dtap, dtalen)) { | ||
| logmsg(LOGMSG_ERROR, | ||
| "%s: invalid trigger unregister payload length %d\n", | ||
| __func__, dtalen); | ||
| return; | ||
| } |
| add: info = malloc(sizeof(trigger_info_t) + (size_t)t->spname_len + 1); | ||
| info->host = intern(trigger_hostname(t)); | ||
| info->trigger_cookie = t->trigger_cookie; | ||
| info->hbeat = now; | ||
| strcpy(info->spname, t->spname); | ||
| memcpy(info->spname, t->spname, (size_t)t->spname_len); |
| const char *hostname = spname + spname_len + 1; | ||
|
|
||
| if (memchr(hostname, '\0', remaining) == NULL) | ||
| return 0; |
2805860 to
063d19c
Compare
roborivers
left a comment
roborivers
left a comment
There was a problem hiding this comment.
Cbuild submission: Success ✓.
Regression testing: Success ✓.
The first 10 failing tests are:
logfill [db unavailable at finish] **quarantined**
consumer_non_atomic_default_consumer_generated **quarantined**
sc_downgrade [timeout] **quarantined**
|
/plugin-branch trigger-length |
roborivers
left a comment
roborivers
left a comment
There was a problem hiding this comment.
Cbuild submission: Success ✓.
Regression testing: Success ✓.
The first 10 failing tests are:
sc_redo [failed with core dumped]
consumer_non_atomic_default_consumer_generated **quarantined**
sc_downgrade [timeout] **quarantined**
roborivers
left a comment
roborivers
left a comment
There was a problem hiding this comment.
Cbuild submission: Success ✓.
Regression testing: Success ✓.
The first 10 failing tests are:
consumer_non_atomic_default_consumer_generated **quarantined**
sc_downgrade [timeout] **quarantined**
sc_truncate_lockorder_generated [timeout] **quarantined**
reco-ddlk-sql [timeout] **quarantined**
|
/plugin-branch trigger-register-length |
|
/adhoc-test tsa |
|
/plugin-branch trigger-register-length |
|
/runtests |
Validate the raw NET_TRIGGER_REGISTER / NET_TRIGGER_UNREGISTER payload against the actual received length before parsing, using offsetof(trigger_reg_t, spname) as the variable-data boundary and reading spname_len into a temporary (no double byteswap, no packet mutation). Also replace the unbounded strcpy() into info->spname with a bounded memcpy() plus explicit NUL terminator.
063d19c to
7f99f29
Compare
|
/plugin-branch trigger-register-length |
roborivers
left a comment
roborivers
left a comment
There was a problem hiding this comment.
Cbuild submission: Error ⚠.
Regression testing: Success ✓.
The first 10 failing tests are:
sc_redo [failed with core dumped]
comdb2sys **quarantined**
ssl_san
consumer_non_atomic_default_consumer_generated **quarantined**
ssl_set_cmd
ssl_dbname
ssl_prefer
sc_downgrade [timeout] **quarantined**
reco-ddlk-sql [timeout] **quarantined**
sql_logfill_autodisable [timeout]
Validate the raw NET_TRIGGER_REGISTER / NET_TRIGGER_UNREGISTER payload against the actual received length before parsing. Also replace the unbounded strcpy() into info->spname with a bounded memcpy() plus explicit NUL terminator.