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

rust/applayer - derive utility functions for app-layer event enums - v7 #5579

Closed
wants to merge 19 commits into from

Conversation

jasonish
Copy link
Member

This PR shows how we can use a Rust procedural macro to derive AppLayer event functions from an emum.

Given an enum like:

#[derive(Debug, PartialEq, AppLayerEvent)]
pub enum DNSEvent {
    MalformedData,
    NotRequest,
    NotResponse,
    ZFlagSet,
}

supporting code will be added allowing the protocol to register get_event_info and get_event_info_by_id callbacks like:

get_eventinfo: Some(DNSEvent::get_event_info),
get_eventinfo_byid: Some(DNSEvent::get_event_info_by_id),

All the parser needs to do is enumerate the different events, the macro will take care of generating the support code.

Redmine issue:
https://redmine.openinfosecfoundation.org/issues/4154

Previous PR:
#5501

Changes from last PR:

  • Convert all Rust parsers.

The derive macro will implement this trait for app-layer
event enums.
Currently has one derive, AppLayerEvent to be used like:

pub enum DNSEvent {
    MalformedData,
    NotRequest,
    NotResponse,
    ZFlagSet,
}

Code will be generated to:
- Convert enum to a c type string
- Convert string to enum variant
- Convert id to enum variant
Provide generic functions for get_event_info and
get_event_info_by_id. These functions can be used by any app-layer
event enum that implements AppLayerEvent.

Unfortunately the parser registration cannot use these functions
directly as generic functions cannot be #[no_mangle]. So they
do need small extern "C" wrappers around them.
Add generation of wrapper functions for get_event_info
and get_event_info_by_id to the derive macro. Eliminates
the need for the wrapper method to be created by the parser
author.
Implementations are not required if they're just going to return
-1. We allow None to be registered for that.
rust/derive/src/applayerevent.rs Show resolved Hide resolved
}

/// Transform names such as "OneTwoThree" to "one_two_three".
pub fn transform_name(in_name: &str) -> String {
Copy link

Choose a reason for hiding this comment

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

Does it make sense to use something like https://crates.io/crates/Inflector here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not too big of a fan of pulling in a dependency to solve something that can be done pretty easily in a function or 2.

///
/// Example usage (DNS app-layer events):
///
/// #[derive(AppLayerEvent)]
Copy link

Choose a reason for hiding this comment

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

Maybe add a rust/derive/tests/test.rs to demonstrate and test.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, docs/examples need a little more detail.

@jasonish jasonish force-pushed the derive-app-event/v7 branch from 23e1694 to bf5cf81 Compare November 19, 2020 01:04
proc-macro = true
path = "@e_rustdir@/derive/src/lib.rs"

[dependencies]
Copy link
Member

Choose a reason for hiding this comment

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

are these build dependencies only? Or do they end up in the compiled bin?

Copy link
Member Author

Choose a reason for hiding this comment

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

They are build only, but can't be labeled as [build-dependencies]. Within the scope of the derive crate, they are runtime dependencies, but only used by Suricata at a build time.

However, it doesn't appear the derive crate can be labeled as a build-dependency either, however, it does not end up in the output binary. Instead the derive crate is compiled and loaded by the compiler as a "compiler" plugin. This becomes more visible in a cross compile scenario. The derive plugin would be compiled for the host environment, not the target environment.

Even though they already show up in our dependency tree, nothing triggers their usage yet so they will be new crates that we vendor. They're rather ubiquitous for anyone building procedural macros.

Copy link
Member

Choose a reason for hiding this comment

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

The last sentence on whether they will show up in the vendored crates is a bit unclear to me. Will they be included, but not end up in the compiled bin?

Copy link
Member Author

Choose a reason for hiding this comment

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

They will be included in our vendor directory, compiled and run, but not in the output binary. However, their output (the generated code) will be in our output binary if that makes sense.

Much like the output of yacc, but where yacc is compiled and run as part of the build.

@jasonish
Copy link
Member Author

Updated version at #6318.

@jasonish jasonish closed this Aug 30, 2021
@jasonish jasonish deleted the derive-app-event/v7 branch September 2, 2021 19:59
jufajardini pushed a commit to jufajardini/suricata that referenced this pull request Oct 24, 2022
jufajardini pushed a commit to jufajardini/suricata that referenced this pull request Oct 24, 2022
jufajardini added a commit to jufajardini/suricata that referenced this pull request Oct 24, 2022
Pgsql's parameters - for message types like StartupMessage and
ParameterStatus, for instance, don't have a finite, definitive set, as
per their documentation. Our json schema was allow expecting a fixed set
of parameters, though, resulting in SV tests failing if different, valid
parameters appeared.

Bug OISF#5579
jufajardini added a commit to jufajardini/suricata that referenced this pull request Oct 24, 2022
For StartupMessages, the database parameter is optional. This moves the
parameter into the optional_parameters list.

Bug OISF#5579
jufajardini added a commit to jufajardini/suricata that referenced this pull request Oct 24, 2022
Since we've done some changes to how the parameters are parsed, add one
more test case to check that.

Bug OISF#5579
jufajardini added a commit to jufajardini/suricata that referenced this pull request Oct 24, 2022
Since we've done some changes to how the parameters are parsed, add one
more test case to check that.

Bug OISF#5579
victorjulien pushed a commit to victorjulien/suricata that referenced this pull request Oct 28, 2022
victorjulien pushed a commit to victorjulien/suricata that referenced this pull request Oct 28, 2022
victorjulien pushed a commit to victorjulien/suricata that referenced this pull request Oct 28, 2022
Pgsql's parameters - for message types like StartupMessage and
ParameterStatus, for instance, don't have a finite, definitive set, as
per their documentation. Our json schema was allow expecting a fixed set
of parameters, though, resulting in SV tests failing if different, valid
parameters appeared.

Bug OISF#5579
victorjulien pushed a commit to victorjulien/suricata that referenced this pull request Oct 28, 2022
For StartupMessages, the database parameter is optional. This moves the
parameter into the optional_parameters list.

Bug OISF#5579
victorjulien pushed a commit to victorjulien/suricata that referenced this pull request Oct 28, 2022
Since we've done some changes to how the parameters are parsed, add one
more test case to check that.

Bug OISF#5579
benignbala pushed a commit to benignbala/suricata that referenced this pull request Nov 12, 2022
benignbala pushed a commit to benignbala/suricata that referenced this pull request Nov 12, 2022
benignbala pushed a commit to benignbala/suricata that referenced this pull request Nov 12, 2022
Pgsql's parameters - for message types like StartupMessage and
ParameterStatus, for instance, don't have a finite, definitive set, as
per their documentation. Our json schema was allow expecting a fixed set
of parameters, though, resulting in SV tests failing if different, valid
parameters appeared.

Bug OISF#5579
benignbala pushed a commit to benignbala/suricata that referenced this pull request Nov 12, 2022
For StartupMessages, the database parameter is optional. This moves the
parameter into the optional_parameters list.

Bug OISF#5579
benignbala pushed a commit to benignbala/suricata that referenced this pull request Nov 12, 2022
Since we've done some changes to how the parameters are parsed, add one
more test case to check that.

Bug OISF#5579
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs rebase Needs rebase to master
Development

Successfully merging this pull request may close these issues.

3 participants