Skip to content

Fuzz impl v2 #3881

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

Merged
merged 14 commits into from
Feb 13, 2025
Merged

Fuzz impl v2 #3881

merged 14 commits into from
Feb 13, 2025

Conversation

rcoh
Copy link
Collaborator

@rcoh rcoh commented Oct 17, 2024

Motivation and Context

This implements fuzzing for smithy-rs servers

Description

Testing

Checklist

  • For changes to the smithy-rs codegen or runtime crates, I have created a changelog entry Markdown file in the .changelog directory, specifying "client," "server," or both in the applies_to key.
  • For changes to the AWS SDK, generated SDK code, or SDK runtime crates, I have created a changelog entry Markdown file in the .changelog directory, specifying "aws-sdk-rust" in the applies_to key.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@rcoh rcoh requested review from a team as code owners October 17, 2024 14:15
@rcoh rcoh requested a review from drganjoo January 9, 2025 16:29
Copy link

github-actions bot commented Jan 9, 2025

A new generated diff is ready to view.

  • No codegen difference in the AWS SDK
  • Client Test (ignoring whitespace)
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

Copy link

github-actions bot commented Jan 9, 2025

A new generated diff is ready to view.

  • No codegen difference in the AWS SDK
  • Client Test (ignoring whitespace)
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

Copy link
Contributor

@david-perez david-perez left a comment

Choose a reason for hiding this comment

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

Good to go but we should ensure we run the fuzzgen test in fuzzgen/src/test/kotlin/software/amazon/smithy/rust/codegen/fuzz/FuzzHarnessBuildPluginTest.kt in CI.

@@ -113,6 +113,7 @@ class CargoTomlGenerator(
"features" to cargoFeatures.toMap(),
).deepMergeWith(manifestCustomizations)

println(cargoToml)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this intentional or a leftover?

operations: [SayHello],
version: "1"
}
@optionalAuth
Copy link
Contributor

Choose a reason for hiding this comment

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

Is @optionalAuth relevant here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no

import software.amazon.smithy.rust.codegen.core.util.runCommand
import software.amazon.smithy.rust.codegen.server.smithy.testutil.serverIntegrationTest

class FuzzHarnessBuildPluginTest() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this test will get run by CI? Since this fuzzgen Gradle module is new.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes fixed thanks

3. The fuzz driver can be used on other fuzz targets.

### Setup
First, you'll need to generate the 1 (or more) versions of a smithy-rs server to test against. The best way to do this is by using the smithy CLI. **This process is fully automated with the `aws-smithy-fuzz setup-smithy`. The following docs are in place in case you want to alter the behavior.**
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
First, you'll need to generate the 1 (or more) versions of a smithy-rs server to test against. The best way to do this is by using the smithy CLI. **This process is fully automated with the `aws-smithy-fuzz setup-smithy`. The following docs are in place in case you want to alter the behavior.**
First, you'll need to generate the 2 (or more) versions of a smithy-rs server to test against. The best way to do this is by using the smithy CLI. **This process is fully automated with the `aws-smithy-fuzz setup-smithy`. The following docs are in place in case you want to alter the behavior.**

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

you can fuzz against 1 — you are then fuzzing for panics instead of fuzzing for comparison

exec(
Command::new("rm")
.arg("-r")
.arg(format!("{}/.m2", home_dir.display()))
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps nuke just ~/.m2/repository/software/amazon/smithy/? Lest we overreach and delete important stuff.

@rcoh rcoh enabled auto-merge February 10, 2025 20:19
Copy link

A new generated diff is ready to view.

  • No codegen difference in the AWS SDK
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

@@ -0,0 +1,111 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a great place to comment so I'm just picking here. This is fine for now but I'd really like to see us refactor our project structure a bit and group codegen all together, something like:

codegen/
     smithy-rs-server-codegen/
     smithy-rs-client-codegen/
     smithy-rs-shared-codegen/
     ...

Example names but we also need to consider publishing and it's useful if the directory for a maven artifact matches the published name (1) easier to find and (2) that's the default anyway when publishing is to use the project name.

I'd like to do something similar for the runtime.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Seems reasonable

@rcoh rcoh added this pull request to the merge queue Feb 12, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 12, 2025
@rcoh rcoh added this pull request to the merge queue Feb 13, 2025
Merged via the queue into main with commit a9d4f1d Feb 13, 2025
44 of 45 checks passed
@rcoh rcoh deleted the fuzz-impl-v2 branch February 13, 2025 13:39
drganjoo pushed a commit that referenced this pull request Feb 14, 2025
## Motivation and Context
This implements fuzzing for smithy-rs servers

## Description
<!--- Describe your changes in detail -->

## Testing
<!--- Please describe in detail how you tested your changes -->
<!--- Include details of your testing environment, and the tests you ran
to -->
<!--- see how your change affects other areas of the code, etc. -->

## Checklist
<!--- If a checkbox below is not applicable, then please DELETE it
rather than leaving it unchecked -->
- [ ] For changes to the smithy-rs codegen or runtime crates, I have
created a changelog entry Markdown file in the `.changelog` directory,
specifying "client," "server," or both in the `applies_to` key.
- [ ] For changes to the AWS SDK, generated SDK code, or SDK runtime
crates, I have created a changelog entry Markdown file in the
`.changelog` directory, specifying "aws-sdk-rust" in the `applies_to`
key.

----

_By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice._

---------

Co-authored-by: david-perez <[email protected]>
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.

4 participants