Skip to content

Fix SOCK_CLOEXEC handling to maintain socktype equality checks#20808

Closed
bagder wants to merge 7 commits intomasterfrom
bagder/cloexec
Closed

Fix SOCK_CLOEXEC handling to maintain socktype equality checks#20808
bagder wants to merge 7 commits intomasterfrom
bagder/cloexec

Conversation

@bagder
Copy link
Copy Markdown
Member

@bagder bagder commented Mar 3, 2026

socket_open unconditionally ORs SOCK_CLOEXEC into addr->socktype. The same addr structure is later reused in cf_socket_open, which checks ctx->addr.socktype == SOCK_STREAM to decide whether to enable TCP_NODELAY and TCP keepalive. With SOCK_CLOEXEC set, the equality check fails, so TCP-specific options are skipped and ctx->sock_connected is also computed against SOCK_DGRAM with a mismatching value.

Follow-up to 0536769

Found by Codex Security

/cc @ibookstein

@nextsilicon-itay-bookstein
Copy link
Copy Markdown

Nice catch. Note that the proposed change will stop affecting the fopensocket != NULL case, which then makes the unconditional fcntl exemption wrong... Maybe it is better to mask out that flag in the equality comparisons when it is defined?

@bagder bagder marked this pull request as ready for review March 7, 2026 10:13
As the SOCK_CLOEXEC and SOCK_NONBLOCK get ORed to the socktype, this
introduces a SOCKTYPE() macro to use when checking for the specific
socket type: DGRAM or STREAM. The macro filters off the non-type related
bits to enable the comparison.

Follow-up to 0536769
@bagder
Copy link
Copy Markdown
Member Author

bagder commented Mar 7, 2026

@aisle-analyzer augment review

@aisle-research-bot
Copy link
Copy Markdown

aisle-research-bot bot commented Mar 7, 2026

🔒 Aisle Security Analysis

✅ We scanned this PR and did not find any security vulnerabilities.

Aisle supplements but does not replace security review.


Analyzed PR: #20808 at commit af2dea5

Last updated on: 2026-03-07T10:34:27Z

@augmentcode
Copy link
Copy Markdown

augmentcode bot commented Mar 7, 2026

🤖 Augment PR Summary

Summary: Prevents TCP-specific socket options from being skipped when addr->socktype has extra bits (e.g., SOCK_CLOEXEC) ORed in.
Changes: Introduces a SOCKTYPE() masking macro and uses it for SOCK_STREAM / SOCK_DGRAM comparisons in cf_socket_open().

🤖 Was this summary useful? React with 👍 or 👎

Copy link
Copy Markdown

@augmentcode augmentcode bot left a comment

Choose a reason for hiding this comment

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

Review completed. 1 suggestion posted.

Fix All in Augment

Comment augment review to trigger a new review at any time.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a regression in cf_socket_open() where SOCK_CLOEXEC (and potentially SOCK_NONBLOCK) being ORed into addr->socktype caused direct equality checks against SOCK_STREAM/SOCK_DGRAM to fail, skipping TCP-specific socket options and miscomputing connection state.

Changes:

  • Introduce a SOCKTYPE() helper macro to strip SOCK_CLOEXEC/SOCK_NONBLOCK bits before comparing socket types.
  • Use SOCKTYPE(ctx->addr.socktype) for TCP detection (SOCK_STREAM) and for sock_connected determination (SOCK_DGRAM).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

@bagder bagder closed this in 08d6497 Mar 7, 2026
@bagder bagder deleted the bagder/cloexec branch March 7, 2026 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

5 participants