Skip to content

servers: fix to reverse SA_RESTART option for sigaction() on modern codepath - #22037

Closed
vszakats wants to merge 6 commits into
curl:masterfrom
vszakats:tsigintr
Closed

servers: fix to reverse SA_RESTART option for sigaction() on modern codepath#22037
vszakats wants to merge 6 commits into
curl:masterfrom
vszakats:tsigintr

Conversation

@vszakats

@vszakats vszakats commented Jun 16, 2026

Copy link
Copy Markdown
Member

Historically servers used the deprecated siginterrupt() function to
configure restart behavior on specific signals. It accepts a flag, where
1 means to remove the SA_RESTART option, and 0 means to enable it.

In year 2021 3fb6e5a introduced the
modern alternative to the codebase, replacing siginterrupt() with
sigaction(). After this patch, supporting, modern, systems reacted on
the same flag, but, by accident, set the SA_RESTART bit when flag is
1, and did not set it when 0. This reversed the previous behavior, and
the one still used on the siginterrupt() legacy codepath.

Fix it by reversing the SA_RESTART logic for the sigaction()
codepath, syncing it with the pre-existing behavior.

I find it odd this did not cause any perceivable issue for 5 years, even
though it's the active one in most Unix envs.

Spotted by GitHub Code Quality, though suggesting to fix
siginterrupt() calls. But looking into the history, those were correct
all along.

Refs:
https://pubs.opengroup.org/onlinepubs/9699919799/functions/siginterrupt.html
https://pubs.opengroup.org/onlinepubs/9699919799/functions/sigaction.html
https://www.man7.org/linux/man-pages/man3/siginterrupt.3.html
https://www.man7.org/linux/man-pages/man2/sigaction.2.html

Follow-up to 3fb6e5a #6529


@vszakats
vszakats marked this pull request as draft June 16, 2026 01:08
@github-actions github-actions Bot added the tests label Jun 16, 2026
@testclutch

This comment was marked as outdated.

@vszakats vszakats changed the title servers: try reversing SA_RESTART option [WIP] servers: try reversing SA_RESTART option, Windows handler func experiments Jun 16, 2026
@vszakats vszakats changed the title [WIP] servers: try reversing SA_RESTART option, Windows handler func experiments [WIP] servers: try reversing SA_RESTART option Jun 18, 2026
@vszakats vszakats changed the title [WIP] servers: try reversing SA_RESTART option [WIP] servers: try reversing SA_RESTART option Jun 18, 2026
@vszakats vszakats changed the title [WIP] servers: try reversing SA_RESTART option servers: try reversing SA_RESTART option Jun 18, 2026
@vszakats vszakats changed the title servers: try reversing SA_RESTART option servers: reverse SA_RESTART option on the modern, sigaction() codepath Jun 18, 2026
@vszakats
vszakats marked this pull request as ready for review June 18, 2026 13:13
@vszakats
vszakats requested a review from Copilot June 18, 2026 13:16
@vszakats vszakats changed the title servers: reverse SA_RESTART option on the modern, sigaction() codepath servers: reverse SA_RESTART option for sigaction() on modern codepath Jun 18, 2026
@vszakats vszakats changed the title servers: reverse SA_RESTART option for sigaction() on modern codepath servers: reverse SA_RESTART option for sigaction(), on modern codepath Jun 18, 2026

Copilot AI left a comment

Copy link
Copy Markdown

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 long-standing behavioral mismatch in the curl test server signal handling: the modern sigaction() path had SA_RESTART behavior reversed relative to the historical siginterrupt() semantics, and this change restores the original meaning of the flag.

Changes:

  • Reverse SA_RESTART handling in set_signal() when using sigaction() so it matches the legacy siginterrupt() behavior.

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

Comment thread tests/server/util.c Outdated
@vszakats
vszakats marked this pull request as draft June 19, 2026 08:15
@vszakats

vszakats commented Aug 4, 2026

Copy link
Copy Markdown
Member Author

I plan to merge this as-is, soon, then try yanking the signal handling in
a follow-up PR. (then, if successful, remove the unused configure bits)

@vszakats
vszakats marked this pull request as ready for review August 4, 2026 14:24
@vszakats
vszakats requested a lite review from Copilot August 4, 2026 14:33
@vszakats

vszakats commented Aug 4, 2026

Copy link
Copy Markdown
Member Author

augment review

@vszakats

vszakats commented Aug 4, 2026

Copy link
Copy Markdown
Member Author

@aisle-analyzer

This reverts commit 5fb79fb56786daa848cf3825a16f053ae17c0d78.
This reverts commit a58c282a15f4238e91425592c09fa6f79e1e292a.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

@vszakats vszakats changed the title servers: reverse SA_RESTART option for sigaction(), on modern codepath servers: fix to reverse SA_RESTART option for sigaction() on modern codepath Aug 4, 2026
augmentcode[bot]

This comment was marked as outdated.

@curl curl deleted a comment from augmentcode Bot Aug 4, 2026
@vszakats
vszakats requested a lite review from Copilot August 4, 2026 15:25
@vszakats

vszakats commented Aug 4, 2026

Copy link
Copy Markdown
Member Author

augment review
@aisle-analyzer

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

@augmentcode

augmentcode Bot commented Aug 4, 2026

Copy link
Copy Markdown
🤖 Augment PR Summary

Summary: This PR fixes the inverted restart behavior for server signal handlers on the modern sigaction() codepath.

Changes:

  • Adjusts SA_RESTART flag handling so the runtime behavior matches historical siginterrupt() semantics.
  • Renames the set_signal() flag to norestart and switches it to an int to reflect the underlying APIs.
  • Updates all call sites to pass explicit 0/1 values for restart vs. no-restart behavior.

Technical Notes: The intent is to keep sigaction() and legacy siginterrupt() paths consistent (1 disables restart, 0 enables it).

🤖 Was this summary useful? React with 👍 or 👎

@augmentcode augmentcode Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review completed. No suggestions at this time.

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

@vszakats

vszakats commented Aug 5, 2026

Copy link
Copy Markdown
Member Author

Well, this may not be the last word said about signals and restart behavior.

Just spotted this commit:
9d0d828

Which sets SIGARLM, to the reverse of how it's set later on. Quite confusing.
I'm thinking maybe deleting the legacy interface would be a step to make this
easier to digest.

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

Labels

Development

Successfully merging this pull request may close these issues.

3 participants