Skip to content

tsh: fix panic when loading profile#38197

Merged
zmb3 merged 1 commit intomasterfrom
zmb3/tsh-panic-fix
Feb 15, 2024
Merged

tsh: fix panic when loading profile#38197
zmb3 merged 1 commit intomasterfrom
zmb3/tsh-panic-fix

Conversation

@zmb3
Copy link
Copy Markdown
Collaborator

@zmb3 zmb3 commented Feb 14, 2024

When unmarshaling the profile YAML, it's possible for the unmarshal to complete without error while not populating the profile struct, which leads to a panic.

Fix the nil pointer dereference by unmarshaling into a Profile instead of a *Profile, and further improve the error message when this happens.

Fixes #38188

Changelog: fix a potential panic in the tsh status command.

Comment thread api/profile/profile.go Outdated
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Open to suggestions for better ways to detect this.

We could check if bytes is empty, if p is the zero value, or look at some field other than name.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I bet there's plenty of things in the profile yaml that could make the profile unusable if wrong; do we want to catch them all, or is it just an advisory check here?

If it's the latter I think that checking the name should be enough.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

It is the latter. 👍

Comment thread api/profile/profile.go Outdated
When unmarshaling the profile YAML, it's possible for the unmarshal
to complete without error while not populating the profile struct,
which leads to a panic.

Fix the nil pointer dereference by unmarshaling into a Profile
instead of a *Profile, and further improve the error message
when this happens.

Fixes #38188
@zmb3 zmb3 force-pushed the zmb3/tsh-panic-fix branch from ac52c4d to 53fca97 Compare February 14, 2024 18:17
@zmb3 zmb3 added this pull request to the merge queue Feb 15, 2024
Merged via the queue into master with commit 80b0912 Feb 15, 2024
@zmb3 zmb3 deleted the zmb3/tsh-panic-fix branch February 15, 2024 21:12
@public-teleport-github-review-bot
Copy link
Copy Markdown

@zmb3 See the table below for backport results.

Branch Result
branch/v13 Create PR
branch/v14 Create PR
branch/v15 Create PR

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

tsh status panic

3 participants