Skip to content

fix truncated event log message #41327

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.

Sign up for GitHub

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

Jump to bottom
Merged
intxgo merged 12 commits into from
Oct 22, 2024
Merged

fix truncated event log message #41327

intxgo merged 12 commits into from
Oct 22, 2024

Conversation

@intxgo
Copy link
Contributor

@intxgo intxgo commented Oct 18, 2024 •
edited
Loading

Fixes

Proposed commit message

Fix windows event log ingest issues caused by event message truncation.

  1. Increased scratch buffer size from 8K wide characters to 256K wide characters. The scratch buffer is used for efficiency, one buffer per event source. It was set really low.
  2. Handling insufficient scratch buffer size condition. Windows API documentation is not very clear about it but the APIs return ERROR_INSUFFICIENT_BUFFER only when used with nil buffer, otherwise they happily succeed copying as much as the given buffer allows (truncating message). In such case we will retry with an ad-hoc pool buffer of sufficient size. In real world the initial 256K characters buffer should be large enough, so this is really a what-if fallback.

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.

Disruptive User Impact

Author's Checklist

  • [ ]

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

@intxgo intxgo added bugfix backport-8.15 Automated backport to the 8.15 branch with mergify backport-8.16 Automated backport with mergify labels Oct 18, 2024
@intxgo intxgo requested a review from a team as a code owner October 18, 2024 20:00
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Oct 18, 2024
@intxgo intxgo added the Team:Security-Windows Platform Windows Platform Team in Security Solution label Oct 18, 2024
@elasticmachine
Copy link
Contributor

elasticmachine commented Oct 18, 2024

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 Oct 18, 2024
@mergify
Copy link
Contributor

mergify bot commented Oct 18, 2024

backport-8.x has been added to help with the transition to the new branch 8.x.
If you don't need it please use backport-skip label and remove the backport-8.x label.

@mergify mergify bot assigned intxgo Oct 18, 2024
@mergify mergify bot added the backport-8.x Automated backport to the 8.x branch with mergify label Oct 18, 2024
@intxgo
Copy link
Contributor Author

intxgo commented Oct 18, 2024 

>> check: Checking source code for common problems
Error: some files are not up-to-date. Run 'make update' then review and commit the changes. Modified: [winlogbeat/docs/modules_list.asciidoc]
make[1]: *** [../libbeat/scripts/Makefile:153: check] Error 1
make[1]: Leaving directory '/home/runner/work/beats/beats/winlogbeat'
make: *** [Makefile:94: check] Error 1
Error: Process completed with exit code 2.

I'm not sure how can I do it on Windows, it seems to be Linux hint?

@intxgo intxgo marked this pull request as draft October 21, 2024 11:10
@intxgo intxgo marked this pull request as ready for review October 21, 2024 11:33
matthewscherer
matthewscherer approved these changes Oct 21, 2024
View reviewed changes
Copy link

@matthewscherer matthewscherer left a comment

Choose a reason for hiding this comment

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

Did you add any tests for this case as well?

winlogbeat/sys/wineventlog/wineventlog_windows.go

