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.

Sign up for GitHub

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

Restore Serialize for two fuzz targets #572

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Restore Serialize for two fuzz targets #572

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9949731
Re-derive `Serialize` for `protobuf-roundtrip`
adpaco-aws Mar 7, 2025
c0cb75d
Implement custom serializer for `wildcard-matching`
adpaco-aws Mar 7, 2025
5b60e2b
Reformat
adpaco-aws Mar 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion cedar-drt/fuzz/fuzz_targets/protobuf-roundtrip.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,16 @@ use cedar_policy_generators::{
settings::ABACSettings,
};

#[derive(Debug, Clone)]
use serde::Serialize;

#[derive(Debug, Clone, Serialize)]
struct FuzzTargetInput {
#[serde(skip)]
request: ABACRequest,
policy: ABACPolicy,
#[serde(skip)]
entities: Entities,
#[serde(skip)]
schema: cedar_policy_validator::ValidatorSchema,
}
Comment on lines +38 to 47
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm still interested in what this Serialize is used for, and whether we could just remove uses of Serialize for this FuzzTargetInput instead of restoring Serialize here.

If we do go this route, I'm wondering whether this Serialize implementation which serializes only the policy is going to be helpful, or just cause even more confusion for whatever is consuming it. For instance, if it's supposed to be a representation of the logged FuzzTargetInput, the fact that you only see the policy is probably going to be confusing or even deceiving while debugging a problem with a FuzzTargetInput.

Copy link

Choose a reason for hiding this comment

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

I assume it's used here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can remove that code? Is it used by anything?

Copy link

Choose a reason for hiding this comment

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

Nothing if you don't use --features log.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the question is whether --features log is useful if it only logs the policy and not the other inputs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I doubt the logs are useful given that they ignore most of the input. I decided to open the PR because it was taking me too long to figure where LOGFILE is being set, and how it's connected to other components (i.e., are these files being expected somewhere else?).

Copy link

@katherine-hough katherine-hough Mar 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It's probably not useful. I assume this is used to analyze a random sample of generated inputs after the campaign completes presumably to collect statistics about what was generated or debug generation. Since policy generation depends on the other generated inputs, I imagine just having the policies wouldn't help very much with either. Long term I think there is probably way to re-implement this that doesn't depend on the generated structures implementing Serialize.

Copy link
Contributor

Choose a reason for hiding this comment

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

I concur.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't find this being used anywhere. I'm going to open a PR to delete the feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update: I've found where it's used, and looks like at least a couple targets might be using it. Don't think anyone is looking at the logged information though.


Expand Down
24 changes: 24 additions & 0 deletions cedar-drt/fuzz/fuzz_targets/wildcard-matching.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ use cedar_drt_inner::fuzz_target;
use cedar_policy_core::ast::{Pattern, PatternElem};
use libfuzzer_sys::arbitrary::{self, Arbitrary, Result, Unstructured};
use regex::{escape, Regex};
use serde::ser::SerializeStruct;
use serde::{Serialize, Serializer};

/// Input expected by this fuzz target:
/// A pattern and a string that matches it
Expand All @@ -31,6 +33,28 @@ struct FuzzTargetInput {
pub string: String,
}

impl Serialize for FuzzTargetInput {
fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
where
S: Serializer,
{
let mut state = serializer.serialize_struct("FuzzTargetInput", 2)?;

let pattern: Vec<String> = self
.pattern
.iter()
.map(|e| match e {
PatternElem::Char(c) => c.to_string(),
PatternElem::Wildcard => "*".to_string(),
})
.collect();

state.serialize_field("pattern", &pattern)?;
state.serialize_field("string", &self.string)?;
state.end()
}
}

/// A wrapper struct for valid characters:
/// A character `c` is valid if it satisfies two criteria:
/// 1. c as u32 <= 0xffff (i.e., c is in the Basic Multilingual Plane.)
Expand Down