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

Added 2 fuzzers #13818

Merged
merged 15 commits into from
Dec 10, 2020
Merged

Added 2 fuzzers #13818

merged 15 commits into from
Dec 10, 2020

Conversation

AdamKorcz
Copy link
Contributor

@AdamKorcz AdamKorcz commented Dec 3, 2020

This PR adds 2 fuzzers that target markdown.RenderRaw and markup.PostProcess respectively. These fuzzers are implemented by way of the go-fuzz fuzzing engine.

Fuzzing is a way of testing programs whereby pseudo-random data is passed to a target with the goal of finding bugs and vulnerabilities.

I have worked on setting continuous fuzzing up for these two fuzzers which will allow them to run for longer and find harder-to-find bugs as well. I have set up the integration files for gitea on OSS-fuzz (google/oss-fuzz#4775), and upon merging the 2 fuzzers here, they would be able to run continuously through OSS-fuzz. OSS-fuzz is a free service for open source projects, and if/when bugs are found, maintainers get notified with an email containing a link to a detailed bug report with stacktrace and reproducer test-case. While it is a free service it is offered with an implied expectation that bugs are fixed, so that the resources spent on fuzzing gitea go towards resolving bugs in the codebase.

All that is needed to setup continuous fuzzing would be at least one maintainer email address.

For some examples of previous bugs found from fuzzing Golang projects I recommend checking out the trophy list on the go-fuzz repository: https://github.com/dvyukov/go-fuzz#trophies

Signed-off-by: AdamKorcz adam@adalogics.com

Signed-off-by: AdamKorcz <adam@adalogics.com>
Signed-off-by: AdamKorcz <adam@adalogics.com>
@AdamKorcz
Copy link
Contributor Author

OSS-fuzz integration set up here: google/oss-fuzz#4775

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 3, 2020
@codecov-io
Copy link

codecov-io commented Dec 3, 2020

Codecov Report

Merging #13818 (ee3b406) into master (1cb1fb8) will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #13818      +/-   ##
==========================================
+ Coverage   42.21%   42.23%   +0.01%     
==========================================
  Files         710      710              
  Lines       77232    77232              
==========================================
+ Hits        32607    32616       +9     
+ Misses      39255    39252       -3     
+ Partials     5370     5364       -6     
Impacted Files Coverage Δ
modules/util/timer.go 42.85% <0.00%> (-42.86%) ⬇️
modules/indexer/stats/queue.go 52.94% <0.00%> (-23.53%) ⬇️
modules/indexer/stats/db.go 43.47% <0.00%> (-8.70%) ⬇️
modules/charset/charset.go 68.53% <0.00%> (-4.50%) ⬇️
services/mailer/mail.go 60.21% <0.00%> (-1.08%) ⬇️
services/pull/pull.go 40.68% <0.00%> (-0.50%) ⬇️
modules/notification/mail/mail.go 39.08% <0.00%> (ø)
models/error.go 39.31% <0.00%> (+0.32%) ⬆️
models/notification.go 67.24% <0.00%> (+0.98%) ⬆️
services/mirror/mirror.go 16.89% <0.00%> (+1.10%) ⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0c5fca2...ee3b406. Read the comment docs.

@lafriks
Copy link
Member

lafriks commented Dec 3, 2020

Email for receiving reports could be admin@gitea.io

@silverwind
Copy link
Member

silverwind commented Dec 3, 2020

Can we not introduce another top-level directory? I'd suggest making a tools/fuzz.go or tools/fuzz/fuzz.go for this.

@AdamKorcz
Copy link
Contributor Author

AdamKorcz commented Dec 3, 2020

@silverwind Not a problem from a fuzzing-perspective. Just let me know if that is what we go for.

@lafriks Duly noted!

@silverwind
Copy link
Member

I'd say start with tools/fuzz.go if you expect this to stay a single file, otherwise make a directory.

@AdamKorcz
Copy link
Contributor Author

@silverwind Done!

tools/fuzz.go Show resolved Hide resolved
@silverwind
Copy link
Member

silverwind commented Dec 4, 2020

Is it possible to make this runnable locally? e.g. through a make target like make fuzz.

Co-authored-by: 6543 <6543@obermui.de>
@AdamKorcz
Copy link
Contributor Author

Is it possible to make this runnable locally? e.g. through a make target like make fuzz.

Sure is. This would install some dependencies locally. I would prefer to set this up in a separate PR as it can be a quick fix or a longer task depending on the existing build system which I have not perused yet. Let me know if you are fine with that or would prefer it in this PR. Either way I am happy to do it.

@6543 6543 added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Dec 4, 2020
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 4, 2020
@lunny
Copy link
Member

lunny commented Dec 4, 2020

Could this be a feature of Gitea to check repositories hosted on Gitea instance?

@AdamKorcz
Copy link
Contributor Author

@lunny I am afraid I don't understand what you mean by "check"?

@silverwind
Copy link
Member

Let me know if you are fine with that or would prefer it in this PR

I guess it's fine to do separately if you promise to follow up on it 😉

@6543 6543 removed the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 5, 2020
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 6, 2020
@AdamKorcz
Copy link
Contributor Author

I guess it's fine to do separately if you promise to follow up on it 😉

I will be happy to. I would like to see this integrated into the build system.

@GiteaBot GiteaBot removed the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 7, 2020
tools/fuzz.go Outdated Show resolved Hide resolved
tools/fuzz.go Outdated Show resolved Hide resolved
6543 and others added 2 commits December 9, 2020 15:11
Co-authored-by: silverwind <me@silverwind.io>
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Dec 9, 2020
@silverwind
Copy link
Member

silverwind commented Dec 9, 2020

Oh, I think we need to add tools to GO_DIRS in Makefile so it gets linted and stuff.

@6543
Copy link
Member

6543 commented Dec 9, 2020

@AdamKorcz

diff --git a/Makefile b/Makefile
index 0e33047aa..e21cf20f8 100644
--- a/Makefile
+++ b/Makefile
@@ -110,7 +110,7 @@ TAGS ?=
 TAGS_SPLIT := $(subst $(COMMA), ,$(TAGS))
 TAGS_EVIDENCE := $(MAKE_EVIDENCE_DIR)/tags
 
-GO_DIRS := cmd integrations models modules routers build services vendor
+GO_DIRS := cmd integrations models modules routers build services vendor tools
 GO_SOURCES := $(wildcard *.go)

@6543
Copy link
Member

6543 commented Dec 10, 2020

@AdamKorcz ping ?

@AdamKorcz
Copy link
Contributor Author

Still here. I will have a look at the diff this evening

@AdamKorcz
Copy link
Contributor Author

@6543 Sorry for the delay. I ran the patch with git apply but I get the error "error: corrupt patch at line 12".

@zeripath
Copy link
Contributor

@6543 Sorry for the delay. I ran the patch with git apply but I get the error "error: corrupt patch at line 12".

AFAIU it just needs an extra empty newline

@AdamKorcz
Copy link
Contributor Author

AFAIU it just needs an extra empty newline

Thanks for the suggestion. That just gave "... at line 13".

Nonetheless, I have added tools to the Makefile manually. Let me now if any further changes are needed from my side.

@6543 6543 merged commit 94415f7 into go-gitea:master Dec 10, 2020
@6543
Copy link
Member

6543 commented Dec 10, 2020

@AdamKorcz pleace follow up with a make target to fuzz localy :)

@go-gitea go-gitea locked and limited conversation to collaborators Jan 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants