You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by GitBox <gi...@apache.org> on 2021/03/15 08:40:53 UTC
[GitHub] [thrift] catenacyber opened a new pull request #2351: Adds fuzz target for oss-fuzz integration
catenacyber opened a new pull request #2351:
URL: https://github.com/apache/thrift/pull/2351
<!-- Explain the changes in the pull request below: -->
Adds a fuzz target for oss-fuzz integration cf https://github.com/google/oss-fuzz/pull/5264
<!-- We recommend you review the checklist/tips before submitting a pull request. -->
- [ ] Did you create an [Apache Jira](https://issues.apache.org/jira/projects/THRIFT/issues/) ticket? (not required for trivial changes)
This change is about adding a fuzz target, ie a new testing methodology... I do not know if you want a ticket for it
- [ ] If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
No ticket exists
- [X] Did you squash your changes to a single commit? (not required, but preferred)
- [ ] Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
This only adds a fuzz target, and does not touch the existing code
- [ ] If your change does not involve any code, include `[skip ci]` anywhere in the commit message to free up build resources.
This change adds code, but it is testing code, so I do not know if it should skip ci
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] fishy commented on a change in pull request #2351: Adds fuzz target for oss-fuzz integration
Posted by GitBox <gi...@apache.org>.
fishy commented on a change in pull request #2351:
URL: https://github.com/apache/thrift/pull/2351#discussion_r594657337
##########
File path: lib/go/test/fuzz/fuzz.go
##########
@@ -0,0 +1,140 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+// +build gofuzz
+
+package fuzz
+
+import (
+ "context"
+ "fmt"
+ "shared"
+ "strconv"
+ "tutorial"
+
+ "github.com/apache/thrift/lib/go/thrift"
+)
+
+const nbFuzzedProtocols = 2
+
+func fuzzChooseProtocol(d byte, t thrift.TTransport) thrift.TProtocol {
+ switch d % nbFuzzedProtocols {
+ case 0:
+ return thrift.NewTCompactProtocolFactoryConf(nil).GetProtocol(t)
+ }
+ return thrift.NewTBinaryProtocolFactoryConf(nil).GetProtocol(t)
Review comment:
nit: The current code works, but it's hard to reason about. I'd suggest something like this:
```go
switch d % nbFuzzedProtocols {
default:
fallthrough
case 0:
return thrift.NewTBinaryProtocolFactoryConf(nil).GetProtocol(t)
case 1:
return thrift.NewTCompactProtocolFactoryConf(nil).GetProtocol(t)
}
```
(note that I also swapped binary protocol and compact protocol, because binary really should be the default one).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] fishy commented on pull request #2351: Adds fuzz target for oss-fuzz integration
Posted by GitBox <gi...@apache.org>.
fishy commented on pull request #2351:
URL: https://github.com/apache/thrift/pull/2351#issuecomment-802148093
> @fishy do you know what is missing in my Makefile.am ?
> It seems target `all` is empty, but there is nothing to do if we do not build the tests...
Oh you are right. We should still do all the `cd fuzz && go mod edit` under the `test` directory because that's where they are actually used. Sorry about my misleading suggestion.
Actually I think if you just add a unit test under `test` directory to import from `fuzz`, we do not need to do all the `go mod edit` under `fuzz`. we either do them under `test` or not need to do them at all. The test itself passes means that the building of `fuzz` is working, so we do not need to build `fuzz`.
But if you don't want to add and just want to make sure that the building of `fuzz` works, then what you are doing right now should work. I'm not super familiar with automake so I'm not sure how is it supposed to generate the `all` target from `Makefile.am`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] catenacyber commented on pull request #2351: Adds fuzz target for oss-fuzz integration
Posted by GitBox <gi...@apache.org>.
catenacyber commented on pull request #2351:
URL: https://github.com/apache/thrift/pull/2351#issuecomment-799689941
@fishy should I squash the commits together ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] fishy commented on pull request #2351: Adds fuzz target for oss-fuzz integration
Posted by GitBox <gi...@apache.org>.
fishy commented on pull request #2351:
URL: https://github.com/apache/thrift/pull/2351#issuecomment-799724539
> > how is this code being used by libfuzz, exactly?
>
> cf https://github.com/google/oss-fuzz/pull/5264/files#diff-c54441e86b07048406afb5c7ed023b873669ff90d6b81613eedaaf4b90b7eb21R28
>
> I think the trick you are looking for is `go mod edit -replace tutorial=./gen-go/tutorial`
OK thanks.
Can you add the newly added `lib/go/test/thriftfuzz` directory to our travis CI system (e.g. add it to https://github.com/apache/thrift/blob/62beb6751d3c70f8db8fed4a3bb76e4ff3765c22/lib/go/test/Makefile.am#L88 rule)? Currently it's not checked by our CI system, so someone could accidentally break it (for example, make changes to `tutorial.thrift`) without realizing it.
This this is actually a library, I guess you would need to write a test to actually use this library and fill in some fixed bytes. We can't really fuzz it in our travis CI, but we can at least make sure that it still compiles.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] fishy commented on a change in pull request #2351: Adds fuzz target for oss-fuzz integration
Posted by GitBox <gi...@apache.org>.
fishy commented on a change in pull request #2351:
URL: https://github.com/apache/thrift/pull/2351#discussion_r595253172
##########
File path: lib/go/test/Makefile.am
##########
@@ -85,6 +86,17 @@ gopath: $(THRIFT) $(THRIFTTEST) \
cp -r ./dontexportrwtest gopath/src
touch gopath
+gopathfuzz: $(THRIFT) fuzz/fuzz.go
+ cd fuzz && $(THRIFT) -r --gen go ../../../../tutorial/tutorial.thrift
+ cd fuzz && go mod init fuzz
+ cd fuzz/gen-go/shared && go mod init shared
+ cd fuzz/gen-go/tutorial && go mod init tutorial
+ cd fuzz && go mod edit -replace shared=./gen-go/shared
+ cd fuzz && go mod edit -replace tutorial=./gen-go/tutorial
+ cd fuzz && go mod edit -replace github.com/apache/thrift/lib/go/thrift=../../../../lib/go/thrift
+ go build fuzz
Review comment:
this line should be `cd fuzz && go build` instead
(does make return to your current directory after every command? I see we did a lot of `cd fuzz` in the commends but never `cd ..`)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] catenacyber commented on pull request #2351: Adds fuzz target for oss-fuzz integration
Posted by GitBox <gi...@apache.org>.
catenacyber commented on pull request #2351:
URL: https://github.com/apache/thrift/pull/2351#issuecomment-802812336
> I'm not super familiar with automake so I'm not sure how is it supposed to generate the all target from Makefile.am.
Found I needed to make the `SUBDIRS += fuzz` conditional
I also added a golang test, let's see what CI thinks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] catenacyber commented on pull request #2351: Adds fuzz target for oss-fuzz integration
Posted by GitBox <gi...@apache.org>.
catenacyber commented on pull request #2351:
URL: https://github.com/apache/thrift/pull/2351#issuecomment-805575789
Squashed and force-pushed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] catenacyber commented on a change in pull request #2351: Adds fuzz target for oss-fuzz integration
Posted by GitBox <gi...@apache.org>.
catenacyber commented on a change in pull request #2351:
URL: https://github.com/apache/thrift/pull/2351#discussion_r594929249
##########
File path: lib/go/test/fuzz/fuzz.go
##########
@@ -0,0 +1,140 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+// +build gofuzz
+
+package fuzz
+
+import (
+ "context"
+ "fmt"
+ "shared"
+ "strconv"
+ "tutorial"
+
+ "github.com/apache/thrift/lib/go/thrift"
+)
+
+const nbFuzzedProtocols = 2
+
+func fuzzChooseProtocol(d byte, t thrift.TTransport) thrift.TProtocol {
+ switch d % nbFuzzedProtocols {
+ case 0:
+ return thrift.NewTCompactProtocolFactoryConf(nil).GetProtocol(t)
+ }
+ return thrift.NewTBinaryProtocolFactoryConf(nil).GetProtocol(t)
Review comment:
ok
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] catenacyber commented on a change in pull request #2351: Adds fuzz target for oss-fuzz integration
Posted by GitBox <gi...@apache.org>.
catenacyber commented on a change in pull request #2351:
URL: https://github.com/apache/thrift/pull/2351#discussion_r599869807
##########
File path: lib/go/test/fuzz/fuzz_test.go
##########
@@ -0,0 +1,26 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+// +build gofuzz
+
+package fuzz
+
Review comment:
Thanks, we are making progress :-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] fishy commented on pull request #2351: Adds fuzz target for oss-fuzz integration
Posted by GitBox <gi...@apache.org>.
fishy commented on pull request #2351:
URL: https://github.com/apache/thrift/pull/2351#issuecomment-799690108
> @fishy should I squash the commits together ?
Yes please.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] fishy commented on a change in pull request #2351: Adds fuzz target for oss-fuzz integration
Posted by GitBox <gi...@apache.org>.
fishy commented on a change in pull request #2351:
URL: https://github.com/apache/thrift/pull/2351#discussion_r595255689
##########
File path: lib/go/test/Makefile.am
##########
@@ -85,6 +86,17 @@ gopath: $(THRIFT) $(THRIFTTEST) \
cp -r ./dontexportrwtest gopath/src
touch gopath
+gopathfuzz: $(THRIFT) fuzz/fuzz.go
+ cd fuzz && $(THRIFT) -r --gen go ../../../../tutorial/tutorial.thrift
+ cd fuzz && go mod init fuzz
+ cd fuzz/gen-go/shared && go mod init shared
+ cd fuzz/gen-go/tutorial && go mod init tutorial
+ cd fuzz && go mod edit -replace shared=./gen-go/shared
+ cd fuzz && go mod edit -replace tutorial=./gen-go/tutorial
+ cd fuzz && go mod edit -replace github.com/apache/thrift/lib/go/thrift=../../../../lib/go/thrift
+ go build fuzz
Review comment:
Also if we make this a rule in the fuzz directory instead of the test directory, we won't need to do `cd fuzz` in every command.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] catenacyber commented on a change in pull request #2351: Adds fuzz target for oss-fuzz integration
Posted by GitBox <gi...@apache.org>.
catenacyber commented on a change in pull request #2351:
URL: https://github.com/apache/thrift/pull/2351#discussion_r596634343
##########
File path: lib/go/test/Makefile.am
##########
@@ -85,6 +86,17 @@ gopath: $(THRIFT) $(THRIFTTEST) \
cp -r ./dontexportrwtest gopath/src
touch gopath
+gopathfuzz: $(THRIFT) fuzz/fuzz.go
+ cd fuzz && $(THRIFT) -r --gen go ../../../../tutorial/tutorial.thrift
+ cd fuzz && go mod init fuzz
+ cd fuzz/gen-go/shared && go mod init shared
+ cd fuzz/gen-go/tutorial && go mod init tutorial
+ cd fuzz && go mod edit -replace shared=./gen-go/shared
+ cd fuzz && go mod edit -replace tutorial=./gen-go/tutorial
+ cd fuzz && go mod edit -replace github.com/apache/thrift/lib/go/thrift=../../../../lib/go/thrift
+ go build fuzz
Review comment:
> does make return to your current directory after every command?
It seems so (I tried `pwd` and I saw I was not in `fuzz` subdirectory)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] catenacyber commented on a change in pull request #2351: Adds fuzz target for oss-fuzz integration
Posted by GitBox <gi...@apache.org>.
catenacyber commented on a change in pull request #2351:
URL: https://github.com/apache/thrift/pull/2351#discussion_r594617289
##########
File path: tutorial/go/fuzz/fuzz.go
##########
@@ -0,0 +1,144 @@
+package thriftfuzz
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import (
+ "context"
+ "fmt"
+ "shared"
+ "strconv"
+ "tutorial"
+
+ "github.com/apache/thrift/lib/go/thrift"
+)
+
+func fuzzChooseProtocol(d byte, t thrift.TTransport) thrift.TProtocol {
+ switch d {
+ case 0:
+ return thrift.NewTCompactProtocolFactory().GetProtocol(t)
+ }
+ return thrift.NewTBinaryProtocolFactoryDefault().GetProtocol(t)
+}
+
+func Fuzz(data []byte) int {
+ if len(data) < 2 {
+ return 0
+ }
+ inputTransport := thrift.NewTMemoryBuffer()
+ inputTransport.Buffer.Write(data[2:])
+ outputTransport := thrift.NewTMemoryBuffer()
+ outputProtocol := fuzzChooseProtocol(data[0], outputTransport)
+ inputProtocol := fuzzChooseProtocol(data[1], inputTransport)
+ ctx := thrift.SetResponseHelper(
+ context.Background(),
+ thrift.TResponseHelper{
+ THeaderResponseHelper: thrift.NewTHeaderResponseHelper(outputProtocol),
+ },
+ )
+ handler := NewCalculatorHandler()
+ processor := tutorial.NewCalculatorProcessor(handler)
+ ok := true
+ var err error
+ for ok {
+ ok, err = processor.Process(ctx, inputProtocol, outputProtocol)
+ if err != nil {
+ // Handle parse error
+ return 0
+ }
+res := make([]byte, 1024)
Review comment:
Running `go fmt` over it
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] catenacyber commented on a change in pull request #2351: Adds fuzz target for oss-fuzz integration
Posted by GitBox <gi...@apache.org>.
catenacyber commented on a change in pull request #2351:
URL: https://github.com/apache/thrift/pull/2351#discussion_r594618847
##########
File path: tutorial/go/fuzz/fuzz.go
##########
@@ -0,0 +1,144 @@
+package thriftfuzz
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import (
+ "context"
+ "fmt"
+ "shared"
+ "strconv"
+ "tutorial"
+
+ "github.com/apache/thrift/lib/go/thrift"
+)
+
+func fuzzChooseProtocol(d byte, t thrift.TTransport) thrift.TProtocol {
+ switch d {
+ case 0:
+ return thrift.NewTCompactProtocolFactory().GetProtocol(t)
+ }
+ return thrift.NewTBinaryProtocolFactoryDefault().GetProtocol(t)
Review comment:
ok
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] fishy commented on pull request #2351: Adds fuzz target for oss-fuzz integration
Posted by GitBox <gi...@apache.org>.
fishy commented on pull request #2351:
URL: https://github.com/apache/thrift/pull/2351#issuecomment-805331695
Oh nice travis passed.
@catenacyber If you squash your PR into a single commit I can merge it for you.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] catenacyber commented on pull request #2351: Adds fuzz target for oss-fuzz integration
Posted by GitBox <gi...@apache.org>.
catenacyber commented on pull request #2351:
URL: https://github.com/apache/thrift/pull/2351#issuecomment-803790583
> It adds the makefile to configure.ac
Just had a flash about the same thing...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] fishy merged pull request #2351: Adds fuzz target for oss-fuzz integration
Posted by GitBox <gi...@apache.org>.
fishy merged pull request #2351:
URL: https://github.com/apache/thrift/pull/2351
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] catenacyber commented on a change in pull request #2351: Adds fuzz target for oss-fuzz integration
Posted by GitBox <gi...@apache.org>.
catenacyber commented on a change in pull request #2351:
URL: https://github.com/apache/thrift/pull/2351#discussion_r594620682
##########
File path: tutorial/go/fuzz/fuzz.go
##########
@@ -0,0 +1,144 @@
+package thriftfuzz
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import (
+ "context"
+ "fmt"
+ "shared"
+ "strconv"
+ "tutorial"
+
+ "github.com/apache/thrift/lib/go/thrift"
+)
+
+func fuzzChooseProtocol(d byte, t thrift.TTransport) thrift.TProtocol {
+ switch d {
+ case 0:
Review comment:
> if this is truly a random byte
It is not when using libfuzzer
And there are other protocols which should take the values 1,2,3,4
So, I add it with `const nbFuzzedProtocols = 2`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] fishy commented on pull request #2351: Adds fuzz target for oss-fuzz integration
Posted by GitBox <gi...@apache.org>.
fishy commented on pull request #2351:
URL: https://github.com/apache/thrift/pull/2351#issuecomment-802979450
The go code/test part looks good to me. The automake part certainly need more work but I'm not familiar with that part. I asked in the maillist for help. :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] fishy commented on pull request #2351: Adds fuzz target for oss-fuzz integration
Posted by GitBox <gi...@apache.org>.
fishy commented on pull request #2351:
URL: https://github.com/apache/thrift/pull/2351#issuecomment-804412022
Looks like we made pretty good progress.
The current failure is caused by:
```
go: fuzz imports
github.com/apache/thrift/lib/go/thrift: parsing ../../thrift/go.mod: open /thrift/src/lib/go/thrift/go.mod: no such file or directory
```
I'm working on adding `go.mod` to the root directory, which would resolve that problem, but at the mean time you can just do `go mod init` either at the root or `lib/go/thrift` during the build process to walk around it 😅
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] catenacyber commented on pull request #2351: Adds fuzz target for oss-fuzz integration
Posted by GitBox <gi...@apache.org>.
catenacyber commented on pull request #2351:
URL: https://github.com/apache/thrift/pull/2351#issuecomment-803646508
Thanks for trying another commit @Jens-G
@fishy did you get answers on the mailing list ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] fishy commented on pull request #2351: Adds fuzz target for oss-fuzz integration
Posted by GitBox <gi...@apache.org>.
fishy commented on pull request #2351:
URL: https://github.com/apache/thrift/pull/2351#issuecomment-803517988
Travis still has several build failed with:
```
Making all in test
make[4]: Entering directory '/thrift/src/lib/go/test'
Making all in fuzz
make[5]: Entering directory '/thrift/src/lib/go/test/fuzz'
make[5]: *** No rule to make target 'all'. Stop.
make[5]: Leaving directory '/thrift/src/lib/go/test/fuzz'
Makefile:564: recipe for target 'all-recursive' failed
make[4]: *** [all-recursive] Error 1
make[4]: Leaving directory '/thrift/src/lib/go/test'
```
So I guess we don't have the `all` target under `fuzz` generated correctly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] fishy commented on a change in pull request #2351: Adds fuzz target for oss-fuzz integration
Posted by GitBox <gi...@apache.org>.
fishy commented on a change in pull request #2351:
URL: https://github.com/apache/thrift/pull/2351#discussion_r599726482
##########
File path: lib/go/test/fuzz/fuzz_test.go
##########
@@ -0,0 +1,26 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+// +build gofuzz
+
+package fuzz
+
Review comment:
you forgot to `import ( "testing" )` in this file.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] fishy commented on pull request #2351: Adds fuzz target for oss-fuzz integration
Posted by GitBox <gi...@apache.org>.
fishy commented on pull request #2351:
URL: https://github.com/apache/thrift/pull/2351#issuecomment-803653901
I believe Jens is the answer I got :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] catenacyber commented on a change in pull request #2351: Adds fuzz target for oss-fuzz integration
Posted by GitBox <gi...@apache.org>.
catenacyber commented on a change in pull request #2351:
URL: https://github.com/apache/thrift/pull/2351#discussion_r594617808
##########
File path: tutorial/go/fuzz/fuzz.go
##########
@@ -0,0 +1,144 @@
+package thriftfuzz
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
Review comment:
ok
##########
File path: tutorial/go/fuzz/fuzz.go
##########
@@ -0,0 +1,144 @@
+package thriftfuzz
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import (
+ "context"
+ "fmt"
+ "shared"
+ "strconv"
+ "tutorial"
+
+ "github.com/apache/thrift/lib/go/thrift"
+)
+
+func fuzzChooseProtocol(d byte, t thrift.TTransport) thrift.TProtocol {
+ switch d {
+ case 0:
+ return thrift.NewTCompactProtocolFactory().GetProtocol(t)
+ }
+ return thrift.NewTBinaryProtocolFactoryDefault().GetProtocol(t)
+}
+
+func Fuzz(data []byte) int {
+ if len(data) < 2 {
+ return 0
+ }
+ inputTransport := thrift.NewTMemoryBuffer()
+ inputTransport.Buffer.Write(data[2:])
+ outputTransport := thrift.NewTMemoryBuffer()
+ outputProtocol := fuzzChooseProtocol(data[0], outputTransport)
+ inputProtocol := fuzzChooseProtocol(data[1], inputTransport)
+ ctx := thrift.SetResponseHelper(
+ context.Background(),
+ thrift.TResponseHelper{
+ THeaderResponseHelper: thrift.NewTHeaderResponseHelper(outputProtocol),
+ },
+ )
+ handler := NewCalculatorHandler()
+ processor := tutorial.NewCalculatorProcessor(handler)
+ ok := true
+ var err error
+ for ok {
+ ok, err = processor.Process(ctx, inputProtocol, outputProtocol)
+ if err != nil {
+ // Handle parse error
+ return 0
+ }
+res := make([]byte, 1024)
+n, err := outputTransport.Buffer.Read(res)
+fmt.Printf("lol %d %s %v\n", n, err, res)
+ }
+ return 1
+}
+
+type CalculatorHandler struct {
+ log map[int]*shared.SharedStruct
+}
+
+func NewCalculatorHandler() *CalculatorHandler {
+ return &CalculatorHandler{log: make(map[int]*shared.SharedStruct)}
+}
+
+func (p *CalculatorHandler) Ping(ctx context.Context) (err error) {
+ fmt.Print("ping()\n")
+ return nil
+}
+
+func (p *CalculatorHandler) Add(ctx context.Context, num1 int32, num2 int32) (retval17 int32, err error) {
+ fmt.Print("add(", num1, ",", num2, ")\n")
+ return num1 + num2, nil
+}
+
+func (p *CalculatorHandler) Calculate(ctx context.Context, logid int32, w *tutorial.Work) (val int32, err error) {
+ fmt.Print("calculate(", logid, ", {", w.Op, ",", w.Num1, ",", w.Num2, "})\n")
+ switch w.Op {
+ case tutorial.Operation_ADD:
+ val = w.Num1 + w.Num2
+ break
+ case tutorial.Operation_SUBTRACT:
+ val = w.Num1 - w.Num2
+ break
+ case tutorial.Operation_MULTIPLY:
+ val = w.Num1 * w.Num2
+ break
+ case tutorial.Operation_DIVIDE:
+ if w.Num2 == 0 {
+ ouch := tutorial.NewInvalidOperation()
+ ouch.WhatOp = int32(w.Op)
+ ouch.Why = "Cannot divide by 0"
+ err = ouch
+ return
+ }
+ val = w.Num1 / w.Num2
+ break
+ default:
+ ouch := tutorial.NewInvalidOperation()
+ ouch.WhatOp = int32(w.Op)
+ ouch.Why = "Unknown operation"
+ err = ouch
+ return
+ }
+ entry := shared.NewSharedStruct()
+ entry.Key = logid
+ entry.Value = strconv.Itoa(int(val))
+ k := int(logid)
+ /*
+ oldvalue, exists := p.log[k]
+ if exists {
+ fmt.Print("Replacing ", oldvalue, " with ", entry, " for key ", k, "\n")
+ } else {
+ fmt.Print("Adding ", entry, " for key ", k, "\n")
+ }
+ */
Review comment:
Removing
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] catenacyber commented on pull request #2351: Adds fuzz target for oss-fuzz integration
Posted by GitBox <gi...@apache.org>.
catenacyber commented on pull request #2351:
URL: https://github.com/apache/thrift/pull/2351#issuecomment-804691267
Trying something.
Let me know in which order you want the PRs merged, and I can rebase id needed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] catenacyber commented on pull request #2351: Adds fuzz target for oss-fuzz integration
Posted by GitBox <gi...@apache.org>.
catenacyber commented on pull request #2351:
URL: https://github.com/apache/thrift/pull/2351#issuecomment-804360690
> I got some "malformed module path"
I suspect this is due to golang version
I try to set env `GO111MODULE=on` so that we have the same behavior as with go 1.16
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] Jens-G edited a comment on pull request #2351: Adds fuzz target for oss-fuzz integration
Posted by GitBox <gi...@apache.org>.
Jens-G edited a comment on pull request #2351:
URL: https://github.com/apache/thrift/pull/2351#issuecomment-803696553
Could you pull one commit from my private repo ossfuzz branch? It adds the makefile to configure.ac, after that I got some "malformed module path" at go mod on "make check"
https://github.com/Jens-G/thrift/tree/ossfuzz
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] catenacyber commented on pull request #2351: Adds fuzz target for oss-fuzz integration
Posted by GitBox <gi...@apache.org>.
catenacyber commented on pull request #2351:
URL: https://github.com/apache/thrift/pull/2351#issuecomment-799721751
> how is this code being used by libfuzz, exactly?
cf https://github.com/google/oss-fuzz/pull/5264/files#diff-c54441e86b07048406afb5c7ed023b873669ff90d6b81613eedaaf4b90b7eb21R28
I think the trick you are looking for is `go mod edit -replace tutorial=./gen-go/tutorial`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] catenacyber commented on a change in pull request #2351: Adds fuzz target for oss-fuzz integration
Posted by GitBox <gi...@apache.org>.
catenacyber commented on a change in pull request #2351:
URL: https://github.com/apache/thrift/pull/2351#discussion_r594617173
##########
File path: tutorial/go/fuzz/fuzz.go
##########
@@ -0,0 +1,144 @@
+package thriftfuzz
Review comment:
> Is there a reason this package name is different from the directory name?
Not really... Just wanted the name to be self-explaining
> Is there a reason this is under tutorial instead of, say, lib/go/test?
I was not sure where to put it.
Since it uses the tutorial, I put it close to it.
but `lib/go/test` seems a better choice. Moving there
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] fishy commented on a change in pull request #2351: Adds fuzz target for oss-fuzz integration
Posted by GitBox <gi...@apache.org>.
fishy commented on a change in pull request #2351:
URL: https://github.com/apache/thrift/pull/2351#discussion_r594485395
##########
File path: tutorial/go/fuzz/fuzz.go
##########
@@ -0,0 +1,144 @@
+package thriftfuzz
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
Review comment:
I think the license header is supposed to be before `package` declaration?
##########
File path: tutorial/go/fuzz/fuzz.go
##########
@@ -0,0 +1,144 @@
+package thriftfuzz
Review comment:
1. Is there a reason this package name is different from the directory name? I understand that this is not really some library code others would import, but this still seem unnecessary.
2. Is there a reason this is under tutorial instead of, say, `lib/go/test`?
##########
File path: tutorial/go/fuzz/fuzz.go
##########
@@ -0,0 +1,144 @@
+package thriftfuzz
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import (
+ "context"
+ "fmt"
+ "shared"
+ "strconv"
+ "tutorial"
+
+ "github.com/apache/thrift/lib/go/thrift"
+)
+
+func fuzzChooseProtocol(d byte, t thrift.TTransport) thrift.TProtocol {
+ switch d {
+ case 0:
+ return thrift.NewTCompactProtocolFactory().GetProtocol(t)
+ }
+ return thrift.NewTBinaryProtocolFactoryDefault().GetProtocol(t)
+}
+
+func Fuzz(data []byte) int {
+ if len(data) < 2 {
+ return 0
+ }
+ inputTransport := thrift.NewTMemoryBuffer()
+ inputTransport.Buffer.Write(data[2:])
+ outputTransport := thrift.NewTMemoryBuffer()
+ outputProtocol := fuzzChooseProtocol(data[0], outputTransport)
+ inputProtocol := fuzzChooseProtocol(data[1], inputTransport)
+ ctx := thrift.SetResponseHelper(
+ context.Background(),
+ thrift.TResponseHelper{
+ THeaderResponseHelper: thrift.NewTHeaderResponseHelper(outputProtocol),
+ },
+ )
+ handler := NewCalculatorHandler()
+ processor := tutorial.NewCalculatorProcessor(handler)
+ ok := true
+ var err error
+ for ok {
+ ok, err = processor.Process(ctx, inputProtocol, outputProtocol)
+ if err != nil {
+ // Handle parse error
+ return 0
+ }
+res := make([]byte, 1024)
+n, err := outputTransport.Buffer.Read(res)
+fmt.Printf("lol %d %s %v\n", n, err, res)
+ }
+ return 1
+}
+
+type CalculatorHandler struct {
+ log map[int]*shared.SharedStruct
+}
+
+func NewCalculatorHandler() *CalculatorHandler {
+ return &CalculatorHandler{log: make(map[int]*shared.SharedStruct)}
+}
+
+func (p *CalculatorHandler) Ping(ctx context.Context) (err error) {
+ fmt.Print("ping()\n")
+ return nil
+}
+
+func (p *CalculatorHandler) Add(ctx context.Context, num1 int32, num2 int32) (retval17 int32, err error) {
+ fmt.Print("add(", num1, ",", num2, ")\n")
+ return num1 + num2, nil
+}
+
+func (p *CalculatorHandler) Calculate(ctx context.Context, logid int32, w *tutorial.Work) (val int32, err error) {
+ fmt.Print("calculate(", logid, ", {", w.Op, ",", w.Num1, ",", w.Num2, "})\n")
+ switch w.Op {
+ case tutorial.Operation_ADD:
+ val = w.Num1 + w.Num2
+ break
+ case tutorial.Operation_SUBTRACT:
+ val = w.Num1 - w.Num2
+ break
+ case tutorial.Operation_MULTIPLY:
+ val = w.Num1 * w.Num2
+ break
+ case tutorial.Operation_DIVIDE:
+ if w.Num2 == 0 {
+ ouch := tutorial.NewInvalidOperation()
+ ouch.WhatOp = int32(w.Op)
+ ouch.Why = "Cannot divide by 0"
+ err = ouch
+ return
+ }
+ val = w.Num1 / w.Num2
+ break
+ default:
+ ouch := tutorial.NewInvalidOperation()
+ ouch.WhatOp = int32(w.Op)
+ ouch.Why = "Unknown operation"
+ err = ouch
+ return
+ }
+ entry := shared.NewSharedStruct()
+ entry.Key = logid
+ entry.Value = strconv.Itoa(int(val))
+ k := int(logid)
+ /*
+ oldvalue, exists := p.log[k]
+ if exists {
+ fmt.Print("Replacing ", oldvalue, " with ", entry, " for key ", k, "\n")
+ } else {
+ fmt.Print("Adding ", entry, " for key ", k, "\n")
+ }
+ */
Review comment:
is this block of commented out code from some copy-paste?
##########
File path: tutorial/go/fuzz/fuzz.go
##########
@@ -0,0 +1,144 @@
+package thriftfuzz
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import (
+ "context"
+ "fmt"
+ "shared"
+ "strconv"
+ "tutorial"
+
+ "github.com/apache/thrift/lib/go/thrift"
+)
+
+func fuzzChooseProtocol(d byte, t thrift.TTransport) thrift.TProtocol {
+ switch d {
+ case 0:
Review comment:
if this is truly a random byte, instead of just using `byte` to hold 1/0 value, then we would have 255/256 chance of using binary protocol and 1/256 chance of using compact protocol. should this be something like `d % 2 == 0` instead?
##########
File path: tutorial/go/fuzz/fuzz.go
##########
@@ -0,0 +1,144 @@
+package thriftfuzz
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import (
+ "context"
+ "fmt"
+ "shared"
+ "strconv"
+ "tutorial"
+
+ "github.com/apache/thrift/lib/go/thrift"
+)
+
+func fuzzChooseProtocol(d byte, t thrift.TTransport) thrift.TProtocol {
+ switch d {
+ case 0:
+ return thrift.NewTCompactProtocolFactory().GetProtocol(t)
+ }
+ return thrift.NewTBinaryProtocolFactoryDefault().GetProtocol(t)
+}
+
+func Fuzz(data []byte) int {
+ if len(data) < 2 {
+ return 0
+ }
+ inputTransport := thrift.NewTMemoryBuffer()
+ inputTransport.Buffer.Write(data[2:])
+ outputTransport := thrift.NewTMemoryBuffer()
+ outputProtocol := fuzzChooseProtocol(data[0], outputTransport)
+ inputProtocol := fuzzChooseProtocol(data[1], inputTransport)
+ ctx := thrift.SetResponseHelper(
+ context.Background(),
+ thrift.TResponseHelper{
+ THeaderResponseHelper: thrift.NewTHeaderResponseHelper(outputProtocol),
+ },
+ )
+ handler := NewCalculatorHandler()
+ processor := tutorial.NewCalculatorProcessor(handler)
+ ok := true
+ var err error
+ for ok {
+ ok, err = processor.Process(ctx, inputProtocol, outputProtocol)
+ if err != nil {
+ // Handle parse error
+ return 0
+ }
+res := make([]byte, 1024)
Review comment:
wrong indentation. is this code auto generated by some tool?
##########
File path: tutorial/go/fuzz/fuzz.go
##########
@@ -0,0 +1,144 @@
+package thriftfuzz
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import (
+ "context"
+ "fmt"
+ "shared"
+ "strconv"
+ "tutorial"
+
+ "github.com/apache/thrift/lib/go/thrift"
+)
+
+func fuzzChooseProtocol(d byte, t thrift.TTransport) thrift.TProtocol {
+ switch d {
+ case 0:
+ return thrift.NewTCompactProtocolFactory().GetProtocol(t)
+ }
+ return thrift.NewTBinaryProtocolFactoryDefault().GetProtocol(t)
Review comment:
nit: those factory constructors are deprecated, please use `NewTCompactProtocolFactoryConf(nil)` and `NewTBinaryProtocolFactoryConf(nil)` instead.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] Jens-G commented on pull request #2351: Adds fuzz target for oss-fuzz integration
Posted by GitBox <gi...@apache.org>.
Jens-G commented on pull request #2351:
URL: https://github.com/apache/thrift/pull/2351#issuecomment-803696553
Could you pull one commit from my private repo ossfuzz branch? It adds the makefile to configure.ac, after that I got some "malformed module path" at go mod on "make check"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] fishy commented on pull request #2351: Adds fuzz target for oss-fuzz integration
Posted by GitBox <gi...@apache.org>.
fishy commented on pull request #2351:
URL: https://github.com/apache/thrift/pull/2351#issuecomment-804436945
> Looks like we made pretty good progress.
>
> The current failure is caused by:
>
> ```
> go: fuzz imports
> github.com/apache/thrift/lib/go/thrift: parsing ../../thrift/go.mod: open /thrift/src/lib/go/thrift/go.mod: no such file or directory
> ```
>
> I'm working on adding `go.mod` to the root directory, which would resolve that problem, but at the mean time you can just do `go mod init` either at the root or `lib/go/thrift` during the build process to walk around it 😅
https://github.com/apache/thrift/pull/2353
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] fishy commented on pull request #2351: Adds fuzz target for oss-fuzz integration
Posted by GitBox <gi...@apache.org>.
fishy commented on pull request #2351:
URL: https://github.com/apache/thrift/pull/2351#issuecomment-799698727
how is this code being used by libfuzz, exactly? if libfuzz imports this code as a third party go import, then the thrift compiled `tutorial` package also need to be imported by it (and the import path of `tutorial` likely won't work).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] catenacyber commented on pull request #2351: Adds fuzz target for oss-fuzz integration
Posted by GitBox <gi...@apache.org>.
catenacyber commented on pull request #2351:
URL: https://github.com/apache/thrift/pull/2351#issuecomment-801884628
@fishy do you know what is missing in my Makefile.am ?
It seems target `all` is empty, but there is nothing to do if we do not build the tests...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] Jens-G edited a comment on pull request #2351: Adds fuzz target for oss-fuzz integration
Posted by GitBox <gi...@apache.org>.
Jens-G edited a comment on pull request #2351:
URL: https://github.com/apache/thrift/pull/2351#issuecomment-803696553
Could you pull one commit from my private repo ossfuzz branch? It adds the makefile to configure.ac, after that I got some "malformed module path" at go mod on "make check"
https://github.com/Jens-G/thrift/tree/ossfuzz
https://github.com/Jens-G/thrift/commit/e80f8d379afeeb7dcc1bcc7b9b3a02ceb8c90a28
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] catenacyber commented on a change in pull request #2351: Adds fuzz target for oss-fuzz integration
Posted by GitBox <gi...@apache.org>.
catenacyber commented on a change in pull request #2351:
URL: https://github.com/apache/thrift/pull/2351#discussion_r596637973
##########
File path: lib/go/test/Makefile.am
##########
@@ -85,6 +86,17 @@ gopath: $(THRIFT) $(THRIFTTEST) \
cp -r ./dontexportrwtest gopath/src
touch gopath
+gopathfuzz: $(THRIFT) fuzz/fuzz.go
+ cd fuzz && $(THRIFT) -r --gen go ../../../../tutorial/tutorial.thrift
+ cd fuzz && go mod init fuzz
+ cd fuzz/gen-go/shared && go mod init shared
+ cd fuzz/gen-go/tutorial && go mod init tutorial
+ cd fuzz && go mod edit -replace shared=./gen-go/shared
+ cd fuzz && go mod edit -replace tutorial=./gen-go/tutorial
+ cd fuzz && go mod edit -replace github.com/apache/thrift/lib/go/thrift=../../../../lib/go/thrift
+ go build fuzz
Review comment:
> Also if we make this a rule in the fuzz directory instead of the test directory
Trying this
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] catenacyber commented on pull request #2351: Adds fuzz target for oss-fuzz integration
Posted by GitBox <gi...@apache.org>.
catenacyber commented on pull request #2351:
URL: https://github.com/apache/thrift/pull/2351#issuecomment-800045983
> Can you add the newly added `lib/go/test/thriftfuzz` directory to our travis CI system (e.g. add it to
>
> https://github.com/apache/thrift/blob/62beb6751d3c70f8db8fed4a3bb76e4ff3765c22/lib/go/test/Makefile.am#L88
>
> rule)? Currently it's not checked by our CI system, so someone could accidentally break it (for example, make changes to `tutorial.thrift`) without realizing it.
> This this is actually a library, I guess you would need to write a test to actually use this library and fill in some fixed bytes. We can't really fuzz it in our travis CI, but we can at least make sure that it still compiles.
So, should I use tutorial.thrift even if the fuzz target is in lib/go/test ?
I am trying some commit against the CI
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org