Adds thrift project #5264

catenacyber · 2021-02-28T10:00:45Z

@Jens-G would you be interested in continuous fuzzing for thrift ?
This Pull Request is an initial integration with oss-fuzz with only a a fuzz target against a go server (the tutorial.thrift).

It quickly finds some timeouts.
A quick profiling shows that this timeout is due to a lack of error checking (see patch below)

I think that thrift would be especially fit for differential fuzzing to test that we always get the same answer for one request whatever the implementation of the server...

diff --git a/lib/go/thrift/protocol.go b/lib/go/thrift/protocol.go
index 0a69bd4..4768c8f 100644
--- a/lib/go/thrift/protocol.go
+++ b/lib/go/thrift/protocol.go
@@ -122,11 +122,14 @@ func Skip(ctx context.Context, self TProtocol, fieldType TType, maxDepth int) (e
                        return err
                }
                for {
-                       _, typeId, _, _ := self.ReadFieldBegin(ctx)
+                       _, typeId, _, err := self.ReadFieldBegin(ctx)
+                       if err != nil {
+                               return err
+                       }
                        if typeId == STOP {
                                break
                        }
-                       err := Skip(ctx, self, typeId, maxDepth-1)
+                       err = Skip(ctx, self, typeId, maxDepth-1)
                        if err != nil {
                                return err
                        }

(goes down from 14 seconds to 200 ms to process one input)

catenacyber · 2021-02-28T10:02:22Z

projects/thrift/project.yaml

@@ -0,0 +1,11 @@
+homepage: "https://thrift.apache.org/"
+language: c++


@jonathanmetzman
Thrift has 28 languages
I do not know if there should be some settings for these projects (ecc-diff-fuzzer is another one)

How much code is there actually to fuzz in each language?
I assume the core code is written in one language and there are simply wrappers for other languages.
Maybe put the language of the one you are actually fuzzing.
If the core code is C++ why fuzz the go wrappers?

I assume the core code is written in one language and there are simply wrappers for other languages.

This is not the case for thrift, there are no wrappers.
Thrift compiles some schema into one of its 28 languages, and you get a RPC client/server only in this language (ie there is no C++ code run from golang), like 28 implementations of the same protocol(s)
So, there is about as much to fuzz in each language.

(There may also be the schema compiler, but it is itself rather a C++ parser around flex/bison )

OK I misunderstood.
I've filed #5278 to discuss this.
What languages are we fuzzing for now? Just go?

This draft PR has only golang indeed

Jens-G · 2021-02-28T23:38:14Z

to a lack of error checking (see patch below)

How is that possible, when Go has such an sophisticated error handling concept <irony/>?
Ok, jokes aside: Thanks for the patch but please put it into a separate PR.

Jens-G · 2021-02-28T23:39:35Z

clang-12: error: linker command failed with exit code 1 (use -v to see invocation)

Jens-G · 2021-02-28T23:40:16Z

Waht does "cla/google — All necessary CLAs are signed" mean/imply?

Jens-G · 2021-02-28T23:32:52Z

projects/thrift/fuzz.go

@@ -0,0 +1,125 @@
+package thriftfuzz


ASF header missing

Jens-G · 2021-02-28T23:33:46Z

projects/thrift/Dockerfile

@@ -0,0 +1,22 @@
+# Copyright 2021 Google LLC
+#


https://www.apache.org/legal/src-headers.html

@inferno-chromium could you help me with the legal side cf https://google.github.io/oss-fuzz/getting-started/new-project-guide/#copyright-headers ?

Jens-G · 2021-02-28T23:33:54Z

projects/thrift/build.sh

@@ -0,0 +1,38 @@
+#!/bin/bash -eu
+# Copyright 2021 Google LLC
+#


https://www.apache.org/legal/src-headers.html

Jens-G · 2021-02-28T23:34:06Z

projects/thrift/project.yaml

@@ -0,0 +1,11 @@
+homepage: "https://thrift.apache.org/"


ASF header missing

Jens-G · 2021-02-28T23:34:41Z

projects/thrift/project.yaml

+homepage: "https://thrift.apache.org/"
+language: c++
+primary_contact: "[email protected]"
+auto_ccs :


https://thrift.apache.org/mailing

Jens-G · 2021-02-28T23:44:51Z

would you be interested in continuous fuzzing for thrift ?

No idea. Who is going to look at the results on a regular basis?

catenacyber · 2021-03-01T11:04:36Z

would you be interested in continuous fuzzing for thrift ?
No idea. Who is going to look at the results on a regular basis?

The mails listed in project.yaml get a notification when a new bug is found
For the moment, I just put yourself and me.
Who would you like ?
The public mailing lists are not recommended for this case

catenacyber · 2021-03-01T11:05:38Z

Thanks for the patch but please put it into a separate PR.

Ok, I will.
Would you also like to host the fuzz target (ie the file fuzz.go) in the thrift upstream repository ?

catenacyber · 2021-03-01T11:07:07Z

Waht does "cla/google — All necessary CLAs are signed" mean/imply?

It means I signed the Google CLA (so I can contribute to oss-fuzz)

Jens-G · 2021-03-01T21:50:14Z

Oopsie. I somehow was under the impression that this is a PR for Thrift. You may ignore my ASF header comments then.

catenacyber · 2021-03-25T22:12:56Z

@fishy the CI should fail with rust warnings like

error: panic message is not a string literal
   --> src/protocol/compact.rs:641:21
    |
641 |           _ => panic!(format!(
    |  _____________________^
642 | |             "should not have attempted to convert {} to u8",
643 | |             field_type
644 | |         )),
    | |_________^
    |
    = note: `-D non-fmt-panic` implied by `-D warnings`
    = note: this is no longer accepted in Rust 2021
    = note: the panic!() macro supports formatting, so there's no need for the format!() macro here
help: remove the `format!(..)` macro call
    |
641 |         _ => panic!(
642 |             "should not have attempted to convert {} to u8",
643 |             field_type
644 |         ),
    |

Do you know if it is something that will be fixed ?
Workaround is to disable rust at configure time I guess

fishy · 2021-03-25T22:27:20Z

@fishy the CI should fail with rust warnings like

error: panic message is not a string literal
   --> src/protocol/compact.rs:641:21
    |
641 |           _ => panic!(format!(
    |  _____________________^
642 | |             "should not have attempted to convert {} to u8",
643 | |             field_type
644 | |         )),
    | |_________^
    |
    = note: `-D non-fmt-panic` implied by `-D warnings`
    = note: this is no longer accepted in Rust 2021
    = note: the panic!() macro supports formatting, so there's no need for the format!() macro here
help: remove the `format!(..)` macro call
    |
641 |         _ => panic!(
642 |             "should not have attempted to convert {} to u8",
643 |             field_type
644 |         ),
    |

Do you know if it is something that will be fixed ?
Workaround is to disable rust at configure time I guess

Sorry I have no idea. I'm not familiar with the thrift rust library.

catenacyber · 2021-03-26T08:54:19Z

cc @allengeorge

Do you know about these rust errors (clip warnings from nightly)
cf https://github.com/google/oss-fuzz/runs/2197379168?check_suite_focus=true

catenacyber · 2021-03-26T09:15:49Z

Still requires apache/thrift#2358 to be merged

allengeorge · 2021-03-26T14:49:37Z

@catenacyber I didn't think we test using nightly Rust (the latest supported is 1.41). Why are we doing so?

From quick googling online, the panic warnings showed up only on 1.51 onwards (and in the Rust 2021 edition!)

allengeorge · 2021-03-26T14:52:50Z

Fixing the panics to not use the format macros should be trivial. I'll look at the other error, but I can't imagine it's super hard to fix.

catenacyber · 2021-03-26T14:54:33Z

Why are we doing so?

I tried to compile thrift, while having rust nightly, and got these errors
So, I just wanted to report them to you.
Does that answer your question ?
Are you interested in these errors ?

allengeorge · 2021-03-26T14:58:55Z

@catenacyber I think I've been unclear: I appreciate that you're reporting these errors, but I'm simply asking:

why you're using Rust nightly, and,
is this something you're planning on doing continually with Rust nightly going forward

I'm asking because the Thrift build does not test with nightly, so you're more likely to find unexpected errors. It may be worthwhile to pin your Rust version to 1.41 while fuzzing Thrift because that's what we regularly test and support.

And to answer your second question: yes, I will fix these clippy lints.

catenacyber · 2021-03-26T15:04:23Z

why you're using Rust nightly

I started to use it because I needed some -Z flags
For the Suricata project, I think we are testing with both stable and nightly
Nightly has been finding some new helpful warnings from time to time.

catenacyber · 2021-03-28T16:40:00Z

@jonathanmetzman can we disable coverage build for this project for now ? cf #5278
So that CI passes

jonathanmetzman · 2021-03-31T14:31:31Z

@jonathanmetzman can we disable coverage build for this project for now ? cf #5278
So that CI passes

I don't know if we generally do that. But I can merge this.

jonathanmetzman

LGTM

catenacyber · 2021-03-31T14:55:41Z

Ok thanks

fishy · 2022-10-07T20:21:29Z

@catenacyber does oss-fuzz support go's native fuzz support added in go 1.18? now that thrift's go library is on go 1.18+, I think it would make sense to switch the fuzz test code to the native one if possible.

catenacyber · 2022-10-08T06:25:25Z

I think #7519 tracks this

catenacyber commented Feb 28, 2021

View reviewed changes

Jens-G reviewed Feb 28, 2021

View reviewed changes

jonathanmetzman mentioned this pull request Mar 3, 2021

Handle multi-language projects #5278

Open

This was referenced Mar 15, 2021

Early error check in golang struct reading apache/thrift#2350

Merged

Adds fuzz target for oss-fuzz integration apache/thrift#2351

Merged

catenacyber mentioned this pull request Mar 26, 2021

Fix lib/go/test/fuzz for oss-fuzz project apache/thrift#2358

Merged

Adds thrift project

327e6bb

catenacyber force-pushed the thrift branch from 88f3133 to 327e6bb Compare March 26, 2021 15:02

catenacyber marked this pull request as ready for review March 26, 2021 15:02

jonathanmetzman approved these changes Mar 31, 2021

View reviewed changes

jonathanmetzman merged commit 73ebaee into google:master Mar 31, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds thrift project #5264

Adds thrift project #5264

catenacyber commented Feb 28, 2021

catenacyber Feb 28, 2021

jonathanmetzman Feb 28, 2021

catenacyber Feb 28, 2021

jonathanmetzman Mar 3, 2021

catenacyber Mar 3, 2021

Jens-G commented Feb 28, 2021 •

edited

Loading

Jens-G commented Feb 28, 2021

Jens-G commented Feb 28, 2021

Jens-G Feb 28, 2021

Jens-G Feb 28, 2021

catenacyber Mar 1, 2021

Jens-G Feb 28, 2021

Jens-G Feb 28, 2021

Jens-G Feb 28, 2021

Jens-G commented Feb 28, 2021

catenacyber commented Mar 1, 2021

catenacyber commented Mar 1, 2021

catenacyber commented Mar 1, 2021

Jens-G commented Mar 1, 2021

catenacyber commented Mar 25, 2021

fishy commented Mar 25, 2021

catenacyber commented Mar 26, 2021

catenacyber commented Mar 26, 2021

allengeorge commented Mar 26, 2021 •

edited

Loading

allengeorge commented Mar 26, 2021

catenacyber commented Mar 26, 2021

allengeorge commented Mar 26, 2021

catenacyber commented Mar 26, 2021

catenacyber commented Mar 28, 2021

jonathanmetzman commented Mar 31, 2021

jonathanmetzman left a comment

catenacyber commented Mar 31, 2021

fishy commented Oct 7, 2022

catenacyber commented Oct 8, 2022

		@@ -0,0 +1,11 @@
		homepage: "https://thrift.apache.org/"
		language: c++

Adds thrift project #5264

Adds thrift project #5264

Conversation

catenacyber commented Feb 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jens-G commented Feb 28, 2021 • edited Loading

Jens-G commented Feb 28, 2021

Jens-G commented Feb 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jens-G commented Feb 28, 2021

catenacyber commented Mar 1, 2021

catenacyber commented Mar 1, 2021

catenacyber commented Mar 1, 2021

Jens-G commented Mar 1, 2021

catenacyber commented Mar 25, 2021

fishy commented Mar 25, 2021

catenacyber commented Mar 26, 2021

catenacyber commented Mar 26, 2021

allengeorge commented Mar 26, 2021 • edited Loading

allengeorge commented Mar 26, 2021

catenacyber commented Mar 26, 2021

allengeorge commented Mar 26, 2021

catenacyber commented Mar 26, 2021

catenacyber commented Mar 28, 2021

jonathanmetzman commented Mar 31, 2021

jonathanmetzman left a comment

Choose a reason for hiding this comment

catenacyber commented Mar 31, 2021

fishy commented Oct 7, 2022

catenacyber commented Oct 8, 2022

Jens-G commented Feb 28, 2021 •

edited

Loading

allengeorge commented Mar 26, 2021 •

edited

Loading