Skip to content

Conversation

@marc-gr
Copy link
Contributor

@marc-gr marc-gr commented Sep 6, 2024

Proposed commit message

Add check on winlog metrics to prevent panic.

Metrics initialization was deferred to avoid registering them on CheckConfig calls. Now that is inside Open we need to check to avoid panicking when retrying on recoverable errors.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

No changelog entry since the change that introduced this is still not released.

@marc-gr marc-gr requested a review from a team as a code owner September 6, 2024 11:23
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Sep 6, 2024
@marc-gr marc-gr added backport-skip Skip notification from the automated backport with mergify Team:Security-Windows Platform Windows Platform Team in Security Solution labels Sep 6, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/sec-windows-platform (Team:Security-Windows Platform)

@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Sep 6, 2024
@mergify mergify bot assigned marc-gr Sep 6, 2024
l.metrics = newInputMetrics(l.channelName, l.id)
if l.metrics == nil {
l.metrics = newInputMetrics(l.channelName, l.id)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

does it mean that l.Open is called twice without l.Close? If so, there seems to be more issues

https://learn.microsoft.com/en-us/windows/win32/api/winevt/nf-winevt-evtsubscribe#return-value

so perhaps

-return l.openChannel(bookmark)
+ if l.subscription {
+    win.Close(l.subscription)
+}
+return l.openChannel(bookmark)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It only gets called more than once in case it fails to open, so no need to call close in that case.

@marc-gr marc-gr merged commit 8a56ef8 into elastic:main Sep 6, 2024
@marc-gr marc-gr deleted the fix/init-metrics branch September 6, 2024 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport-skip Skip notification from the automated backport with mergify Team:Security-Windows Platform Windows Platform Team in Security Solution

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants