Skip to content

UCT/TCP: Validate AM receive length - v1.23.x - #12124

Merged
roiedanino merged 1 commit into
openucx:v1.23.xfrom
roiedanino:v1.23.x-tcp-recv-len
Oct 20, 2026
Merged

roiedanino merged 1 commit into
openucx:v1.23.xfrom
roiedanino:v1.23.x-tcp-recv-len

Conversation

@roiedanino

Copy link
Copy Markdown
Contributor

What?

Backport #12081 to v1.23.x.

Validate TCP active-message receive lengths at runtime and disconnect a peer when its advertised payload is larger than the receive segment. Include the focused raw TCP regression from the original change.

Why?

The receive path previously relied on an assertion for this wire-input invariant. Assertions are disabled in release builds, so an invalid length could be retained and used later.

* UCT/TCP: Validate AM receive length

Validate wire-provided AM lengths at runtime before retaining the header for a later receive. Disconnect peers that advertise a payload larger than the configured receive segment.\n\nAdd a raw TCP regression that sends the protocol magic and an oversized AM header.

* UCT/TCP: Improve oversized AM diagnostics

Report rejected oversized messages at diagnostic level and use a compile-time constant for the TCP magic value in the regression test.

* UCT/TCP: Reuse maximum AM length

(cherry picked from commit 413775a)
@svc-nvidia-pr-review

Copy link
Copy Markdown

🤖 Starting review — findings will be posted here when done.

Comment thread src/uct/tcp/tcp_ep.c Resolved
setup_conns_to_entity(*m_ent, 1, fds);
post_send(fds.front(), buf);

wait_for_cond([this]() {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Minor: pls use the default timeout, 1 sec can be flaky on a loaded CI machine.

Suggested change
wait_for_cond([this]() {
wait_for_cond([this]() {
return ucs_list_is_empty(&m_tcp_iface->ep_list);
},
[this]() { progress(); });

@svc-nvidia-pr-review

Copy link
Copy Markdown

test/gtest/uct/tcp/test_tcp.cc

Coverage gap to note: the test only asserts that the EP is destroyed; it does not exercise the overflow scenario itself (a peer streaming more than 2 * rx_seg_size after the bogus header), nor does it check the client-side reset via the existing detect_conn_reset() helper. The regression is still caught, so I would not block on it.

Note: I could not read the existing PR discussion or CI state with the tools available here, so please de-duplicate these two comments against any unresolved threads (and +1 instead of reposting) before submitting.

@roiedanino
roiedanino enabled auto-merge (squash) October 20, 2026 11:59
@roiedanino
roiedanino merged commit 6c72eb3 into openucx:v1.23.x Oct 20, 2026
162 checks passed
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