-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
log-pcap: fix segfault on lz4 compressed pcaps #6875
Conversation
Codecov Report
@@ Coverage Diff @@
## master #6875 +/- ##
==========================================
- Coverage 77.72% 77.56% -0.17%
==========================================
Files 628 628
Lines 186493 186921 +428
==========================================
+ Hits 144959 144992 +33
- Misses 41534 41929 +395
Flags with carried forward coverage won't be shown. Click here to find out more. |
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.
Fixed the formatting so checks don't fail.
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.
Thanks for your interest in contributing to Suricata!
We would appreciate if you followed our code submission criteria :)
Especially:
- commit message formatting
- commit message and naming convention (so something like:
log-pcap: fix segfault on lz4 compressed pcaps
and then detail more in the commit body? ) - don't add unnecessary commits (if the formatting fix covers your code changes only, please squash them ;) )
Also, have you signed the Suricata Contribution Agreement?
Hi, Edit: I think I figured out how to squash them with |
commit 30960b2 Author: Marshall Whittaker <marshallwhittaker@gmail.com> Date: Thu Jan 27 13:52:56 2022 -0500 Run the clang 9 formatting script on code fix for lz4 pcap segfault. log-pcap: fix segfault on lz4 compressed pcaps
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.
See inline comments
@@ -576,6 +576,12 @@ static int PcapLog (ThreadVars *t, void *thread_data, const Packet *p) | |||
SCLogError(SC_ERR_FSEEK, "fseek failed: %s", strerror(errno)); | |||
return TM_ECODE_FAILED; | |||
} | |||
comp->file = fopen(pl->filename, "w"); |
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 don't see a check for comp->file
here, which suggests it might be already open and this would leak the previous descriptor. We're in the packet path here, so we would call this for each packet.
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.
If comp->file
is null here, I think it means we didn't properly error check something during setup. I think the issue should be handled during setup/initialization (or file rotation)
src/log-pcap.c
Outdated
"Error opening file for compressed output: %s", | ||
strerror(errno)); | ||
/* took this out so that it won't print twice, but still would function | ||
same SCLogError(SC_ERR_OPENING_FILE, "Error opening file for compressed output: |
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.
if removing output please just remove it instead of leaving it as a comment. It is fine to have a comment explaining why there is no error message here
Kudos! Now you can follow those with the next PR submission :) Can you also create a ticket on our redmine project, to go with that iteration? |
Probably correct to check if this directory is writeable on startup. I didn't happen to look, is everything else (that's working properly) also checked this way? I'm guessing that if the perms somhow got changed incorrectly while Suricata is running it would also fail that way, but the risk is much lower and the performance implications may not be worth doing it in the packet thread. |
Sounds about right to me. Directory should exist and be writable if we are willing to write something in there. |
Warning: no commits in this PR have specified the following ticket(s): Please update the commit(s) and submit a new PR. |
@oxagast are you still working on this ? |
Sorry, I completely forgot about this. I'll have to take another look into it. Thanks for reminding me. |
So I just recompiled the latest version out of OSIF:master and it seems it is already fixed. Before Suricata would try to log the .lz4 compressed logs to a separate directory under the default logging dir. If that dir was not writable it would fault. However now it seem Suricata has changed where it is dropping the .lz4 logs, and is now putting them in the default (or user specified) logging directory, not in a sub-directory. So it seems to be fixed, either intentionally or unintentionally. I'll now close this issue. Thanks! |
Moved a file existence check to fix segfault if Suricata tries to write to an lz4 compressed .pcap file if you don't have permission to write to it. Also removed the previous code that would log this elsewhere, so you don't get double log entries.
https://redmine.openinfosecfoundation.org/issues/5022