Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix tmux crashing bug #6766

Merged
merged 4 commits into from
Nov 16, 2023
Merged

fix tmux crashing bug #6766

merged 4 commits into from
Nov 16, 2023

Conversation

tobiasb-ms
Copy link
Contributor

@tobiasb-ms tobiasb-ms commented Nov 15, 2023 •
edited
Loading

Merge Checklist

All boxes should be checked before merging the PR (just tick any boxes which don't apply to this PR)

  • The toolchain has been rebuilt successfully (or no changes were made to it)
  • The toolchain/worker package manifests are up-to-date
  • Any updated packages successfully build (or no packages were changed)
  • Packages depending on static components modified in this PR (Golang, *-static subpackages, etc.) have had their Release tag incremented.
  • Package tests (%check section) have been verified with RUN_CHECK=y for existing SPEC files, or added to new SPEC files
  • All package sources are available
  • cgmanifest files are up-to-date and sorted (./cgmanifest.json, ./toolkit/scripts/toolchain/cgmanifest.json, .github/workflows/cgmanifest.json)
  • LICENSE-MAP files are up-to-date (./SPECS/LICENSES-AND-NOTICES/data/licenses.json, ./SPECS/LICENSES-AND-NOTICES/LICENSES-MAP.md, ./SPECS/LICENSES-AND-NOTICES/LICENSE-EXCEPTIONS.PHOTON)
  • All source files have up-to-date hashes in the *.signatures.json files
  • sudo make go-tidy-all and sudo make go-test-coverage pass
  • Documentation has been updated to match any changes to the build system
  • Ready to merge
Summary

Fixes issue #6598 which is a crash on selection in tmux. The fix requires an update to ncurses and a patch to tmux. From the patch comments:

ncurses-6.4-20230408 change tparm to require cur_term, which broke tmux usage of it.

ncurses-6.4-20230423 then added tiparm_s that allows usage without cur_term.

tmux change https://github.com/tmux/tmux/commit/39d41d0810d4e8ae6ce8d27776dfbb96722d9319 uses tiparm_s if it exists, but cannot be cleanly applied to tmux tag 3.2a.

That change uses a config setting to created #defines to determine which version of tparm it should use, and only conditionally uses tiparm_s, because it needs to be backwards compatible with previous versions of ncurses.

But to use that, we would need to get the actual source as it appears in github, rather than the released version (they are different downloads: see https://github.com/tmux/tmux/releases).

Fortunately, we have the luxery of forcing tmux to use a version of ncurses that has the function we want (see above).

Given all this, this patch takes the change to use tiparm_s, removes the conditional compilation portion so it always uses tiparm_s and applies it to the code as it exists in 3.2a.

It has both a build-time and run-time dependency on ncurses-6.4-20230423 or later.
Change Log
  • Update ncurses to a version that includes a new version of the tiparm API that tmux uses to fix the crash.
  • Patch tmux to use new API.
Does this affect the toolchain?

YES

Associated issues
Test Methodology

@tobiasb-ms tobiasb-ms requested a review from a team as a code owner November 15, 2023 16:12
@microsoft-github-policy-service microsoft-github-policy-service bot added the main PR Destined for main label Nov 15, 2023
@tobiasb-ms tobiasb-ms force-pushed the tobiasb/fix-tmux-crash-by-updating-ncurses branch from 649a705 to d9196bb Compare November 15, 2023 18:32
neha170
neha170 approved these changes Nov 16, 2023
View reviewed changes
Copy link
Contributor

@neha170 neha170 left a comment

Choose a reason for hiding this comment

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

LGTM. Did we try rebuilding the toolchain with the updated ncurses package?

@tobiasb-ms
Copy link
Contributor Author

LGTM. Did we try rebuilding the toolchain with the updated ncurses package?

Yes. The build I linked to did full toolchain, package and image builds for AMD and ARM.

hbeberman
hbeberman reviewed Nov 16, 2023
View reviewed changes
SPECS/tmux/tmux.spec Outdated Show resolved Hide resolved
tobiasb-ms and others added 2 commits November 16, 2023 11:22
@tobiasb-ms @hbeberman
Update SPECS/tmux/tmux.spec
d8b984b 
Co-authored-by: Henry Beberman <henry.beberman@microsoft.com>
@tobiasb-ms
fix spelling error
a1f78f2
@tobiasb-ms tobiasb-ms merged commit 9dc4183 into main Nov 16, 2023
10 checks passed
@tobiasb-ms tobiasb-ms deleted the tobiasb/fix-tmux-crash-by-updating-ncurses branch November 16, 2023 19:50
@adamiBs adamiBs mentioned this pull request Nov 19, 2023
Closed
gmileka pushed a commit that referenced this pull request Dec 7, 2023
@tobiasb-ms @gmileka
fix tmux crashing bug (#6766)
9716d45 
Co-authored-by: Henry Beberman <henry.beberman@microsoft.com>

Fixes issue #6598 which is a crash on selection in tmux. The fix requires an update to ncurses and a patch to tmux. From the patch comments:

```
ncurses-6.4-20230408 change tparm to require cur_term, which broke tmux usage of it.

ncurses-6.4-20230423 then added tiparm_s that allows usage without cur_term.

tmux change tmux/tmux@39d41d0 uses tiparm_s if it exists, but cannot be cleanly applied to tmux tag 3.2a.

That change uses a config setting to created #defines to determine which version of tparm it should use, and only conditionally uses tiparm_s, because it needs to be backwards compatible with previous versions of ncurses.

But to use that, we would need to get the actual source as it appears in github, rather than the released version (they are different downloads: see https://github.com/tmux/tmux/releases).

Fortunately, we have the luxery of forcing tmux to use a version of ncurses that has the function we want (see above).

Given all this, this patch takes the change to use tiparm_s, removes the conditional compilation portion so it always uses tiparm_s and applies it to the code as it exists in 3.2a.

It has both a build-time and run-time dependency on ncurses-6.4-20230423 or later.
```
freebsd-git pushed a commit to freebsd/freebsd-ports that referenced this pull request Jun 14, 2024
@metalefty
sysutils/tmux: Fix crash when text selection using mouse
5a77b3f 
I re-ported a patch originally ported to 3.2a by Tobias Brick, a
Microsoft employee to 3.3a. The patch depends on ncurses 6.4-20230423 or
later so I bind this port to ncurses:port.

See commit message of the patch for detail [1]

[1] https://github.com/microsoft/azurelinux/blob/a1f78f2/SPECS/tmux/manual-patch-to-fix-crash-due-to-change-to-ncurses.patch

PR:		279276
Approved by:	maintainer timeout
Obtained from:	microsoft/azurelinux#6598
Obtained from:	microsoft/azurelinux#6766
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@neha170 neha170 neha170 approved these changes

@hbeberman hbeberman hbeberman left review comments

@rlmenge rlmenge rlmenge approved these changes

Assignees
No one assigned
Labels
main PR Destined for main
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tobiasb-ms @rlmenge @hbeberman @neha170