Skip to content

Fix a panic in socket handlers #4678

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.

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

Merged
merged 2 commits into from
Mar 28, 2022
Merged

Fix a panic in socket handlers #4678

merged 2 commits into from
Mar 28, 2022

Conversation

echlebek
Copy link
Contributor

What is this change?

This changelog fixes a panic in sensu-backend when a socket handler gets an error mid-write. It also fixes another bug where socket handlers will stop respecting their timeout after the initial connection is established.

Why is this change necessary?

Closes #4675

Does your change need a Changelog entry?

Yes

Were there any complications while making this change?

It was tricky to reproduce the bug with a test but I eventually succeeded.

Have you reviewed and updated the documentation for this change? Is new documentation required?

Docs changes not required.

How did you verify this change?

Fully verified with automated testing.

Is this change a patch?

Yes.

Unfortunately adding this failing unit test was not overly simple. I had
to fix a bug where timeout was not respected by Write(). I also
refactored the unit tests because they all bind to the same port. They
now bind to pseudo-random ports.

Signed-off-by: Eric Chlebek <eric@sensu.io>
This commit fixes a case in socket handlers that can cause sensu-backend
to crash with a nil pointer dereference. The condition occurs when
handlers can connect to a socket, but then get an error while writing.

Signed-off-by: Eric Chlebek <eric@sensu.io>
@echlebek echlebek requested a review from c-kruse March 25, 2022 23:17
@echlebek echlebek self-assigned this Mar 25, 2022
Copy link
Contributor

@c-kruse c-kruse left a comment

Choose a reason for hiding this comment

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

Clever way to get a failing test to reproduce the issue!

LGTM!

@echlebek echlebek merged commit dd38734 into release/6.6 Mar 28, 2022
@echlebek echlebek deleted the bugfix/handler-panic branch March 28, 2022 17:07
agm650 added a commit to agm650/sensu-go that referenced this pull request May 15, 2022
…ource-plural

* 'resource-plural' of github.com:agm650/sensu-go: (26 commits)
  Do not use SetNormalizeFunc
  Updating changelog
  Correcting verbs & resources flags
  Remove support for embedded etcd (sensu#4704)
  Fix types module breakage (sensu#4698)
  Fix CHANGELOG-6 and CHANGELOG-7
  Update core/v3 and types modules
  Update go modules
  add CHANGELOG-7.md, update .gitattributes
  rename CHANGELOG.md in .gitattributes
  rename CHANGELOG.md -> CHANGELOG-6.md
  go mod tidy for types mod
  Fix types module (run go mod tidy)
  Subdue logic (sensu#4680)
  Fix types module (run go mod tidy)
  Update etcd modules to v3.5.2 (sensu#4683)
  Run tests for all modules (sensu#4682)
  Remove json-iterator from the project (sensu#4681)
  [backend/api] Allow consumer to test if authorized to perform action (sensu#4674)
  Fix a panic in socket handlers (sensu#4678)
  ...
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