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 golang protoreflect project #5652

Merged
merged 2 commits into from
May 12, 2021

Conversation

catenacyber
Copy link
Contributor

@jhump would you be interested in continuous fuzzing for protoreflect ?
This enables it with one simple target about protoparse inspired by the one from @johanbrandhorst

@catenacyber
Copy link
Contributor Author

cc @bradleyjkemp as well for fuzzing protoreflect already

@bradleyjkemp
Copy link

The github.com/jhump/protoreflect/dynamic package might also be a good fuzz target (it handles the protobuf wire format).

I included a basic fuzz harness in jhump/protoreflect#233 (comment)

@jhump
Copy link
Contributor

jhump commented Apr 22, 2021

@catenacyber, very cool, definitely interested. Though I must admit that I have no idea how this will work: what runs the fuzzers and when? How am I notified that an issue is found? Is this something I add to CI for my own project and somehow link to this config?

@catenacyber
Copy link
Contributor Author

what runs the fuzzers and when?

Google runs oss-fuzz continuously

How am I notified that an issue is found?

You get a mail.
So, you need to supply a mail address to get these mails...
And it needs to be associated with a google account so that you can access the detailed report linked in the mail

Is this something I add to CI for my own project and somehow link to this config?

After this is merged, you can use CIFuzz in your GitHub CI cf https://google.github.io/oss-fuzz/getting-started/continuous-integration/

@catenacyber
Copy link
Contributor Author

The github.com/jhump/protoreflect/dynamic package might also be a good fuzz target (it handles the protobuf wire format).

I included a basic fuzz harness in jhump/protoreflect#233 (comment)

@bradleyjkemp thanks, but this fuzz target seems to fail on first input and I do not know why
Can you wee if I did something wrong ?

@bradleyjkemp
Copy link

@bradleyjkemp thanks, but this fuzz target seems to fail on first input and I do not know why
Can you wee if I did something wrong ?

How do you mean it fails on the first input?

I've tried running that Fuzz function with a couple strings and it seems to run fine (doesn't immediately panic). Returns errors most of the time but that's expected unless you seed it with a few valid proto message byte sequences

@catenacyber
Copy link
Contributor Author

How do you mean it fails on the first input?

I've tried running that Fuzz function with a couple strings and it seems to run fine (doesn't immediately panic). Returns errors most of the time but that's expected unless you seed it with a few valid proto message byte sequences

I get this kind of output

signal 11 received but handler not on signal stack
fatal error: non-Go code set up signal handler without SA_ONSTACK flag

runtime stack:
runtime: unexpected return pc for runtime.sigtramp called from 0x7ff11b6e4390
stack: frame={sp:0x10c0000bd1e8, fp:0x10c0000bd240} stack=[0x10c0000b5118,0x10c0000bd518)
000010c0000bd0e8:  000010c0000bd108  00000000005a1aa5 <runtime.sigNotOnStack+133> 
000010c0000bd0f8:  000000000092e9c3  0000000000000039 
000010c0000bd108:  000010c0000bd160  00000000005a0a85 <runtime.adjustSignalStack+645> 
000010c0000bd118:  000000000000000b  000010c0000bd148 
000010c0000bd128:  000010c0000bd170  000010c000000600 
000010c0000bd138:  00756ea1389fb8b8  70d04a531fb8c4f4 
000010c0000bd148:  00007ff11be07000  0000000000000000 
000010c0000bd158:  0000000000008000  000010c0000bd1d8 
000010c0000bd168:  00000000005a06ff <runtime.sigtrampgo+319>  000010c00000000b 
000010c0000bd178:  000010c00005e000  000010c0000bd198 
000010c0000bd188:  0000000000563f00 <runtime.mallocgc+736>  000010c0000b7c70 
000010c0000bd198:  0000000000000000  0000000000000000 
000010c0000bd1a8:  0000000000000000  0000000000000000 
000010c0000bd1b8:  0000000000000000  000010c000000600 
000010c0000bd1c8:  000010c0000bd370  000010c0000bd240 
000010c0000bd1d8:  000010c0000bd230  00000000005c1683 <runtime.sigtramp+67> 
000010c0000bd1e8: <000000000000000b  000010c0000bd370 
000010c0000bd1f8:  000010c0000bd240  0000000000000000 
000010c0000bd208:  000010c000000600  000010c00005e000 
000010c0000bd218:  000010c0000bd8f0  000010c0000bd230 
000010c0000bd228:  0000000000dfc8e0  000010c0000bd968 
000010c0000bd238: !00007ff11b6e4390 >0000000000000007 
000010c0000bd248:  0000000000000000  00007ff11be07000 
000010c0000bd258:  000010c000000000  0000000000008000 
000010c0000bd268:  0000000000dfcf60  00000000008a6720 <github.com/jhump/protoreflect/dynamic.(*Message).Descriptor+0> 

If I run this as a main function, I do not get this failure, but I get it when compiling with libfuzzer...

@catenacyber catenacyber marked this pull request as ready for review May 3, 2021 14:11
@fmeum
Copy link
Contributor

fmeum commented May 7, 2021

@catenacyber You are seeing this particular error because of a bug in libFuzzer that causes it to interfer with Go's signal handlers. This is fixed by https://reviews.llvm.org/D101824.

@catenacyber
Copy link
Contributor Author

Thanks @fmeum
May it be the same bug that caused #5080 ?

@fmeum
Copy link
Contributor

fmeum commented May 10, 2021

Thanks @fmeum
May it be the same bug that caused #5080 ?

I'm not sure, but I would tend to say no. The bug fixed by https://reviews.llvm.org/D101824 is 1) purely related to signal handlers and 2) only interfers with Go if the Go program panics/segfaults in the first place.

@catenacyber
Copy link
Contributor Author

friendly ping @asraa

PS : Is it planned to upgrade clang to get the fix from https://reviews.llvm.org/D101824 ?
Right now, I get clang version 12.0.0 (https://github.com/llvm/llvm-project.git 6de4865545da73687dd6d28d153cd345ed5e7918)

Copy link
Contributor

@jonathanmetzman jonathanmetzman left a comment

Choose a reason for hiding this comment

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

LGTM

@jonathanmetzman jonathanmetzman merged commit 90de002 into google:master May 12, 2021
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.

5 participants