syslog-format: cannot parse ipv6 into hostname #1617
Conversation
|
Build FAILURE, the tests were executed on test branch: master and test suite: functions |
There was a problem hiding this comment.
hmm.. I would be very reluctant to change the rfc3164 parser. the colon is there to make sure that if there's no hostname, just the program name, we don't overflow into into the MSG part of the message.
PID is not always present, so '[' is not a good indicator either.
|
Build FAILURE, the tests were executed on test branch: master and test suite: functions |
|
Not nice but maybe works: We could use inet_pton to check if we see a valid ipv6 address. If yes, we set the hostname. If not, we go with the original heuristic. Of course we need to prepare the candidate string for inet pton, so first I need to create nullterminated substring until the first space or '[' character, which then can be passed to inet_pton. |
|
Or this is maybe better: g_hostname_is_ip_address (), from glib 2.22 |
|
never forget the performance implications, this is a very hot path here.
…--
Bazsi
On Mon, Jul 24, 2017 at 3:14 PM, furiel ***@***.***> wrote:
Or this is maybe better: g_hostname_is_ip_address (), from glib 2.22
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#1617 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AArldlMF6mGkVfQSisdZg_SEnfCCNMNwks5sRJi5gaJpZM4OhEHG>
.
|
|
Build SUCCESS, the tests were executed on test branch: master and test suite: functions |
|
Instead of explicitely validating for ipv6 address, I added a new version which relaxes the condition for colon being delimiter. The new heuristics just check a few extra characters around the colon, so probably have lower effect on performance than calling a glib function or inet_pton. |
|
We should check a bit more strictly that it seems like an ipv6 address. (Hexa numbers, colons...) |
furiel
force-pushed
the
hostname_ipv6
branch
2 times, most recently
from
cad7577
to
a6524a4
Compare
|
Build SUCCESS, the tests were executed on test branch: master and test suite: functions |
1 similar comment
|
Build SUCCESS, the tests were executed on test branch: master and test suite: functions |
|
@bazsi I replaced the previous heuristics with a more strict one. In the end, an ipv6-heuristics class added just checks if the candidate is max 8 blocks of max 4-wide hexa characters. I felt the class useful, because the state of the ipv6 part became more complex than the previous one. The good news is I could still avoid memory allocation, so I am very hopeful that we do not lose too much performance. Anyway, we will run a performance test for this version next week. |
modules/syslogformat/syslog-format.c
Outdated
| if (c == ':') | ||
| { | ||
| self->digits_in_segment = 0; | ||
| if (self->current_segment > 8) |
There was a problem hiding this comment.
current_segment is not incremented.
modules/syslogformat/syslog-format.c
Outdated
| typedef struct _IPv6Heuristics | ||
| { | ||
| gchar current_segment; | ||
| gchar digits_in_segment; |
There was a problem hiding this comment.
Please use guint8/gint8 for 8 bit integers.
modules/syslogformat/syslog-format.c
Outdated
| gchar current_segment; | ||
| gchar digits_in_segment; | ||
| gboolean heuristic_failed; | ||
| gboolean (*feed_gchar)(struct _IPv6Heuristics *, gchar); |
There was a problem hiding this comment.
Is there any reason why you made this virtual?
|
Build SUCCESS, the tests were executed on test branch: master and test suite: functions |
|
Build SUCCESS, the tests were executed on test branch: master and test suite: functions |
Hostname parser expected colon to separate msg part from host-program part. But colon is valid character for host in case of ipv6. So some heuristics are added to allow colon during parsing hostname: looking for at maximum 8 blocks of at maximum 4-hexa values, separated by :. This heuristic automatically allows usage of double colons as well. Signed-off-by: Antal Nemes <antal.nemes@balabit.com>
|
Build SUCCESS, the tests were executed on test branch: master and test suite: functions |
|
the perftest passed, there was no regression |
Fixes:
#1616