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 loop while reading from standalone evtx #30006

Merged
merged 4 commits into from
Feb 3, 2022

Conversation

grishinpv
Copy link
Contributor

@grishinpv grishinpv commented Jan 25, 2022

What does this PR do?

Tjis fix adds checking EOF to prevent constant loop while reading standalone evtx file

Why is it important?

When we reach the end of the file (case io.EOF) we set stop = true.
But next we continue the loop regardless stop value and read whole file again and again.

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.

When we reach the end of the file (case io.EOF) we set stop = true.
But next we continue look regardless stop value and read whole file again and again
Fix loop while reading from standalone evtx
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Jan 25, 2022
@mergify
Copy link
Contributor

mergify bot commented Jan 25, 2022

This pull request does not have a backport label. Could you fix it @grishinpv? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-v./d./d./d is the label to automatically backport to the 7./d branch. /d is the digit

NOTE: backport-skip has been added to this pull request.

@mergify mergify bot added the backport-skip Skip notification from the automated backport with mergify label Jan 25, 2022
@elasticmachine
Copy link
Collaborator

elasticmachine commented Jan 25, 2022

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Reason: null

  • Start Time: 2022-02-03T15:56:11.205+0000

  • Duration: 68 min 43 sec

  • Commit: a3fa9bc

Test stats 🧪

Test Results
Failed 0
Passed 1305
Skipped 26
Total 1331

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@elasticmachine
Copy link
Collaborator

Pinging @elastic/siem (Team:SIEM)

@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Jan 26, 2022
@andrewkroh
Copy link
Member

andrewkroh commented Jan 26, 2022

Have you followed the instructions in https://www.elastic.co/guide/en/beats/winlogbeat/7.7/reading-from-evtx.html#reading-from-evtx? Specifically the part about no_more_events: stop. Does that not work? If that's not working then we have a bug.

@elasticmachine
Copy link
Collaborator

Pinging @elastic/security-external-integrations (Team:Security-External Integrations)

Copy link
Contributor

@taylor-swanson taylor-swanson left a comment

Choose a reason for hiding this comment

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

This appears to be the same issue as reported here: https://discuss.elastic.co/t/winlogbeat-wont-exit-winlogbeat-json-output-size-is-different-for-same-file/295706

I did test out the original fix and it does work, but as my other comment points out, there's an edge case that's missed when there are records returned, along with an io.EOF.

winlogbeat/beater/eventlogger.go Show resolved Hide resolved
@ashim-mahara
Copy link

Commenting to track the bug. I was the one who posted on the forum earlier. Let me know if you need any help, I have moderate experience working with Golang.

@taylor-swanson taylor-swanson added backport-7.17 Automated backport to the 7.17 branch with mergify backport-v8.0.0 Automated backport with mergify bug and removed backport-skip Skip notification from the automated backport with mergify labels Jan 31, 2022
@taylor-swanson
Copy link
Contributor

Hi @grishinpv, do you have an update on the status of your change? If you would like, I can push the necessary changes and get this PR merged. If you do push a change, we'll also need an update to CHANGELOG.next.asciidoc, something like:

--- a/CHANGELOG.next.asciidoc
+++ b/CHANGELOG.next.asciidoc
@@ -84,6 +84,7 @@
 - Improve ECS field mappings in Sysmon module. `rule.name` is populated for all events when present. {issue}18364[18364]
 - Remove top level `hash` property from sysmon events {pull}20653[20653]
 - Move module processing from local Javascript processor to ingest node {issue}29184[29184] {pull}29435[29435]
+- Fix run loop when reading from evtx file {pull}30006[30006]
 
 *Functionbeat*

If I don't hear back in the next day or so, I'll go ahead and push the changes so we can get this fix in.

Copy link
Contributor

@leehinman leehinman left a comment

Choose a reason for hiding this comment

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

LGTM

@taylor-swanson taylor-swanson self-requested a review February 3, 2022 15:54
@leehinman
Copy link
Contributor

/test

@andrewkroh andrewkroh added the backport-v8.1.0 Automated backport with mergify label Feb 3, 2022
@taylor-swanson taylor-swanson merged commit 3c6724a into elastic:main Feb 3, 2022
mergify bot pushed a commit that referenced this pull request Feb 3, 2022
- Fix loop while reading from standalone evtx file
- Moved stop variable and check to outer loop

Co-authored-by: Taylor Swanson <[email protected]>
(cherry picked from commit 3c6724a)
mergify bot pushed a commit that referenced this pull request Feb 3, 2022
- Fix loop while reading from standalone evtx file
- Moved stop variable and check to outer loop

Co-authored-by: Taylor Swanson <[email protected]>
(cherry picked from commit 3c6724a)
mergify bot pushed a commit that referenced this pull request Feb 3, 2022
- Fix loop while reading from standalone evtx file
- Moved stop variable and check to outer loop

Co-authored-by: Taylor Swanson <[email protected]>
(cherry picked from commit 3c6724a)
taylor-swanson pushed a commit that referenced this pull request Feb 3, 2022
- Fix loop while reading from standalone evtx file
- Moved stop variable and check to outer loop

Co-authored-by: Taylor Swanson <[email protected]>
(cherry picked from commit 3c6724a)

Co-authored-by: Grishin Pavel <[email protected]>
taylor-swanson pushed a commit that referenced this pull request Feb 3, 2022
- Fix loop while reading from standalone evtx file
- Moved stop variable and check to outer loop

Co-authored-by: Taylor Swanson <[email protected]>
(cherry picked from commit 3c6724a)

Co-authored-by: Grishin Pavel <[email protected]>
taylor-swanson pushed a commit that referenced this pull request Feb 3, 2022
…30188)

* Fix loop while reading from standalone evtx (#30006)

- Fix loop while reading from standalone evtx file
- Moved stop variable and check to outer loop

Co-authored-by: Taylor Swanson <[email protected]>
(cherry picked from commit 3c6724a)
@ashim-mahara
Copy link

Still not working on Winlogbeat 8.0.0 Windows x86_64. Behavior is as before as explained in https://discuss.elastic.co/t/winlogbeat-wont-exit-winlogbeat-json-output-size-is-different-for-same-file/295706.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-7.17 Automated backport to the 7.17 branch with mergify backport-v8.0.0 Automated backport with mergify backport-v8.1.0 Automated backport with mergify bug Winlogbeat
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants