-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 off-by one for octet-counting code in in_sysylog.rb #2771
Conversation
break | ||
end | ||
pos = idx + msg.size |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] msg.size
is num
break | ||
end | ||
pos = idx + msg.size | ||
# syslog might append a \n as an extra frame delimiter | ||
msg.chomp! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How can we handle if the user adds \n
at the end of msg intentionally?
In the situation, it shouldn't trim \n
from msg.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was limiting this change to preventing the removal of characters other than \n
. I could add that as a configuration option to control if the chomp should be called.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked into this more and currently in the default parser configuration chomping at the in_syslog level has no impact because parse_syslog strips off trailing \n
.
However, that is only part of the story. Until #2767 lands if RFC5424 is selected as the format for parser_syslog messages it will fail to parse unless in_syslog does a chomp on the incoming messages.
There also is a second issue, without this change octet counted transport always strips off the last character of a message (incorrectly presumed to be a delimiter) while the delimited transport does not strip off the delimiter! So if chomp is off by default then it will be a behavior change for octect counted transport. If chomp is on by default then it will be a behavior change for delimited transport. In light of that I would personally would keep chomp off by default as it because it will impact fewer users and this change is supposed to address that fact that octect counted transport is currently incorrect.
d = create_driver([CONFIG, "<transport tcp> \n</transport>", 'frame_type octet_count'].join("\n")) | ||
tests = create_test_case | ||
|
||
d.run(expect_emits: 2) do | ||
d.run(expect_emits: 1) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason you changed this line? I think it shouldn't be changed since two messages are sent.
Sorry for the delay! I left some comments. BTW, Could you follow DCO? https://github.com/fluent/fluentd/pull/2771/checks?check_run_id=384067806 |
@onet-git friendly ping |
done in #2942. thanks! |
Which issue(s) this PR fixes:
Fixes #
What this PR does / why we need it:
There is an off-by-one counting issue in octet-transported syslog messages. In most case syslog transport clients will append a
\n
to the end of each line, hiding this bug. The bug gets triggered whenever a syslog client doesn't append a\n
.Docs Changes:
None
Release Note:
Support octet-counted syslog transport when the last character in each message is not a \n.