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

Adds fuzz testing for demangle #878

Merged
merged 1 commit into from
Feb 24, 2023
Merged

Adds fuzz testing for demangle #878

merged 1 commit into from
Feb 24, 2023

Conversation

catenacyber
Copy link
Contributor

@sergiud here is a PR to get fuzzing built with cmake
cf google/oss-fuzz#9160 (comment)

Copy link
Collaborator

@sergiud sergiud left a comment

Choose a reason for hiding this comment

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

Sorry for the late review. I have a couple of remarks that need to be addressed.

CMakeLists.txt Outdated Show resolved Hide resolved
src/fuzz_demangle.cc Outdated Show resolved Hide resolved
src/fuzz_demangle.cc Outdated Show resolved Hide resolved
src/fuzz_demangle.cc Outdated Show resolved Hide resolved
@catenacyber
Copy link
Contributor Author

@sergiud I pushed fixes for your review :-)

@catenacyber
Copy link
Contributor Author

friendly ping @sergiud ;-)

@sergiud
Copy link
Collaborator

sergiud commented Feb 19, 2023

Thanks for the ping @catenacyber. I'm very sorry for the delay. I will look into PR during the coming week. Please stay tuned.

@sergiud
Copy link
Collaborator

sergiud commented Feb 24, 2023

Ideally, we would run fuzzing as part of our Github workflow. What do we need for that?

src/fuzz_demangle.cc Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@catenacyber
Copy link
Contributor Author

Ideally, we would run fuzzing as part of our Github workflow. What do we need for that?

3 steps :

@sergiud sergiud added this to the 0.7 milestone Feb 24, 2023
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@catenacyber
Copy link
Contributor Author

@sergiud I hope I got cmake right ;-)

CMakeLists.txt Outdated Show resolved Hide resolved
@sergiud
Copy link
Collaborator

sergiud commented Feb 24, 2023

@catenacyber Almost there.

Another question: glog will be moving to C++14 shortly. Anything that we need to consider here?

@catenacyber
Copy link
Contributor Author

I do not see why c++14 would be a problem.

@sergiud
Copy link
Collaborator

sergiud commented Feb 24, 2023

OK, great!

One more thing: once you are done with the last change, please also rebase onto current master and squash if possible.

@catenacyber
Copy link
Contributor Author

OK, great!

One more thing: once you are done with the last change, please also rebase onto current master and squash if possible.

Done ;-)

@codecov-commenter
Copy link

codecov-commenter commented Feb 24, 2023

Codecov Report

Merging #878 (ebc7804) into master (1b59cb0) will decrease coverage by 0.37%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #878      +/-   ##
==========================================
- Coverage   73.94%   73.58%   -0.37%     
==========================================
  Files          17       17              
  Lines        3282     3282              
==========================================
- Hits         2427     2415      -12     
- Misses        855      867      +12     
Impacted Files Coverage Δ
src/logging.cc 74.18% <0.00%> (-0.87%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@sergiud
Copy link
Collaborator

sergiud commented Feb 24, 2023

Thanks!

@sergiud sergiud merged commit 846c3fe into google:master Feb 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants