Skip to content
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

Fuzz Testing Implementation #298

Merged
merged 1 commit into from
Jul 21, 2023
Merged

Fuzz Testing Implementation #298

merged 1 commit into from
Jul 21, 2023

Conversation

tylerzhao7684
Copy link
Contributor

@tylerzhao7684 tylerzhao7684 commented Jun 6, 2023

  • Fuzz Testing Directory and contents added
  • CMake configured for Fuzz Testing directory
  • Header file created to allow unit test to load data from a properly-formatted yaml file
  • Python script created to mutate a single variable and output to a new file
  • Python script created to do fuzz testing for a single unit test file
  • Python script created to do fuzz testing for multiple test files
  • Save fuzz testing outputs to a new directory
  • Load data for different data types
  • Identify which variables are suitable for fuzzing
  • Workaround for unit test memory leak issue
  • Interpret results from fuzz testing and determine if a failure is "secure"
  • Code Coverage Report
  • Workflow that triggers fuzz testing
  • Documentation on how to do fuzz testing locally

@zibaiwan zibaiwan self-requested a review June 16, 2023 14:51
@zibaiwan
Copy link
Contributor

Thanks Tyler for working on this. The approach looks good to me! I assume we won't check-in all unit tests file unless we have the corresponding yml file to fuzz test them, correct? I will take a closer look on the scripts too.

@tylerzhao7684
Copy link
Contributor Author

Proof that the workflow is correctly catching Address Sanitizer errors (ASAN errors):
This workflow run has 3 ASAN errors, which is due to this change, the reason why there are 3 is because that test is ran 3 times, due to 3 mutable variables in the input yml file

@tylerzhao7684
Copy link
Contributor Author

Proof that the test is able to detect hangs correctly:
This workflow run has 18 hangs because I changed the timeout to 1 second which is obviously not enough. The point is that the script is able to detect hang correctly and I would expect it to do the same if a test does not finish in 60 seconds

@tylerzhao7684
Copy link
Contributor Author

Proof that the test is able to catch assertion failures correctly:
This workflow run has 4 assertion failures because of this change. There are 4 of them, 3 of them are because that the same test ran 3 times, due to 3 mutable variables in the input yml file. The remaining assertion failure is an actual assertion failure due to mutated variable (Download the artifact here if you want to see the test output)

@tylerzhao7684 tylerzhao7684 marked this pull request as ready for review July 4, 2023 21:17
Copy link
Contributor

@zibaiwan zibaiwan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Tyler, I have reviewed some of your changes. They look very good to me. I had some small comments below. I will continue reviewing some technical details and add more comments.

@tylerzhao7684
Copy link
Contributor Author

Thanks for the review Zibai! I thought about it and I changed my mind on this. I believe although I built the fuzz test infra based off the current unit tests, we should not regard them as the same test. In other words. we can add different contents to the fuzz tests and the unit tests. The commonality of fuzz tests and unit tests is that they test the same source function (i.e. src/acl_auto_configure.cpp). The difference is that one tests for security vulnerability, the other one tests for correctness.

This opens up opportunity to create tests that are fuzz test only, such as a more in-depth test that goes through multiple source functions that interacts with the fuzzed variable.

I hope this answers:

I assume if any of those auto_discovery string is updated in the test/acl_auto_configure_test.cpp, they need to be updated here?

We update that file quite frequently, and then the developer needs to update this file and the yml file, which add more workload to the developer.

If the board/a10_ref, hardware/a10_ref_small and fake_bsp are exactly same as the one in the non fuzz test directory. I don't think we should checkin those files again. Can you see if you have anyway to reduce the redundancy here?

What do you think?

@zibaiwan
Copy link
Contributor

Thanks for the review Zibai! I thought about it and I changed my mind on this. I believe although I built the fuzz test infra based off the current unit tests, we should not regard them as the same test. In other words. we can add different contents to the fuzz tests and the unit tests. The commonality of fuzz tests and unit tests is that they test the same source function (i.e. src/acl_auto_configure.cpp). The difference is that one tests for security vulnerability, the other one tests for correctness.

This opens up opportunity to create tests that are fuzz test only, such as a more in-depth test that goes through multiple source functions that interacts with the fuzzed variable.

I hope this answers:

I assume if any of those auto_discovery string is updated in the test/acl_auto_configure_test.cpp, they need to be updated here?

We update that file quite frequently, and then the developer needs to update this file and the yml file, which add more workload to the developer.

If the board/a10_ref, hardware/a10_ref_small and fake_bsp are exactly same as the one in the non fuzz test directory. I don't think we should checkin those files again. Can you see if you have anyway to reduce the redundancy here?

What do you think?

Hi Tyler, yeah I agree we should consider those tests are different. But I think the developer should still update the fuzz test if they make the change to the acl_auto_configure.cpp, right? I guess you meant they are similar right now but they can be very different in the future, I agree with you and that means the automation is not neccessary.

However, I do believe we should not create duplicate files of those bsp files as I don't think we will do anything different for BSP in fuzz testing. Can you check if there is anyway the fuzz test can just reuse those BSPs?

@tylerzhao7684
Copy link
Contributor Author

Thanks for the review Zibai! I thought about it and I changed my mind on this. I believe although I built the fuzz test infra based off the current unit tests, we should not regard them as the same test. In other words. we can add different contents to the fuzz tests and the unit tests. The commonality of fuzz tests and unit tests is that they test the same source function (i.e. src/acl_auto_configure.cpp). The difference is that one tests for security vulnerability, the other one tests for correctness.
This opens up opportunity to create tests that are fuzz test only, such as a more in-depth test that goes through multiple source functions that interacts with the fuzzed variable.
I hope this answers:

I assume if any of those auto_discovery string is updated in the test/acl_auto_configure_test.cpp, they need to be updated here?

We update that file quite frequently, and then the developer needs to update this file and the yml file, which add more workload to the developer.

If the board/a10_ref, hardware/a10_ref_small and fake_bsp are exactly same as the one in the non fuzz test directory. I don't think we should checkin those files again. Can you see if you have anyway to reduce the redundancy here?

What do you think?

Hi Tyler, yeah I agree we should consider those tests are different. But I think the developer should still update the fuzz test if they make the change to the acl_auto_configure.cpp, right? I guess you meant they are similar right now but they can be very different in the future, I agree with you and that means the automation is not neccessary.

However, I do believe we should not create duplicate files of those bsp files as I don't think we will do anything different for BSP in fuzz testing. Can you check if there is anyway the fuzz test can just reuse those BSPs?

Yup I'll remove the BSPs in the fuzz_testing directory.
Note that we only reference BSP when doing export AOCL_BOARD_PACKAGE_ROOT="$(git rev-parse --show-toplevel)/test/board/a10_ref" as documented in runtime README, as a pre-requisite for running unit tests.

@tylerzhao7684 tylerzhao7684 requested a review from zibaiwan July 19, 2023 17:07
@tylerzhao7684 tylerzhao7684 force-pushed the fuzz branch 2 times, most recently from f2d041a to c421c14 Compare July 20, 2023 21:29
@zibaiwan zibaiwan self-requested a review July 21, 2023 14:04
zibaiwan
zibaiwan previously approved these changes Jul 21, 2023
Copy link
Contributor

@zibaiwan zibaiwan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Tyler. I think this PR is ready to be merged once the pull_request testing removed in the yml.

@zibaiwan zibaiwan merged commit 98ab300 into intel:main Jul 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants