Skip to content

{184324980} Add bounds check for legacy request buffers#5958

Open
mponomar wants to merge 1 commit into
bloomberg:mainfrom
mponomar:dispatch-tagged-bounds-check
Open

{184324980} Add bounds check for legacy request buffers#5958
mponomar wants to merge 1 commit into
bloomberg:mainfrom
mponomar:dispatch-tagged-bounds-check

Conversation

@mponomar
Copy link
Copy Markdown
Contributor

No description provided.

@mponomar mponomar force-pushed the dispatch-tagged-bounds-check branch from 4cb0175 to 81751e2 Compare May 21, 2026 14:08
Copy link
Copy Markdown

@roborivers roborivers left a comment

Choose a reason for hiding this comment

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

Cbuild submission: Success ✓.
Regression testing: Success ✓.

The first 10 failing tests are:
logfill [db unavailable at finish] **quarantined**
sc_timepart_multiddl_generated
consumer_non_atomic_default_consumer_generated **quarantined**

hdr.opcode = 255; // invalid opcode
sz = sizeof(hdr);
memcpy(p_slock->bigbuf, &hdr, sz);
// application should get a 199 (ERR_BADREQ)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should buf be made to point to p_slock->bigbuf too? Or point to &hdr even? I asked because buf still gets used below (e.g., in line 525, the req_hdr_get call) and I find it a bit confusing that it's pointing to data of invalid size - even if sz invariably addresses the bound checking issue itself.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure. It can be set for consistency.

Signed-off-by: Mike Ponomarenko <mponomarenko@bloomberg.net>
@mponomar mponomar force-pushed the dispatch-tagged-bounds-check branch from 81751e2 to 5934e9e Compare May 27, 2026 15:12
Copy link
Copy Markdown

@roborivers roborivers left a comment

Choose a reason for hiding this comment

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

Cbuild submission: Success ✓.
Regression testing: Success ✓.

The first 10 failing tests are:
sc_truncate [db unavailable at finish]
consumer_non_atomic_default_consumer_generated **quarantined**
reco-ddlk-sql [timeout] **quarantined**

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants