-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
in_tail: avoid double free for multiline msgpack buffer #4187
Conversation
Signed-off-by: Rayhan Hossain <[email protected]>
33a5a85
to
5fd0236
Compare
@@ -489,6 +489,7 @@ int flb_tail_mult_flush(msgpack_sbuffer *mp_sbuf, msgpack_packer *mp_pck, | |||
file->mult_keys = 0; | |||
file->mult_flush_timeout = 0; | |||
msgpack_sbuffer_destroy(&file->mult_sbuf); | |||
file->mult_sbuf.data = NULL; |
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.
So by setting it to NULL, then later on line 1024, when msgpack_sbuffer_destroy
is called, it won't free it again since calling free on NULL does nothing, and that function is just a wrapper around free: https://github.com/msgpack/msgpack-c/blob/c_master/include/msgpack/sbuffer.h#L39
@hossain-rayhan Just for clarification, you wrote "Fluent Bit is crashing". I'm wondering if your finding is related to the "[error] [multiline] invalid stream id , could not append content to multiline context" error which seams to plague quite a couple of installations (including us) when looking at forums. fluent-bit is not crashing in this case, did'nt check code until now, but i guess exception is handled, msg written to stderr and fb continues. So do you expect that your fix will solve this when included in upcoming release ? |
Hi @RalfWenzel , I don't think I saw |
Thanks for explanation. I'll dig a little deeper. |
follow up: issue related to message i mentioned above: #4190 |
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.
LGTM.
By the way, msgpack-c supports an API to set NULL.
How about it ?
https://github.com/fluent/fluent-bit/blob/v1.8.8/lib/msgpack-c/include/msgpack/sbuffer.h#L94
@hossain-rayhan 1.8 PR? |
Hey sorry, I will cut this today. |
PR against 1.8: #4243 |
Signed-off-by: Rayhan Hossain [email protected]
We are double freeing
&file->mult_sbuf
for multiline flush and Fluent Bit is crasing. This happens when we use multiline parser and rotate the file.The first free is happening at
tail_multiline.c:491
and the next free is happening attail_file.c:1024
.From my investigation, I think we can avoid it in two different ways.
tail_multiline.c:491
which I am doing in this PR. This way we won't have an issue if tail_file.c tries again to free that NULL assigned memory. I tested and it works.tail_file.c:1024
. I am not sure if it has any other use case. But this will also work and I have tested it.Fixes #4177
Copy: aws/aws-for-fluent-bit#255
Valgrind Output:
Enter
[N/A]
in the box, if an item is not applicable to your change.Testing
Before we can approve your change; please submit the following in a comment:
N/A
] Example configuration file for the changeDocumentation
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.