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

Fuzzing: Add new fuzzer #2848

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

AdamKorcz
Copy link
Contributor

This PR adds a fuzzer that creates a container with a randomized configuration and then calls State().

Furthermore a seed and a dictionary is added to help the fuzzer mutate relevant input data.

Lastly the OSS-fuzz build file has been updated to include this new fuzzer.

@AdamKorcz AdamKorcz force-pushed the fuzz2 branch 2 times, most recently from 497925a to 7115446 Compare March 11, 2021 14:54
@AdamKorcz
Copy link
Contributor Author

Failing build looks unrelated

Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

The fuzzer should be ensured to run FuzzStateApi() only with a valid JSON bytes. I'm not sure whether it is possible with go-fuzz, but without ensuring that, fuzzing is not really meaningful. (Assuming that json.Unmarshal is stable enough and not worth fuzzing.)

@AdamKorcz
Copy link
Contributor Author

AdamKorcz commented Mar 12, 2021

The fuzzer should be ensured to run FuzzStateApi() only with a valid JSON bytes.

You are right, and the fuzzer will take care of that. It will eventually start creating json to get a valid container.

The way the fuzzer creates the container allows it to be validated:

l := &LinuxFactory{
Root: root,
InitPath: "/proc/self/exe",
InitArgs: []string{os.Args[0], "init"},
Validator: validate.New(),
CriuPath: "criu",
}

... and if the fuzzer does not generate a valid container, it will return and try over:

container, err := newContainer(root, config)
if err != nil {
	return 0
}

State() is therefore not called on invalid containers.

(Assuming that json.Unmarshal is stable enough and not worth fuzzing.)

json.Unmarshal is being fuzzed elsewhere continuously.

@AdamKorcz
Copy link
Contributor Author

@AkihiroSuda Can you check what I wrote? It would be helpful to get this up and running on OSS-fuzz to see how it performs continuously. I will modify the fuzzer over the next couple of weeks, and getting feedback from OSS-fuzz helps tremendously in that process.

@AdamKorcz AdamKorcz force-pushed the fuzz2 branch 5 times, most recently from 2ddf1be to e202efc Compare March 30, 2021 11:25
@AdamKorcz
Copy link
Contributor Author

@AkihiroSuda Please check the updated version of the fuzzer. We have aborted using json to create the config and instead pass random values to the struct.

Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

Looks better, thanks

go.mod Outdated Show resolved Hide resolved
@AdamKorcz AdamKorcz force-pushed the fuzz2 branch 3 times, most recently from 02db130 to 08f58e9 Compare March 30, 2021 17:53
@AdamKorcz
Copy link
Contributor Author

@kolyshkin Could you take a look at the failing centos test? It looks unrelated to the files in this PR.

@AdamKorcz
Copy link
Contributor Author

@kolyshkin @AkihiroSuda How is this one looking now?

AkihiroSuda
AkihiroSuda previously approved these changes Mar 31, 2021
@AdamKorcz
Copy link
Contributor Author

I ran the tests again, and this PR now passes

@AdamKorcz
Copy link
Contributor Author

@AkihiroSuda @kolyshkin How is this one looking now?

@kolyshkin
Copy link
Contributor

@AdamKorcz can you please fix the commit message? Now it merely says "Updated" which is not a good description of what the commit does.

@kolyshkin
Copy link
Contributor

Also needs a rebase.

kolyshkin
kolyshkin previously approved these changes Apr 30, 2021
Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

LGTM

(but this will probably need a rebase after #2925 is merged)

@AdamKorcz
Copy link
Contributor Author

@kolyshkin Can this be merged?

@caniszczyk
Copy link
Contributor

it has branch conflicts now

but cc: @opencontainers/runc-maintainers

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

rebased

Copy link
Member

@cyphar cyphar left a comment

Choose a reason for hiding this comment

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

LGTM.

@cyphar
Copy link
Member

cyphar commented Jul 16, 2021

/ping @opencontainers/runc-maintainers

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.

5 participants