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

syslogformat: fix underflow if no bytes left to parse #3329

Closed

Conversation

aleksandrgilfanov
Copy link

Issue: #3328

@kira-syslogng
Copy link
Contributor

This user does not have permission to start the build. Can one of the admins verify this patch and start the build?
(admin: you have the next options (make sure you checked the code):
"ok to test" to accept this pull request (and further changes) for testing
"test this please" for a one time test run
"add to whitelist" add author of a Pull Request to whitelist (globally, be careful, it means this user can trigger kira for any PR)
do nothing -> CI won't start)

2 similar comments
@kira-syslogng
Copy link
Contributor

This user does not have permission to start the build. Can one of the admins verify this patch and start the build?
(admin: you have the next options (make sure you checked the code):
"ok to test" to accept this pull request (and further changes) for testing
"test this please" for a one time test run
"add to whitelist" add author of a Pull Request to whitelist (globally, be careful, it means this user can trigger kira for any PR)
do nothing -> CI won't start)

@kira-syslogng
Copy link
Contributor

This user does not have permission to start the build. Can one of the admins verify this patch and start the build?
(admin: you have the next options (make sure you checked the code):
"ok to test" to accept this pull request (and further changes) for testing
"test this please" for a one time test run
"add to whitelist" add author of a Pull Request to whitelist (globally, be careful, it means this user can trigger kira for any PR)
do nothing -> CI won't start)

@alltilla
Copy link
Collaborator

@kira-syslogng ok to test

@kira-syslogng
Copy link
Contributor

Build FAILURE

@Kokan
Copy link
Collaborator

Kokan commented Jun 20, 2020

Thanks for the PR, as I stated in the #3328 issue imho this is far from complete.
@bazsi had a suggestion[1] in the issue, based on his reasoning seq id is originally just a heuristics I agree with him. The code should be change accordingly.

Additionally it would be great to see a unit test covering this case, as it could both make sure this is not broken again and the once agreed behaviour is documented via the test.

The patch also had additional crash prevention after processing log_msg_parse_cisco_timestamp_attributes and log_msg_parse_skip_chars, that part of the patch was also just to prevent the crash. But the fix should consider what step should be done by the parser (see options in the original issue).

Additionally this patch seems to introduce an actual bug caught by test. By default syslog-ng does not process empty lines in a file, but there is an option to do so flags(empty-lines). If you set up a file source with this option and send an empty message the generated message contains some uninitialised memory.

@version: 3.27

log {
  source { file("/tmp/input" flags(empty-lines)); };
  destination { file("/tmp/output"); };
};

Write an empty line into the /tmp/input and output file should have the following:

Jun 20 21:34:41 work syslog-ng[109532]: Error processing log message:

Instead the current version prints:

Jun 20 21:34:41 work syslog-ng[109532]: Error processing log message:
�����������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������
�����������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������
�����������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������
�����������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������
�����������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������
�����������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������
�����������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������
�����������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������
�����������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������
�����������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������
����������������������������������������������������������������������������������������������������������

@aleksandrgilfanov Do you have the capacity to push forward this PR based on my comment ?

[1] #3328 (comment)

@Kokan
Copy link
Collaborator

Kokan commented Jul 24, 2020

Closing in favor of #3364.

@Kokan Kokan closed this Jul 24, 2020
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.

None yet

4 participants