intel / fpga-runtime-for-opencl

Intel® FPGA Runtime for OpenCL™ Software Technology
Other
34 stars 68 forks source link

Fuzz Testing Implementation #298

Closed tylerzhao7684 closed 1 year ago

tylerzhao7684 commented 1 year ago
zibaiwan commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.