Restore Serialize for two fuzz targets
#572
Conversation
Signed-off-by: Adrian Palacios <accorell@amazon.com>
Signed-off-by: Adrian Palacios <accorell@amazon.com>
adpaco-aws
requested review from
cdisselkoen,
katherine-hough and
john-h-kastner-aws
| #[derive(Debug, Clone, Serialize)] | ||
| struct FuzzTargetInput { | ||
| #[serde(skip)] | ||
| request: ABACRequest, | ||
| policy: ABACPolicy, | ||
| #[serde(skip)] | ||
| entities: Entities, | ||
| #[serde(skip)] | ||
| schema: cedar_policy_validator::ValidatorSchema, | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I think we can remove that code? Is it used by anything?
There was a problem hiding this comment.
Nothing if you don't use --features log.
There was a problem hiding this comment.
I guess the question is whether --features log is useful if it only logs the policy and not the other inputs.
There was a problem hiding this comment.
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?).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I can't find this being used anywhere. I'm going to open a PR to delete the feature.
There was a problem hiding this comment.
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.
Description of changes: Our fuzzers are built which
--features logwhich in turn require theSerializetrait to be implemented for fuzz target inputs. cedar-policy/cedar#1520 removed theSerializeimplementations from many core/validator types, but CI didn't catch it because we don't test the build with--features log.This PR restores
Serializefor two fuzz targets:protobuf-roundtrip: We use#[serde(skip)]for each field exceptpolicy. This is how it's done in other targets, and I assumeValidatorSchemais safe to skip because it's built fromSchemawhich is skipped in other targets as well.wildcard-matching: We implement a simple custom serializer to avoid restoring it incedar.