Add fuzzing test to flowgger #79
Conversation
Co-authored-by: Chukwuemeka Ewurum <[email protected]>
| @@ -7,7 +7,7 @@ mod tcp; | |||
| #[cfg(feature = "tls")] | |||
| mod tls; | |||
| #[cfg(feature = "syslog")] | |||
| mod udp_input; | |||
| pub mod udp_input; | |||
There was a problem hiding this comment.
can you make those public only in test mode ? and keep them private otherwise ? Applicable to all scope changes
flowgger.toml
Outdated
| ################### | ||
| # Fuzzing Test # | ||
| ################### | ||
| fuzzed_message_count = 500 |
There was a problem hiding this comment.
why is this in the prod config ? this should be in the test file as constant or in a dedicated file. not in the config file
There was a problem hiding this comment.
will do. Thanks
tests/fuzzer.rs
Outdated
|
|
||
| const DEFAULT_FUZZED_MESSAGE_COUNT: u64 = 500; | ||
|
|
||
| fn get_file_output(config: &Config) -> Box<dyn Output> { |
There was a problem hiding this comment.
there is not a single comment in the file. please explain what you're doing, why, what are the success criteria, when is this run....
tests/fuzzer.rs
Outdated
| }; | ||
|
|
||
| if let Some(entry) = config.config.get_mut("output").unwrap().get_mut("file_rotation_time"){ | ||
| *entry = Value::Integer(0); |
There was a problem hiding this comment.
why are you checking for the file rotation time ? it's an optional config entry anyway
There was a problem hiding this comment.
This sets the file_rotation_time to 0, so that multiple output files are not created during the test. Will add comments related to this.
tests/fuzzer.rs
Outdated
| let file_output_path = config.lookup("output.file_path").map_or(DEFAULT_OUTPUT_FILEPATH, |x| { | ||
| x.as_str().expect("File output path missing in config") | ||
| }); | ||
| remove_output_file(&file_output_path); |
There was a problem hiding this comment.
if the output file doesn't exist, will you panic ? shouldn't you expect the output to not be found ?
There was a problem hiding this comment.
This refers to the output filename defined in the config file, not the actual file on the physical filesystem.
If it's missing, this defaults to the constant value defined, else panics
tests/fuzzer.rs
Outdated
| #[test] | ||
| fn test_fuzzer(){ | ||
| let config = get_config(); | ||
| let fuzzed_message_count = match config.lookup("test.fuzzed_message_count"){ |
There was a problem hiding this comment.
the test config shouldnt' be in the prod config
There was a problem hiding this comment.
addressed in the next rev
tests/fuzzer.rs
Outdated
| }); | ||
| remove_output_file(&file_output_path); | ||
|
|
||
| if let Ok(s) = std::str::from_utf8(data) { |
There was a problem hiding this comment.
s as a variable is not a readable name, please update
tests/fuzzer.rs
Outdated
| fs::remove_file(file_output_path); | ||
| } | ||
|
|
||
| pub fn fuzz_target_rfc3164(data: &[u8]) { |
There was a problem hiding this comment.
so, am i getting this right ?are you creating a listener for each message, send the message, check the output file, delete it, close the listeners, and do that again ? 500 times ???
why not just create the listeners, fuzz data with handle_record_maybe_compressed and open the output file just once, parse the records, and error out if one can't be read ?
There was a problem hiding this comment.
addressed in next rev
Co-authored-by: Chukwuemeka Ewurum <[email protected]>
src/flowgger/fuzzer.rs
Outdated
| @@ -0,0 +1,210 @@ | |||
| extern crate quickcheck; | |||
|
|
|||
| use crate::flowgger; | |||
There was a problem hiding this comment.
if this is in source file, and only used for test, make it clear. e.g. like in test_utils: prefix the file with test_ to make it clear it is not a source file. and don't make any import outside the test clause to make sure nothing is imported
src/flowgger/fuzzer.rs
Outdated
|
|
||
| use crate::flowgger; | ||
|
|
||
| use quickcheck::QuickCheck; |
There was a problem hiding this comment.
all those modules will be imported in prod compilation as well, even thoguh no code. and modules like quickcheck should are only available as dev dependency, so it would fail.
src/flowgger/fuzzer.rs
Outdated
| let encoder = get_encoder_rfc3164(&config); | ||
| let decoder = get_decoder_rfc3164(&config); | ||
| let (decoder, encoder): (Box<dyn Decoder>, Box<dyn Encoder>) = | ||
| (decoder.clone_boxed(), encoder.clone_boxed()); |
There was a problem hiding this comment.
i think we talked that basically only the handle_record needs to be called fore very message. cloning the static config and recreating the same encoder and decoder for every message is useless.
I expect tx, decoder, and encoder to be shared. E.g. have a structure containing the 3 of them which you chsare with your mutex ?
src/flowgger/fuzzer.rs
Outdated
| let tx: SyncSender<Vec<u8>> = sender_guard.take().unwrap(); | ||
| drop(tx); | ||
|
|
||
| thread::sleep(time::Duration::from_millis(1000)); |
There was a problem hiding this comment.
explain why you need to sleep please. if the drop is not blocking and you're waiting, how do you know 1 sec is enough ?
Co-authored-by: Chukwuemeka Ewurum <[email protected]>
Co-authored-by: Chukwuemeka Ewurum <[email protected]>
Co-authored-by: Chukwuemeka Ewurum <[email protected]>
Co-authored-by: Chukwuemeka Ewurum <[email protected]>
src/flowgger/test_fuzzer.rs
Outdated
| pub fn fuzz_target_rfc3164(data: String) { | ||
| unsafe{ | ||
| // Extract the required fields from the global context structure, which is wrapped around by a Mutex | ||
| let mut guard = match GLOBAL_CONTEXT.lock() { |
There was a problem hiding this comment.
Set_context(), we could hsve had a get_context() 😬. I'm being picky
|
Nice improvements since the first commit ! |
Co-authored-by: Chukwuemeka Ewurum <[email protected]>
|
Build fails due to workflow and cross compilation being broken. I've enabled manual runs for the workflow and re ran it on the current release (which previously passed) https://github.com/awslabs/flowgger/actions/runs/9127101215. And it fails the same way. Since we're using a cross build and openssl is not included anymore, we were using an older version. we'll have to fix this. I've opened an issue for us to fix the workflow - again -. In the meantime, having tested this locally, and no production code being modified, it is safe to merge. |
Description of changes:
This change seeks to add fuzzing tests to Flowgger. With arbitrary strings as inputs, Flowgger is expected to either throw an error or successfully parse the log entry.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.