memcached: apply patch: fix crash in binary protocol - #18459
memcached: apply patch: fix crash in binary protocol#18459Chris Gunn (cwize1) wants to merge 2 commits into
Conversation
microsoft-github-policy-service
Bot
added
Packaging
3.0-dev
|
/azurepipelines run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). 1 pipeline(s) were filtered out due to trigger conditions. |
Kanishk Bansal (Kanishk-Bansal)
left a comment
Kanishk Bansal (Kanishk-Bansal)
left a comment
There was a problem hiding this comment.
Patch Analysis (Matches Upstream)
- Buddy Build
- patch applied during the build (check
rpm.log) - patch include an upstream reference
- PR has security tag
Kanishk Bansal (Kanishk-Bansal)
added
the
ready-for-stable-review
| LOGGER_LOG(c->thread->l, LOG_MUTATIONS, LOGGER_ITEM_STORE, | ||
| NULL, status, 0, key, nkey, req->message.body.expiration, | ||
| - ITEM_clsid(it), c->sfd); | ||
| + 0, c->sfd); |
There was a problem hiding this comment.
issue (blocking): supply the missing nbytes argument before making this logger path reachable
LOGGER_ITEM_STORE consumes eight variadic fields after entry:
status, comm, key, nkey, nbytes, ttl, clsid, sfdThis call supplies only seven:
status, 0, key, nkey, expiration, 0, c->sfdBefore this patch, evaluating ITEM_clsid(it) crashes because it is NULL. After replacing it with 0, the logger runs and reads one argument past the supplied list. The fields are shifted (expiration becomes nbytes, 0 becomes ttl, and c->sfd becomes clsid), while sfd is read from undefined variadic state. That can corrupt the mutation record and potentially expose stale process data to a watch mutations client.
Please include all eight fields. Based on the successful-store call contract, the failure path should resemble:
| + 0, c->sfd); | |
| NULL, status, 0, key, nkey, vlen + 2, | |
| realtime(req->message.body.expiration), 0, c->sfd); |
The same seven-argument mismatch is still present on upstream master, so this should also be reported upstream.
Upstream evidence:
There was a problem hiding this comment.
Reviewing this closer, given that this issue is present in upstream and is a cosmetic logging defect (key and nkey are still correct. nbytes is a log line element, and not part of any memcpy operations), I will change this to non-blocking. It is more important to get this initial crash fixed, and the logging defect is cosmetic.
We should also report this logging issue upstream, and if fixed upstream, backport that fix to our version.
|
Could you please check if this PR: #18504 supersedes this? |
4 participants
No description provided.