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.

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

Fix crash and deadlocks in the support for recursive logging #7127

Merged

Conversation

Smjert
Copy link
Member

@Smjert Smjert commented May 28, 2021

The code has been slightly refactored to remove an incorrect usage
of std::futures in a queue, where it was possible to try to wait on the
same std::future twice, which would lead to a crash.

Even after fixing the crash, it was possible that threads dealing
with recursive logging would change order and wait on a std::future
of another thread, causing a deadlock.

Finally the deferred log relaying wasn't always used when it should've,
due to a confusion between boolean values.
A enum class has been used instead to be more expressive about
the relaying mode asked.

Fixes #6928

The code has been slightly refactored to remove an incorrect usage
of std::futures in a queue, where it was possible to try to wait on the
same std::future twice, which would lead to a crash.

Even after fixing the crash, it was possible that threads dealing
with recursive logging would change order and wait on a std::future
of another thread, causing a deadlock.

Finally the deferred log relaying wasn't always used when it should've,
due to a confusion between boolean values.
A enum class has been used instead to be more expressive about
the relaying mode asked.
@Smjert Smjert requested review from a team as code owners May 28, 2021 12:39
@theopolis
Copy link
Member

Is there a potential for a future to exist while osquery is shutting down?

@Smjert
Copy link
Member Author

Smjert commented May 29, 2021

Is there a potential for a future to exist while osquery is shutting down?

I'm not sure I understand your question; meaning, the optional future is not empty as long as there's "a thread" (but a very specific one, closely related to the one that waits) that needs to do deferred/async logging.

So it may be set when osquery is asked to shutdown and a thread is waiting on it, but I expect the deferred logging thread to return/yield as usual, so the future can immediately stop waiting.

@theopolis theopolis merged commit 3541ef9 into osquery:master May 30, 2021
@Smjert Smjert deleted the stefano/fix/logger-deferred-logging branch June 8, 2021 12:29
aikuchin pushed a commit to aikuchin/osquery that referenced this pull request Jul 11, 2023
…0 to master

* commit '367b03dd1baeb99506de13a897eff0456c287791': (46 commits)
  4.9.0 Changelog (osquery#7152)
  packaging: update rendered chocolatey spec icon URL (osquery#7148)
  Add additional paths to `apps` and `launchd` (osquery#7154)
  custom curl_certificate timeouts would never be used (osquery#7151)
  Add current WMI location for dell bios info (osquery#7103)
  enable other stats on containers that don't have traditional networks (osquery#7145)
  Add Prefetch table (osquery#7076)
  Add detection/handling for updated XProtect path in macOS Big Sur (osquery#7138)
  Trigger event cleanup checks every 256 events (osquery#7143)
  pipe_channel not reading all data in a message (osquery#7139)
  libs: Update libyara to version 4.1.1 (osquery#7133)
  libs: Update librdkafka to version 1.7.0 (osquery#7134)
  Update website generators (osquery#7136)
  7118: Make generaing an extension uuid thread safe (osquery#7135)
  Alternate check for packageIdentifiers key (osquery#7099)
  Website: Note windows support for yara (osquery#7130)
  Fix crash and deadlocks in the support for recursive logging (osquery#7127)
  Implement infinite enrollment retries with tls_enrollment_max_attempts=0 (osquery#7125)
  Remove POSIX-only -fexceptions on Windows (osquery#7126)
  Minor cleanup of unused variables (osquery#7128)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

flaky test: osquery_logger_tests-test segfaults
2 participants