var bufferPtr *byte
if renderBuf != nil {
if len(renderBuf) > 0 {
Copy link

@matthewscherer matthewscherer Oct 21, 2024

Choose a reason for hiding this comment

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

👍

winlogbeat/sys/wineventlog/wineventlog_windows.go Outdated
// this behavior is bad for EvtFormatMessageKeyword as then the API returns a list of null terminated
// strings in the buffer (it's fine for now as we don't use this parameter value).
return common.UTF16ToUTF8Bytes(renderBuf, out)
// bufferUsed indicates the size used internally to render the message. When called with nil buffer
Copy link

@matthewscherer matthewscherer Oct 21, 2024

Choose a reason for hiding this comment

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

At some point it might be nice to profile this code and see how many time we actually see an insufficient buffer size passed to EvtFormatMessage(). I guess it's hard to know which messages our customers are using the most and which ones usually overrun this buffer.

Copy link
Contributor Author

@intxgo intxgo Oct 21, 2024

Choose a reason for hiding this comment

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

Yes, that's why I opted to increase the scratch buffer significantly to avoid double rendering. The size of the scratch buffer is still insignificant as it's only one buffer per event channel. Anyway unused memory from the buffer will just get paged out at runtime.

No I didn't add any test for the truncation as with having the fallback of allocating memory from the pool this should always succeed.

gabriellandau
gabriellandau reviewed Oct 21, 2024
View reviewed changes
gabriellandau
gabriellandau previously requested changes Oct 21, 2024
View reviewed changes
@gabriellandau gabriellandau dismissed their stale review October 21, 2024 19:19

No blockers

marc-gr
marc-gr reviewed Oct 22, 2024
View reviewed changes
@intxgo intxgo merged commit 99d11eb into elastic:main Oct 22, 2024
@intxgo intxgo deleted the fix-truncated-log branch October 22, 2024 14:01
mergify bot pushed a commit that referenced this pull request Oct 22, 2024
@intxgo @mergify
fix truncated event log message (#41327)
c0f842a 
* fix truncated event log

* changelog

* fix warning

* fix golint

* playing hide an catch with CI

* size in bytes

* review

* code review

* add comment, unify code path

* refactor code

(cherry picked from commit 99d11eb)
@mergify mergify bot mentioned this pull request Oct 22, 2024
Merged
6 tasks
mergify bot pushed a commit that referenced this pull request Oct 22, 2024
@intxgo @mergify
fix truncated event log message (#41327)
944cb2a 
* fix truncated event log

* changelog

* fix warning

* fix golint

* playing hide an catch with CI

* size in bytes

* review

* code review

* add comment, unify code path

* refactor code

(cherry picked from commit 99d11eb)
mergify bot pushed a commit that referenced this pull request Oct 22, 2024
@intxgo @mergify
fix truncated event log message (#41327)
e4cdf14 
* fix truncated event log

* changelog

* fix warning

* fix golint

* playing hide an catch with CI

* size in bytes

* review

* code review

* add comment, unify code path

* refactor code

(cherry picked from commit 99d11eb)
This was referenced Oct 22, 2024
Merged
Merged
intxgo added a commit that referenced this pull request Oct 23, 2024
@mergify @intxgo
fix truncated event log message (#41327) (#41368)
330535f 
* fix truncated event log

* changelog

* fix warning

* fix golint

* playing hide an catch with CI

* size in bytes

* review

* code review

* add comment, unify code path

* refactor code

(cherry picked from commit 99d11eb)

Co-authored-by: Leszek Kubik <[email protected]>
intxgo added a commit that referenced this pull request Oct 23, 2024
@mergify @intxgo
fix truncated event log message (#41327) (#41367)
a64621c 
* fix truncated event log

* changelog

* fix warning

* fix golint

* playing hide an catch with CI

* size in bytes

* review

* code review

* add comment, unify code path

* refactor code

(cherry picked from commit 99d11eb)

Co-authored-by: Leszek Kubik <[email protected]>
intxgo added a commit that referenced this pull request Oct 23, 2024
@mergify @intxgo
fix truncated event log message (#41327) (#41366)
fd2299e 
* fix truncated event log

* changelog

* fix warning

* fix golint

* playing hide an catch with CI

* size in bytes

* review

* code review

* add comment, unify code path

* refactor code

(cherry picked from commit 99d11eb)

Co-authored-by: Leszek Kubik <[email protected]>
@khushijain21 khushijain21 mentioned this pull request Jun 23, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gabriellandau gabriellandau gabriellandau left review comments

@marc-gr marc-gr marc-gr approved these changes

@matthewscherer matthewscherer matthewscherer approved these changes

Assignees

@intxgo intxgo

Labels

backport-8.x Automated backport to the 8.x branch with mergify backport-8.15 Automated backport to the 8.15 branch with mergify backport-8.16 Automated backport with mergify bugfix Team:Security-Windows Platform Windows Platform Team in Security Solution

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@intxgo @elasticmachine @marc-gr @gabriellandau @matthewscherer