Add fuzzing test to flowgger #79

cahakgeorge · 2024-05-10T12:05:18Z

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.

Co-authored-by: Chukwuemeka Ewurum <cewurum@amazon.com>

vche · 2024-05-10T12:30:59Z

src/flowgger/input/mod.rs

@@ -7,7 +7,7 @@ mod tcp;
 #[cfg(feature = "tls")]
 mod tls;
 #[cfg(feature = "syslog")]
-mod udp_input;
+pub mod udp_input;


can you make those public only in test mode ? and keep them private otherwise ? Applicable to all scope changes

vche · 2024-05-10T12:33:25Z

flowgger.toml

+###################
+#  Fuzzing Test   #
+###################
+fuzzed_message_count = 500


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

will do. Thanks

vche · 2024-05-10T12:34:15Z

tests/fuzzer.rs

+
+const DEFAULT_FUZZED_MESSAGE_COUNT: u64 = 500;
+
+fn get_file_output(config: &Config) -> Box<dyn Output> {


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....

vche · 2024-05-10T12:35:42Z

tests/fuzzer.rs

+    };
+
+    if let Some(entry) = config.config.get_mut("output").unwrap().get_mut("file_rotation_time"){
+        *entry = Value::Integer(0);


why are you checking for the file rotation time ? it's an optional config entry anyway

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.

vche · 2024-05-10T12:45:02Z

tests/fuzzer.rs

+    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);


if the output file doesn't exist, will you panic ? shouldn't you expect the output to not be found ?

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

vche · 2024-05-10T12:46:40Z

tests/fuzzer.rs

+#[test]
+fn test_fuzzer(){
+    let config = get_config();
+    let fuzzed_message_count = match config.lookup("test.fuzzed_message_count"){


the test config shouldnt' be in the prod config

addressed in the next rev

vche · 2024-05-10T12:50:27Z

tests/fuzzer.rs

+    });
+    remove_output_file(&file_output_path);
+
+    if let Ok(s) = std::str::from_utf8(data) {


s as a variable is not a readable name, please update

vche · 2024-05-10T12:52:27Z

tests/fuzzer.rs

+    fs::remove_file(file_output_path);
+}
+
+pub fn fuzz_target_rfc3164(data: &[u8]) {


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 ?

addressed in next rev

Co-authored-by: Chukwuemeka Ewurum <cewurum@amazon.com>

vche · 2024-05-15T10:09:20Z

src/flowgger/fuzzer.rs

@@ -0,0 +1,210 @@
+extern crate quickcheck;
+
+use crate::flowgger;


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

vche · 2024-05-15T10:09:33Z

src/flowgger/fuzzer.rs

+
+use crate::flowgger;
+
+use quickcheck::QuickCheck;


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.

vche · 2024-05-15T10:18:14Z

src/flowgger/fuzzer.rs

+        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());


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 ?

vche · 2024-05-15T10:18:57Z

src/flowgger/fuzzer.rs

+        let tx: SyncSender<Vec<u8>> = sender_guard.take().unwrap();
+        drop(tx);
+
+        thread::sleep(time::Duration::from_millis(1000));


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 <cewurum@amazon.com>

vche · 2024-05-16T19:12:11Z

src/flowgger/test_fuzzer.rs

+    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() {


Set_context(), we could hsve had a get_context() 😬. I'm being picky

vche · 2024-05-16T19:13:54Z

Nice improvements since the first commit !

Co-authored-by: Chukwuemeka Ewurum <cewurum@amazon.com>

rejao

lgtm

vche · 2024-05-17T11:02:37Z

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.

Add fuzzing test to flowgger

d3c7d0a

Co-authored-by: Chukwuemeka Ewurum <cewurum@amazon.com>

vche requested changes May 10, 2024

View reviewed changes

cahakgeorge and others added 2 commits May 15, 2024 09:48

Add fuzzing test to flowgger

7627f6f

Co-authored-by: Chukwuemeka Ewurum <cewurum@amazon.com>

Add fuzzing test to flowgger

0d59908

cahakgeorge requested a review from vche May 15, 2024 09:52

vche reviewed May 15, 2024

View reviewed changes

Add fuzzing test to flowgger

fe72d17

Co-authored-by: Chukwuemeka Ewurum <cewurum@amazon.com>

cahakgeorge requested a review from vche May 16, 2024 15:14

cahakgeorge and others added 3 commits May 16, 2024 18:57

Add fuzzing test to flowgger

5d0c208

Co-authored-by: Chukwuemeka Ewurum <cewurum@amazon.com>

Add fuzzing test to flowgger

f4c98f6

Co-authored-by: Chukwuemeka Ewurum <cewurum@amazon.com>

Add fuzzing test to flowgger

eae05b5

Co-authored-by: Chukwuemeka Ewurum <cewurum@amazon.com>

vche approved these changes May 16, 2024

View reviewed changes

Updated code formatting

1248350

Co-authored-by: Chukwuemeka Ewurum <cewurum@amazon.com>

cahakgeorge requested a review from vche May 16, 2024 21:04

vche approved these changes May 17, 2024

View reviewed changes

rejao approved these changes May 17, 2024

View reviewed changes

vche merged commit 72f86cf into awslabs:master May 17, 2024
1 of 15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add fuzzing test to flowgger #79

Add fuzzing test to flowgger #79

cahakgeorge commented May 10, 2024

vche May 10, 2024

cahakgeorge May 14, 2024

vche May 10, 2024

cahakgeorge May 14, 2024

vche May 10, 2024

vche May 10, 2024

cahakgeorge May 14, 2024

vche May 10, 2024

cahakgeorge May 14, 2024

vche May 10, 2024

cahakgeorge May 14, 2024

vche May 10, 2024

cahakgeorge May 14, 2024

vche May 10, 2024

cahakgeorge May 14, 2024

vche May 15, 2024

vche May 15, 2024

vche May 15, 2024

vche May 15, 2024

vche May 16, 2024

vche commented May 16, 2024

rejao left a comment

vche commented May 17, 2024 •

edited


		const DEFAULT_FUZZED_MESSAGE_COUNT: u64 = 500;

		fn get_file_output(config: &Config) -> Box<dyn Output> {

		@@ -0,0 +1,210 @@
		extern crate quickcheck;

		use crate::flowgger;

Add fuzzing test to flowgger #79

Add fuzzing test to flowgger #79

Conversation

cahakgeorge commented May 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vche commented May 16, 2024

rejao left a comment

Choose a reason for hiding this comment

vche commented May 17, 2024 • edited

vche commented May 17, 2024 •

edited