diff options
Diffstat (limited to 'doc/userguide/devguide/codebase')
-rw-r--r-- | doc/userguide/devguide/codebase/code-style.rst | 756 | ||||
-rw-r--r-- | doc/userguide/devguide/codebase/contributing/code-submission-process.rst | 68 | ||||
-rw-r--r-- | doc/userguide/devguide/codebase/contributing/contribution-process.rst | 271 | ||||
-rw-r--r-- | doc/userguide/devguide/codebase/contributing/github-pr-workflow.rst | 46 | ||||
-rw-r--r-- | doc/userguide/devguide/codebase/contributing/index.rst | 9 | ||||
-rw-r--r-- | doc/userguide/devguide/codebase/fuzz-testing.rst | 31 | ||||
-rw-r--r-- | doc/userguide/devguide/codebase/img/InputCaptureExample.png | bin | 0 -> 315383 bytes | |||
-rw-r--r-- | doc/userguide/devguide/codebase/index.rst | 13 | ||||
-rw-r--r-- | doc/userguide/devguide/codebase/installation-from-git.rst | 154 | ||||
-rw-r--r-- | doc/userguide/devguide/codebase/testing.rst | 158 | ||||
-rw-r--r-- | doc/userguide/devguide/codebase/unittests-c.rst | 148 | ||||
-rw-r--r-- | doc/userguide/devguide/codebase/unittests-rust.rst | 91 |
12 files changed, 1745 insertions, 0 deletions
diff --git a/doc/userguide/devguide/codebase/code-style.rst b/doc/userguide/devguide/codebase/code-style.rst new file mode 100644 index 0000000..1d1d009 --- /dev/null +++ b/doc/userguide/devguide/codebase/code-style.rst @@ -0,0 +1,756 @@ +.. _Coding Style: + +============ +Coding Style +============ + +Suricata uses a fairly strict coding style. This document describes it. + +Formatting +~~~~~~~~~~ + +clang-format +^^^^^^^^^^^^ +``clang-format`` is configured to help you with formatting C code. + +.. note:: + + The ``.clang-format`` script requires clang 9 or newer. + +Format your Changes +******************* + +Before opening a pull request, please also try to ensure it is formatted +properly. We use ``clang-format`` for this, which has git integration through the +``git-clang-format`` script to only format your changes. + +On some systems, it may already be installed (or be installable via your package +manager). If so, you can simply run it. + +It is recommended to format each commit as you go. However, you can always +reformat your whole branch after the fact. + +.. note:: + + Depending on your installation, you might have to use the version-specific + ``git clang-format`` in the commands below, e.g. ``git clang-format-9``, + and possibly even provide the ``clang-format`` binary with + ``--binary clang-format-9``. + + As an alternative, you can use the provided ``scripts/clang-format.sh`` + that isolates you from the different versions. + +Formatting the most recent commit only +"""""""""""""""""""""""""""""""""""""" +The following command will format only the code changed in the most recent commit: + +.. code-block:: bash + + $ git clang-format HEAD^ + # Or with script: + $ scripts/clang-format.sh commit + +Note that this modifies the files, but doesn't commit them. If the changes are +trivial, you’ll likely want to run + +.. code-block:: bash + + $ git commit --amend -a + +in order to update the last commit with all pending changes. + +For bigger formatting changes, we do ask you to add them to separate, dedicated +commits. + +Formatting code in staging +"""""""""""""""""""""""""" +The following command will format the changes in staging, i.e. files you +``git add``-ed: + +.. code-block:: bash + + $ git clang-format + # Or with script: + $ scripts/clang-format.sh cached + +If you also want to change the unstaged changes, do: + +.. code-block:: bash + + $ git clang-format --force + # Or with script: + $ scripts/clang-format.sh cached --force + +Formatting your branch's commits +"""""""""""""""""""""""""""""""" +In case you have multiple commits on your branch already and forgot to +format them you can fix that up as well. + +The following command will format every commit in your branch off master and +rewrite history using the existing commit metadata. + +Tip: Create a new version of your branch first and run this off the new version. + +.. code-block:: bash + + # In a new version of your pull request: + $ scripts/clang-format.sh rewrite-branch + +Note that the above should only be used for rather minimal formatting changes. +As mentioned, we prefer that you add such changes to a dedicated commit for +formatting changes: + +.. code-block:: bash + + # Format all changes by commits on your branch: + $ git clang-format first_commit_on_your_branch^ + # Or with script: + $ scripts/clang-format.sh branch + +Note the usage of ``first_commit_on_your_branch^``, not ``master``, to avoid picking up +new commits on ``master`` in case you've updated master since you've branched. + +Check formatting +"""""""""""""""" +Check if your branch changes' formatting is correct with: + +.. code-block:: bash + + $ scripts/clang-format.sh check-branch + +Add the ``--diffstat`` parameter if you want to see the files needing formatting. +Add the ``--diff`` parameter if you want to see the actual diff of the formatting +change. + +Formatting a whole file +""""""""""""""""""""""" + ++--------------------------------------------------------------------+ +| **Note** | +| | +| Do not reformat whole files by default, i.e. do not use | +| ``clang-format`` proper in general. | ++--------------------------------------------------------------------+ + +If you were ever to do so, formatting changes of existing code with clang-format +shall be a different commit and must not be mixed with actual code changes. + +.. code-block:: bash + + $ clang-format -i {file} + +Disabling clang-format +********************** + +There might be times, where the clang-format's formatting might not please. +This might mostly happen with macros, arrays (single or multi-dimensional ones), +struct initialization, or where one manually formatted code. + +You can always disable clang-format. + +.. code-block:: c + + /* clang-format off */ + #define APP_LAYER_INCOMPLETE(c, n) (AppLayerResult){1, (c), (n)} + /* clang-format on */ + +Installing clang-format and git-clang-format +******************************************** +clang-format 9 or newer is required. + +On ubuntu 18.04: + +- It is sufficient to only install clang-format, e.g. + + .. code-block:: bash + + $ sudo apt-get install clang-format-9 + +- See http://apt.llvm.org for other releases in case the clang-format version + is not found in the default repos. + +On fedora: + +- Install the ``clang`` and ``git-clang-format`` packages with + + .. code-block:: bash + + $ sudo dnf install clang git-clang-format + + +Line length +^^^^^^^^^^^ + +Limit line lengths to 100 characters. + +When wrapping lines that are too long, they should be indented at least 8 +spaces from previous line. You should attempt to wrap the minimal portion of +the line to meet the 100 character limit. + +clang-format: + - ColumnLimit: 100 + - ContinuationIndentWidth: 8 + - ReflowComments: true + + +Indent +^^^^^^ + +We use 4 space indentation. + +.. code-block:: c + + int DecodeEthernet(ThreadVars *tv, DecodeThreadVars *dtv, Packet *p, + uint8_t *pkt, uint16_t len, PacketQueue *pq) + { + SCPerfCounterIncr(dtv->counter_eth, tv->sc_perf_pca); + + if (unlikely(len < ETHERNET_HEADER_LEN)) { + ENGINE_SET_INVALID_EVENT(p, ETHERNET_PKT_TOO_SMALL); + return TM_ECODE_FAILED; + } + + ... + + DecodeNetworkLayer(tv, dtv, SCNtohs(p->ethh->eth_type), p, + pkt + ETHERNET_HEADER_LEN, len - ETHERNET_HEADER_LEN); + + return TM_ECODE_OK; + } + +Use 8 space indentation when wrapping function parameters, loops and if statements. + +Use 4 space indentation when wrapping variable definitions. + +.. code-block:: c + + const SCPlugin PluginSpec = { + .name = OUTPUT_NAME, + .author = "Some Developer", + .license = "GPLv2", + .Init = TemplateInit, + }; + + +clang-format: + - AlignAfterOpenBracket: DontAlign + - Cpp11BracedListStyle: false + - IndentWidth: 4 + - TabWidth: 8 [llvm]_ + - UseTab: Never [llvm]_ + +Braces +^^^^^^ + +Functions should have the opening brace on a newline: + +.. code-block:: c + + int SomeFunction(void) + { + DoSomething(); + } + +Note: you may encounter non-compliant code. + +Control and loop statements should have the opening brace on the same line: + +.. code-block:: c + + if (unlikely(len < ETHERNET_HEADER_LEN)) { + ENGINE_SET_INVALID_EVENT(p, ETHERNET_PKT_TOO_SMALL); + return TM_ECODE_FAILED; + } + + for (ascii_code = 0; ascii_code < 256; ascii_code++) { + ctx->goto_table[ctx->state_count][ascii_code] = SC_AC_FAIL; + } + + while (funcs != NULL) { + temp = funcs; + funcs = funcs->next; + SCFree(temp); + } + +Opening and closing braces go on the same line as as the _else_ (also known as a "cuddled else"). + +.. code-block:: c + + if (this) { + DoThis(); + } else { + DoThat(); + } + +Structs, unions and enums should have the opening brace on the same line: + +.. code-block:: c + + union { + TCPVars tcpvars; + ICMPV4Vars icmpv4vars; + ICMPV6Vars icmpv6vars; + } l4vars; + + struct { + uint8_t type; + uint8_t code; + } icmp_s; + + enum { + DETECT_TAG_TYPE_SESSION, + DETECT_TAG_TYPE_HOST, + DETECT_TAG_TYPE_MAX + }; + +clang-format: + - BreakBeforeBraces: Custom [breakbeforebraces]_ + - BraceWrapping: + + - AfterClass: true + - AfterControlStatement: false + - AfterEnum: false + - AfterFunction: true + - AfterStruct: false + - AfterUnion: false + - AfterExternBlock: true + - BeforeElse: false + - IndentBraces: false + +Flow +~~~~ + +Don't use conditions and statements on the same line. E.g. + +.. code-block:: c + + if (a) b = a; // <- wrong + + if (a) + b = a; // <- right + + for (int i = 0; i < 32; ++i) f(i); // <- wrong + + for (int i = 0; i < 32; ++i) + f(i); // <- right + +Don't put short or empty functions and structs on one line. + +.. code-block:: c + + void empty_function(void) + { + } + + int short_function(void) + { + return 1; + } + +Don't use unnecessary branching. E.g.: + +.. code-block:: c + + if (error) { + goto error; + } else { + a = b; + } + + +Can be written as: + +.. code-block:: c + + if (error) { + goto error; + } + a = b; + +clang-format: + - AllowShortBlocksOnASingleLine: false [llvm]_ + - AllowShortBlocksOnASingleLine: Never [llvm]_ (breaking change in clang 10!) [clang10]_ + - AllowShortEnumsOnASingleLine: false [clang11]_ + - AllowShortFunctionsOnASingleLine: None + - AllowShortIfStatementsOnASingleLine: Never [llvm]_ + - AllowShortLoopsOnASingleLine: false [llvm]_ + - BreakBeforeBraces: Custom [breakbeforebraces]_ + - BraceWrapping: + + - SplitEmptyFunction: true + - SplitEmptyRecord: true + +Alignment +~~~~~~~~~ + +Pointers +^^^^^^^^ +Pointers shall be right aligned. + +.. code-block:: c + + void *ptr; + void f(int *a, const char *b); + void (*foo)(int *); + +clang-format: + - PointerAlignment: Right + - DerivePointerAlignment: false + +Declarations and Comments +^^^^^^^^^^^^^^^^^^^^^^^^^ +Trailing comments should be aligned for consecutive lines. + +.. code-block:: c + + struct bla { + int a; /* comment */ + unsigned bb; /* comment */ + int *ccc; /* comment */ + }; + + void alignment() + { + // multiple consecutive vars + int a = 13; /* comment */ + int32_t abc = 1312; /* comment */ + int abcdefghikl = 13; /* comment */ + } + +clang-format: + - AlignConsecutiveAssignments: false + - AlignConsecutiveDeclarations: false + - AlignTrailingComments: true + +Functions +~~~~~~~~~ + +parameter names +^^^^^^^^^^^^^^^ + +TODO + +Function names +^^^^^^^^^^^^^^ + +Function names are NamedLikeThis(). + +.. code-block:: c + + static ConfNode *ConfGetNodeOrCreate(char *name, int final) + +static vs non-static +^^^^^^^^^^^^^^^^^^^^ + +Functions should be declared static whenever possible. + +inline +^^^^^^ + +The inlining of functions should be used only in critical paths. + +Variables +~~~~~~~~~ + +Names +^^^^^ + +A variable is ``named_like_this`` in all lowercase. + +.. code-block:: c + + ConfNode *parent_node = root; + +Generally, use descriptive variable names. + +In loop vars, make sure ``i`` is a signed int type. + +Scope +^^^^^ + +TODO + +Macros +~~~~~~ + +Macro names are ALL_CAPS_WITH_UNDERSCORES. +Enclose parameters in parens on each usage inside the macro. + +Align macro values on consecutive lines. + +.. code-block:: c + + #define ACTION_ALERT 0x01 + #define ACTION_DROP 0x02 + #define ACTION_REJECT 0x04 + #define ACTION_REJECT_DST 0x08 + #define ACTION_REJECT_BOTH 0x10 + #define ACTION_PASS 0x20 + +Align escape for multi-line macros right-most at ColumnLimit. + +.. code-block:: c + + #define MULTILINE_DEF(a, b) \ + if ((a) > 2) { \ + auto temp = (b) / 2; \ + (b) += 10; \ + someFunctionCall((a), (b)); \ + } + +clang-format: + - AlignConsecutiveMacros: true [clang9]_ + - AlignEscapedNewlines: Right + +Comments +~~~~~~~~ + +TODO + +Function comments +^^^^^^^^^^^^^^^^^ + +We use Doxygen, functions are documented using Doxygen notation: + +.. code-block:: c + + /** + * \brief Helper function to get a node, creating it if it does not + * exist. + * + * This function exits on memory failure as creating configuration + * nodes is usually part of application initialization. + * + * \param name The name of the configuration node to get. + * \param final Flag to set created nodes as final or not. + * + * \retval The existing configuration node if it exists, or a newly + * created node for the provided name. On error, NULL will be returned. + */ + static ConfNode *ConfGetNodeOrCreate(char *name, int final) + +General comments +^^^^^^^^^^^^^^^^ + +We use ``/* foobar */`` style and try to avoid ``//`` style. + +File names +~~~~~~~~~~ + +File names are all lowercase and have a .c. .h or .rs (Rust) extension. + +Most files have a _subsystem_ prefix, e.g. ``detect-dsize.c, util-ip.c`` + +Some cases have a multi-layer prefix, e.g. ``util-mpm-ac.c`` + +Enums +~~~~~ + +Use a common prefix for all enum values. Value names are ALL_CAPS_WITH_UNDERSCORES. + +Put each enum values on a separate line. +Tip: Add a trailing comma to the last element to force "one-value-per-line" +formatting in clang-format. + +.. code-block:: c + + enum { VALUE_ONE, VALUE_TWO }; // <- wrong + + // right + enum { + VALUE_ONE, + VALUE_TWO, // <- force one-value-per-line + }; + +clang-format: + - AllowShortEnumsOnASingleLine: false [clang11]_ + +Structures and typedefs +~~~~~~~~~~~~~~~~~~~~~~~ + +TODO + +switch statements +~~~~~~~~~~~~~~~~~ + +Switch statements are indented like in the following example, so the 'case' is indented from the switch: + +.. code-block:: c + + switch (ntohs(p->ethh->eth_type)) { + case ETHERNET_TYPE_IP: + DecodeIPV4(tv, dtv, p, pkt + ETHERNET_HEADER_LEN, + len - ETHERNET_HEADER_LEN, pq); + break; + +Fall through cases will be commented with ``/* fall through */``. E.g.: + +.. code-block:: c + + switch (suri->run_mode) { + case RUNMODE_PCAP_DEV: + case RUNMODE_AFP_DEV: + case RUNMODE_PFRING: + /* find payload for interface and use it */ + default_packet_size = GetIfaceMaxPacketSize(suri->pcap_dev); + if (default_packet_size) + break; + /* fall through */ + default: + default_packet_size = DEFAULT_PACKET_SIZE; + + +Do not put short case labels on one line. +Put opening brace on same line as case statement. + +.. code-block:: c + + switch (a) { + case 13: { + int a = bla(); + break; + } + case 15: + blu(); + break; + default: + gugus(); + } + + +clang-format: + - IndentCaseLabels: true + - IndentCaseBlocks: false [clang11]_ + - AllowShortCaseLabelsOnASingleLine: false [llvm]_ + - BreakBeforeBraces: Custom [breakbeforebraces]_ + - BraceWrapping: + + - AfterCaseLabel: false (default) + +const +~~~~~ + +TODO + +goto +~~~~ + +Goto statements should be used with care. Generally, we use it primarily for error handling. E.g.: + +.. code-block:: c + + static DetectFileextData *DetectFileextParse (char *str) + { + DetectFileextData *fileext = NULL; + + fileext = SCMalloc(sizeof(DetectFileextData)); + if (unlikely(fileext == NULL)) + goto error; + + memset(fileext, 0x00, sizeof(DetectFileextData)); + + if (DetectContentDataParse("fileext", str, &fileext->ext, &fileext->len, &fileext->flags) == -1) { + goto error; + } + + return fileext; + + error: + if (fileext != NULL) + DetectFileextFree(fileext); + return NULL; + } + +Put goto labels at brace level. + +.. code-block:: c + + int goto_style_nested() + { + if (foo()) { + label1: + bar(); + } + + label2: + return 1; + } + +clang-format: + - IndentGotoLabels: true (default) [clang10]_ + +Includes +~~~~~~~~ + +TODO + +A .c file shall include it's own header first. + +clang-format: + - SortIncludes: false + +Unittests +~~~~~~~~~ + +When writing unittests that use a data array containing a protocol message, please put an explanatory comment that contain the readable content of the message + +So instead of: + +.. code-block:: c + + int SMTPProcessDataChunkTest02(void) + { + char mimemsg[] = {0x4D, 0x49, 0x4D, 0x45, 0x2D, 0x56, 0x65, 0x72, + +you should have something like: + +.. code-block:: c + + int SMTPParserTest14(void) + { + /* 220 mx.google.com ESMTP d15sm986283wfl.6<CR><LF> */ + static uint8_t welcome_reply[] = { 0x32, 0x32, 0x30, 0x20, + +Banned functions +~~~~~~~~~~~~~~~~ + ++------------+---------------+-----------+ +| function | replacement | reason | ++============+===============+===========+ +| strtok | strtok_r | | ++------------+---------------+-----------+ +| sprintf | snprintf | unsafe | ++------------+---------------+-----------+ +| strcat | strlcat | unsafe | ++------------+---------------+-----------+ +| strcpy | strlcpy | unsafe | ++------------+---------------+-----------+ +| strncpy | strlcat | | ++------------+---------------+-----------+ +| strncat | strlcpy | | ++------------+---------------+-----------+ +| strndup | |OS specific| ++------------+---------------+-----------+ +| strchrnul | | | ++------------+---------------+-----------+ +| rand | | | ++------------+---------------+-----------+ +| rand_r | | | ++------------+---------------+-----------+ +| index | | | ++------------+---------------+-----------+ +| rindex | | | ++------------+---------------+-----------+ +| bzero | memset | | ++------------+---------------+-----------+ + +Also, check the existing code. If yours is wildly different, it's wrong. +Example: https://github.com/oisf/suricata/blob/master/src/decode-ethernet.c + +.. rubric:: Footnotes + +.. [llvm] Default LLVM clang-format Style +.. [clang9] Requires clang 9 +.. [clang10] Requires clang 10 +.. [clang11] Requires clang 11 +.. [breakbeforebraces] BreakBeforeBraces: Mozilla is closest, but does not split empty functions/structs diff --git a/doc/userguide/devguide/codebase/contributing/code-submission-process.rst b/doc/userguide/devguide/codebase/contributing/code-submission-process.rst new file mode 100644 index 0000000..22bf160 --- /dev/null +++ b/doc/userguide/devguide/codebase/contributing/code-submission-process.rst @@ -0,0 +1,68 @@ +Code Submission Process +======================= + +.. _commits: + +Commits +~~~~~~~ + +#. Commits need to be logically separated. Don't fix unrelated things in one commit. +#. Don't add unnecessary commits, if commit 2 fixes commit 1 merge them together (squash) +#. Commits need to have proper messages, explaining anything that is non-trivial +#. Commits should not at the same time change, rename and/or move code. Use separate commits + for each of this, e.g, a commit to rename files, then a commit to change the code. +#. Documentation updates should be in their own commit (not mixed with code commits) +#. Commit messages need to be properly formatted: + * Meaningful and short (50 chars max) subject line followed by an empty line + * Naming convention: prefix message with sub-system ("rule parsing: fixing foobar"). If + you're not sure what to use, look at past commits to the file(s) in your PR. + * Description, wrapped at ~72 characters +#. Commits should be individually compilable, starting with the oldest commit. Make sure that + each commit can be built if it and the preceding commits in the PR are used. +#. Commits should be authored with the format: "FirstName LastName <name@example.com>" + +Information that needs to be part of a commit (if applicable): + +#. Ticket it fixes. E.g. "Fixes Bug #123." +#. Compiler warnings addressed. +#. Coverity Scan issues addressed. +#. Static analyzer error it fixes (cppcheck/scan-build/etc) + +When in doubt, check our git history for other messages or changes done to the +same module your're working on. This is a good example of a `commit message +<https://github.com/OISF/suricata/commit/33fca4d4db112b75ffa22eb2e6f14f038cbcc1f9>`_:: + + pcap/file: normalize file timestamps + + Normalize the timestamps that are too far in the past to epoch. + + Bug: #6240. + +.. _pull-requests-criteria: + +Pull Requests +~~~~~~~~~~~~~ + +A github pull request is actually just a pointer to a branch in your tree. GitHub provides a review interface that we use. + +#. A branch can only be used in for an individual PR. +#. A branch should not be updated after the pull request +#. A pull request always needs a good description (link to issue tracker if related to a ticket). +#. Incremental pull requests need to link to the prior iteration +#. Incremental pull requests need to describe changes since the last PR +#. Link to the ticket(s) that are addressed to it. +#. When fixing an issue, update the issue status to ``In Review`` after submitting the PR. +#. Pull requests are automatically tested using github actions (https://github.com/OISF/suricata/blob/master/.github/workflows/builds.yml). + Failing builds won't be considered and should be closed immediately. +#. Pull requests that change, or add a feature should include a documentation update commit + +Tests and QA +~~~~~~~~~~~~ + +As much as possible, new functionality should be easy to QA. + +#. Add ``suricata-verify`` tests for verification. See https://github.com/OISF/suricata-verify +#. Add unittests if a ``suricata-verify`` test isn't possible. +#. Provide pcaps that reproduce the problem. Try to trim as much as possible to the pcap includes the minimal + set of packets that demonstrate the problem. +#. Provide example rules if the code added new keywords or new options to existing keywords diff --git a/doc/userguide/devguide/codebase/contributing/contribution-process.rst b/doc/userguide/devguide/codebase/contributing/contribution-process.rst new file mode 100644 index 0000000..198ff14 --- /dev/null +++ b/doc/userguide/devguide/codebase/contributing/contribution-process.rst @@ -0,0 +1,271 @@ +************************ +Contributing to Suricata +************************ + +This guide describes what steps to take if you want to contribute a patch or +patchset to Suricata. + +Essentially, these are: + +#. Agree to and sign our :ref:`Contribution Agreement<contribution-agreement>` +#. Communicate early, and use the :ref:`preferred channels <communication-channels>` +#. :ref:`claim-ticket` +#. :ref:`Fork from master <what-branch-to-work-on>` +#. Follow our :ref:`Coding Style` +#. Use our :ref:`documentation-style` +#. Stick to our :ref:`commit guidelines<commits>` +#. Add version numbers to your :ref:`Pull Requests <send-a-pull-request>` +#. Incorporate :ref:`feedback` into new PRs +#. [Work merged] :ref:`Wrap up! <wrap-up>` + +The rest of this document will cover those in detail. + +.. _contribution-agreement: + +.. note:: Important! + + Before contributing, please review and sign our `Contribution Agreement + <https://suricata.io/contribution-agreements/>`_. + +.. _communication-channels: + +Communication is Key! +===================== + +To clarify questions, discuss or suggest new features, talk about bugs and +optimizations, and/or ask for help, it is important to communicate. + +These are our main channels: + +- `Suricata's issue tracker <https://redmine.openinfosecfoundation.org/ + projects/suricata/issues>`_ +- `Suricata's forum <https://forum.suricata.io/c/developers/8>`_ +- `Suricata's Discord server <https://discord.com/invite/t3rV2x7MrG>`_ + + +.. _claim-ticket: + +Claim (or open) a ticket +======================== + +For features and bugs we need `tickets <https://redmine.openinfosecfoundation. +org/projects/suricata/issues>`_. Tickets help us keep track of the work done, +indicate when changes need backports etc. + +They are also important if you would like to see your new feature officially +added to our tool: the ticket documents your ideas so we can analyze how do they +fit in our plans for Suricata, and, if the feature is accepted, we can properly +track progress etc. + +.. note:: If you want to add new functionalities (e.g. a new application layer + protocol), please ask us first whether we see that being merged into + Suricata or not. This helps both sides understand how the new feature will + fit in our roadmap, and prevents wasting time and motivation with + contributions that we may not accept. Therefore, `before` starting any code + related to a new feature, do request comments from the team about it. + +For really trivial fixes or cleanups we won't need that. + +Once work on the issue has been agreed upon: + +Assign the ticket to yourself. For this, you will need to have the "developer" +role. You can ask for that directly on the ticket you want to claim or mention +that you are interested in working on `ticket number` on our `Developer's +channel on Discord <https://discord.com/channels/864648830553292840/ +888087709002891324>`_. + +If a ticket is already assigned to someone, please reach out on the ticket or +ask the person first. + +You can reach out to other community members via `Suricata's Discord server +<https://discord.com/invite/t3rV2x7MrG>`_. + + +Expectations +============ + +If you submit a new feature that is not part of Suricata's core functionalities, +it will have the `community supported`_ status. This means we would expect some +commitment from you, or the organization who is sponsoring your work, before we +could approve the new feature, as the Suricata development team is pretty lean +(and many times overworked). + +This means we expect that: + + * the new contribution comes with a set of Suricata-verify tests (and + possibly unit tests, where those apply), before we can approve it; + * proof of compatibility with existing keywords/features is provided, + when the contribution is for replacing an existing feature; + * you would maintain the feature once it is approved - or some other + community member would do that, in case you cannot. + +.. note:: + + Regardless of contribution size or complexity, we expect that you respect + our guidelines and processes. We appreciate community contributors: + Suricata wouldn't be what it is without them; and the value of our tool and + community also comes from how seriously we take all this, so we ask that + our contributors do the same! + +.. _community supported: + +What does "community supported" and "supporting a feature" mean? +----------------------------------------------------------------- + +If a feature is *community supported*, the Suricata team will try to spend +minimal time on it - to be able to focus on the core functionalities. If for any +reason you're not willing or able to commit to supporting a feature, please +indicate this. + +The team and/or community members can then consider offering help. It is best +to indicate this prior to doing the actual work, because we will reject features +if no one steps up. + +It is also important to note that *community supported* features will be +disabled by default, and if it brings in new dependencies (libraries or Rust +crates) those will also be optional and disabled by default. + +**Supporting a feature** means to actually *maintain* it: + +* fixing bugs +* writing documentation +* keeping it up to date +* offering end-user support via forum and/or Discord chat + +.. _stale-tickets-policy: + +Stale tickets policy +==================== + +We understand that people's availability and interested to volunteer their time +to our project may change. Therefore, to prevent tickets going stale (not worked +on), and issues going unsolved for a long time, we have a policy to unclaim +tickets if there are no contribution updates within 6 months. + +If you claim a ticket and later on find out that you won't be able to work on +it, it is also appreciated if you inform that to us in the ticket and unclaim +it, so everyone knows that work is still open and waiting to be done. + +.. _what-branch-to-work-on: + +What branch to work on +====================== + +There are 2 or 3 active branches: + + * master-x.x.x (e.g. master-6.x.y) + * master + +The former is the stable branch. The latter the development branch. + +The stable branch should only be worked on for important bug fixes. Those are +mainly expected from more experienced contributors. + +Development of new features or large scale redesign is done in the development +branch. New development and new contributors should work with ``master`` except +in very special cases - which should and would be discussed with us first. + +If in doubt, please reach out to us via :ref:`Redmine, Discord or +forum <communication-channels>`. + +.. _create-your-own-branch: + +Create your own branch +====================== + +It's useful to create descriptive branch names. You're working on ticket 123 to +improve GeoIP? Name your branch "geoip-feature-123-v1". The "-v1" addition is +for feedback. When incorporating feedback you will have to create a new branch +for each pull request. So, when you address the first feedback, you will work in +"geoip-feature-123-v2" and so on. + +For more details check: `Creating a branch to do your changes <https://redmine. +openinfosecfoundation.org/projects/suricata/wiki/GitHub_work_flow#Creating-a- +branch-to-do-your-changes>`_ + + +Coding Style +============ + +We have a :ref:`Coding Style` that must be followed. + +.. _documentation-style: + +Documentation Style +=================== + +For documenting *code*, please follow Rust documentation and/or Doxygen +guidelines, according to what your contribution is using (Rust or C). + +If you are writing or updating *documentation pages*, please: + +* wrap up lines at 79 (80 at most) characters; +* when adding diagrams or images, we prefer alternatives that can be generated + automatically, if possible; +* bear in mind that our documentation is published on `Read the Docs <https:/ + /docs.suricata.io/en/latest/#suricata-user-guide>`_ and can also be + built to pdf, so it is important that it looks good in such formats. + + +Commit History matters +====================== + +Please consider our :ref:`Commit guidelines <commits>` before submitting your PR. + +.. _send-a-pull-request: + +Send a Pull Request +=================== + +The pull request is used to request inclusion of your patches into the main +repository. Before it is merged, it will be reviewed and pushed through a QA +process. + +Please consider our :ref:`Pull Requests Criteria <pull-requests-criteria>` when +submitting. + +We have 'GitHub-CI' integration enabled. This means some automated build check, +suricata-verity and unit tests are performed on the pull request. Generally, +this is ready after a few minutes. If the test fails, the pull request won't be +considered. So please, when you submit something, keep an eye on the checks, +and address any failures - if you do not understand what they are, it is fine to +ask about them on the failing PR itself. + +Before merge, we also perform other integration tests in our private QA-lab. If +those fail, we may request further changes, even if the GitHub-CI has passed. + +.. _feedback: + +Feedback +======== + +You'll likely get some feedback. Even our most experienced devs do, so don't +feel bad about it. + +After discussing what needs to be changed (usually on the PR itself), it's time +to go back to ":ref:`create-your-own-branch`" and do it all again. This process +can iterate quite a few times, as the contribution is refined. + +.. _wrap-up: + +Wrapping up +=========== + +Merged! Cleanup +--------------- + +Congrats! Your change has been merged into the main repository. Many thanks! + +We strongly suggest cleaning up: delete your related branches, both locally and +on GitHub - this helps you in keeping things organized when you want to make new +contributions. + +.. _update-ticket: + +Update ticket +------------- + +You can now put the URL of the *merged* pull request in the Redmine ticket. +Next, mark the ticket as "Closed" or "Resolved". + +Well done! You are all set now. diff --git a/doc/userguide/devguide/codebase/contributing/github-pr-workflow.rst b/doc/userguide/devguide/codebase/contributing/github-pr-workflow.rst new file mode 100644 index 0000000..618c966 --- /dev/null +++ b/doc/userguide/devguide/codebase/contributing/github-pr-workflow.rst @@ -0,0 +1,46 @@ +GitHub Pull Request Workflow +============================ + +Draft Pull Requests +~~~~~~~~~~~~~~~~~~~ + +A Pull Request (PR) should be marked as `draft` if it is not intended to be merged as is, +but is waiting for some sort of feedback. +The author of the PR should be explicit with what kind of feedback is expected +(CI/QA run, discussion on the code, etc...) + +GitHub filter is ``is:pr is:open draft:true sort:updated-asc`` + +A draft may be closed if it has not been updated in two months. + +Mergeable Pull Requests +~~~~~~~~~~~~~~~~~~~~~~~ + +When a Pull Request is intended to be merged as is, the workflow is the following: + 1. get reviewed, and either request changes or get approved + 2. if approved, get staged in a next branch (with other PRs), wait for CI validation + (and eventually request changes if CI finds anything) + 3. get merged and closed + +A newly created PR should match the filter +``is:pr is:open draft:false review:none sort:updated-asc no:assignee`` +The whole team is responsible to assign a PR to someone precise within 2 weeks. + +When someone gets assigned a PR, the PR should get a review status within 2 weeks: +either changes requested, approved, or assigned to someone else if more +expertise is needed. + +GitHub filter for changes-requested PRs is ``is:pr is:open draft:false sort: +updated-asc review:changes-requested`` + +Such a PR may be closed if it has not been updated in two months. +It is expected that the author creates a new PR with a new version of the patch +as described in :ref:`Pull Requests Criteria <pull-requests-criteria>`. + +Command to get approved PRs is ``gh pr list --json number,reviewDecision --search +"state:open type:pr -review:none" | jq '.[] | select(.reviewDecision=="")'`` + +Web UI filter does not work cf https://github.com/orgs/community/discussions/55826 + +Once in approved state, the PRs are in the responsibility of the merger, along +with the next branches/PRs. diff --git a/doc/userguide/devguide/codebase/contributing/index.rst b/doc/userguide/devguide/codebase/contributing/index.rst new file mode 100644 index 0000000..e0d2912 --- /dev/null +++ b/doc/userguide/devguide/codebase/contributing/index.rst @@ -0,0 +1,9 @@ +Contributing +============ + +.. toctree:: + :maxdepth: 2 + + contribution-process + code-submission-process + github-pr-workflow diff --git a/doc/userguide/devguide/codebase/fuzz-testing.rst b/doc/userguide/devguide/codebase/fuzz-testing.rst new file mode 100644 index 0000000..cd8b6ed --- /dev/null +++ b/doc/userguide/devguide/codebase/fuzz-testing.rst @@ -0,0 +1,31 @@ +Fuzz Testing +============ + +To enable fuzz targets compilation, add `--enable-fuzztargets` to configure. + +.. note:: This changes various parts of Suricata making the `suricata` binary + unsafe for production use. + +The targets can be used with libFuzzer, AFL and other fuzz platforms. + + +Running the Fuzzers +------------------- + +TODO. For now see src/tests/fuzz/README + +Reproducing issues +^^^^^^^^^^^^^^^^^^ + + +Extending Coverage +------------------ + +Adding Fuzz Targets +------------------- + + +Oss-Fuzz +-------- + +Suricata is continuously fuzz tested in Oss-Fuzz. See https://github.com/google/oss-fuzz/tree/master/projects/suricata diff --git a/doc/userguide/devguide/codebase/img/InputCaptureExample.png b/doc/userguide/devguide/codebase/img/InputCaptureExample.png Binary files differnew file mode 100644 index 0000000..7fc3519 --- /dev/null +++ b/doc/userguide/devguide/codebase/img/InputCaptureExample.png diff --git a/doc/userguide/devguide/codebase/index.rst b/doc/userguide/devguide/codebase/index.rst new file mode 100644 index 0000000..f6cb955 --- /dev/null +++ b/doc/userguide/devguide/codebase/index.rst @@ -0,0 +1,13 @@ +Working with the Codebase +========================= + +.. toctree:: + :maxdepth: 2 + + contributing/index.rst + installation-from-git + code-style + fuzz-testing + testing + unittests-c + unittests-rust diff --git a/doc/userguide/devguide/codebase/installation-from-git.rst b/doc/userguide/devguide/codebase/installation-from-git.rst new file mode 100644 index 0000000..9d7a45a --- /dev/null +++ b/doc/userguide/devguide/codebase/installation-from-git.rst @@ -0,0 +1,154 @@ +.. _Installation from GIT: + +Installation from GIT +===================== + +Ubuntu Installation from GIT +---------------------------- + +This document will explain how to install and use the most recent code of +Suricata on Ubuntu. Installing from GIT on other operating systems is +basically the same, except that some commands are Ubuntu-specific +(like sudo and apt-get). In case you are using another operating system, +you should replace those commands with your OS-specific commands. + +.. note:: + + These instructions were tested on Ubuntu 22.04. + +Pre-installation requirements +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Before you can build Suricata for your system, run the following command +to ensure that you have everything you need for the installation. + +.. code-block:: bash + + sudo apt-get -y install libpcre2-dev build-essential autoconf \ + automake libtool libpcap-dev libnet1-dev libyaml-0-2 libyaml-dev \ + pkg-config zlib1g zlib1g-dev libcap-ng-dev libcap-ng0 make \ + libmagic-dev libjansson-dev rustc cargo jq git-core + +Add ``${HOME}/.cargo/bin`` to your path: + +.. code-block:: bash + + export PATH=$PATH:${HOME}/.cargo/bin + cargo install --force cbindgen + +Depending on the current status of your system, it may take a while to +complete this process. + +**IPS** + +By default, Suricata works as an IDS. If you want to use it as an IDS and IPS +program, enter: + +.. code-block:: bash + + sudo apt-get -y install libnetfilter-queue-dev libnetfilter-queue1 \ + libnfnetlink-dev libnfnetlink0 + +Suricata +~~~~~~~~ + +First, it is convenient to create a directory for Suricata. +Name it 'suricata' or 'oisf', for example. Open the terminal and enter: + +.. code-block:: bash + + mkdir suricata # mkdir oisf + +Followed by: + +.. code-block:: bash + + cd suricata # cd oisf + +Next, enter the following line in the terminal: + +.. code-block:: bash + + git clone https://github.com/OISF/suricata.git + cd suricata + +Libhtp and suricata-update are not bundled. Get them by doing: + +.. code-block:: bash + + ./scripts/bundle.sh + +Followed by: + +.. code-block:: bash + + ./autogen.sh + +To configure, please enter: + +.. code-block:: bash + + ./configure + +To compile, please enter: + +.. code-block:: bash + + make + +To install Suricata, enter: + +.. code-block:: bash + + sudo make install + sudo ldconfig + +Auto-setup +~~~~~~~~~~ + +You can also use the available auto-setup features of Suricata. Ex: + +.. code-block:: bash + + ./configure && make && sudo make install-conf + +*make install-conf* +would do the regular "make install" and then it would automatically +create/setup all the necessary directories and ``suricata.yaml`` for you. + +.. code-block:: bash + + ./configure && make && make install-rules + +*make install-rules* +would do the regular "make install" and then it would automatically download +and set-up the latest ruleset from Emerging Threats available for Suricata. + +.. code-block:: bash + + ./configure && make && make install-full + +*make install-full* +would combine everything mentioned above (install-conf and install-rules) - +and will present you with a ready to run (configured and set-up) Suricata. + +Post installation +~~~~~~~~~~~~~~~~~ + +Please continue with :ref:`Basic setup`. + +In case you have already created your Suricata directory and cloned the +repository in it, if you want to update your local repository with the +most recent code, please run: + +.. code-block:: bash + + cd suricata/suricata + +next, enter: + +.. code-block:: bash + + git pull + +After that, you should run *./autogen.sh* again. diff --git a/doc/userguide/devguide/codebase/testing.rst b/doc/userguide/devguide/codebase/testing.rst new file mode 100644 index 0000000..c712e90 --- /dev/null +++ b/doc/userguide/devguide/codebase/testing.rst @@ -0,0 +1,158 @@ +**************** +Testing Suricata +**************** + +.. contents:: Table of Contents + +General Concepts +================ + +There are a few ways of testing Suricata: + +- **Unit tests**: for independently checking specific functions or portions of code. This guide has specific sections to + further explain those, for C and Rust; +- `Suricata-Verify <https://github.com/OISF/suricata-verify>`_: those are used to check more complex behavior, like the log output or the alert counts for a given input, where that input is usually comprised of several packets; +- **Static and dynamic analysis tools**: to help in finding bugs, memory leaks and other issues (like `scan-build <https://clang-analyzer.llvm.org/scan-build.html#scanbuild_basicusage>`_, from `clang`, which is also used for our C formatting checks; or ASAN, which checks for memory issues); +- **Fuzz testing**: especially good for uncovering existing, often non-trivial bugs. For more on how to fuzz test Suricata, check :doc:`fuzz-testing`; +- **CI checks**: each PR submitted to the project's public repositories will be run against a suit of Continuous Integration + workflows, as part of our QA process. Those cover: formatting and commit checks; fuzz tests (CI Fuzz), and several builds. See our `github workflows <https://github.com/OISF/suricata/tree/master/.github/workflows>`_ for details and those in + action at `<https://github.com/OISF/suricata/actions>`_. + + .. note:: If you can run unit tests or other checks and report failures in our `issue tracker <https://redmine.openinfosecfoundation.org/projects/suricata/issues>`_, that is rather useful and appreciated! + +The focus of this document are Unit tests and Suricata-Verify tests, especially on offering some guidance regarding when to use each type of test, and how to prepare input +for them. + +Unit tests +========== + +Use these to check that specific functions behave as expected, in success and in failure scenarios. Specially useful +during development, for nom parsers in the Rust codebase, for instance, or for checking that messages +or message parts of a protocol/stream are processed as they should. + +To execute all unit tests (both from C and Rust code), as well as ``libhtp`` ones, from the Suricata main directory, run:: + + make check + +Check the Suricata Devguide on :doc:`unittests-c` or :doc:`unittests-rust` for more on how to write and run unit tests, +given that the way to do so differs, depending on the language. + +Code Examples +^^^^^^^^^^^^^ + +An example from the `DNS parser <https://github.com/OISF/suricata/blob/master/rust/src/dns/parser.rs#L417>`_. This +checks that the given raw input (note the comments indicating what it means), once processed by ``dns_parse_name`` yields +the expected result, including the unparsed portion. + +.. code-block:: rust + + /// Parse a simple name with no pointers. + #[test] + fn test_dns_parse_name() { + let buf: &[u8] = &[ + 0x09, 0x63, /* .......c */ + 0x6c, 0x69, 0x65, 0x6e, 0x74, 0x2d, 0x63, 0x66, /* lient-cf */ + 0x07, 0x64, 0x72, 0x6f, 0x70, 0x62, 0x6f, 0x78, /* .dropbox */ + 0x03, 0x63, 0x6f, 0x6d, 0x00, 0x00, 0x01, 0x00, /* .com.... */ + ]; + let expected_remainder: &[u8] = &[0x00, 0x01, 0x00]; + let (remainder,name) = dns_parse_name(buf, buf).unwrap(); + assert_eq!("client-cf.dropbox.com".as_bytes(), &name[..]); + assert_eq!(remainder, expected_remainder); + } + +From the C side, ``decode-ethernet.c`` offers an good example: + +.. code-block:: c + + /** + * Test a DCE ethernet frame that is too small. + */ + static int DecodeEthernetTestDceTooSmall(void) + { + uint8_t raw_eth[] = { + 0x00, 0x10, 0x94, 0x55, 0x00, 0x01, 0x00, 0x10, + 0x94, 0x56, 0x00, 0x01, 0x89, 0x03, + }; + + Packet *p = PacketGetFromAlloc(); + FAIL_IF_NULL(p); + ThreadVars tv; + DecodeThreadVars dtv; + + memset(&dtv, 0, sizeof(DecodeThreadVars)); + memset(&tv, 0, sizeof(ThreadVars)); + + DecodeEthernet(&tv, &dtv, p, raw_eth, sizeof(raw_eth)); + + FAIL_IF_NOT(ENGINE_ISSET_EVENT(p, DCE_PKT_TOO_SMALL)); + + PacketFree(p); + PASS; + } + +Suricata-Verify +=============== + +As mentioned above, these tests are used to check more complex behavior that involve a complete flow, with exchange of requests and responses. This can be done in an easier and more straightforward way, +since one doesn't have to simulate the network traffic and Suricata engine mechanics - one simply runs it, with the desired input packet capture, +configuration and checks. + +A Suricata-verify test can help to ensure that code refactoring doesn't affect protocol logs, or signature detection, +for instance, as this could have a major impact to Suricata users and integrators. + +For simpler tests, providing the pcap input is enough. But it is also possible to provide Suricata rules to be +inspected, and have Suricata Verify match for alerts and specific events. + +Refer to the `Suricata Verify readme <https://github.com/OISF/suricata-verify#readme>`_ for details on how to create +this type of test. It suffices to have a packet capture representative of the behavior one wants to test, and then +follow the steps described there. + +The Git repository for the Suricata Verify tests is a great source for examples, like the `app-layer-template <https://github.com/OISF/suricata-verify/tree/master/tests/app-layer-template>`_ one. + +Generating Input +================ + +Using real traffic +^^^^^^^^^^^^^^^^^^ + +Having a packet capture for the desired protocol you want to test, open it in `Wireshark <https://www.wireshark.org/>`_, and select the specific +packet chosen for the test input, then use the Wireshark option ``Follow [TCP/UDP/HTTP/HTTP2/QUIC] Stream``. This allows for inspecting the whole network traffic stream in a different window. +There, it's possible to choose to ``Show and save data as`` ``C Arrays``, as well as to select if one wants to see the whole conversation or just **client** or **server** packets. +It is also possible to reach the same effect by accessing the **Analyze->Follow->TCP Stream** top menu in Wireshark. +(There are other stream options, the available one will depend on the type of network traffic captured). + +This option will show the packet data as hexadecimal compatible with C-array style, and easily adapted for Rust, +as well. As shown in the image: + +.. image:: img/InputCaptureExample.png + +Wireshark can be also used to `capture sample network traffic <https://gitlab.com/wireshark/wireshark/-/wikis/CaptureSetup>`_ and generate pcap files. + +Crafting input samples with Scapy +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +It is also possible to use Scapy to create specific traffic: `Scapy usage +<https://scapy.readthedocs.io/en/latest/usage.html>`_ + +Suricata-verify tests have several examples of pcaps generated in such a way. Look for Python scripts like the one used +for the `dce-udp-scapy +<https://github.com/OISF/suricata-verify/blob/master/tests/dcerpc/dcerpc-udp-scapy/dcerpc_udp_scapy.py>`_. + +Other examples from our Suricata-Verify tests: +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +Going through Suricata-Verify tests `readme` files it is also possible to find an assorted collection of pcap generation possibilities, some with explanation on the how-tos. To list a few: + +- `http2-range <https://github.com/OISF/suricata-verify/blob/master/tests/http2-range/README.md>`_ +- `http-range <https://github.com/OISF/suricata-verify/blob/master/tests/http-range/README.md>`_ +- `smb2-delete <https://github.com/OISF/suricata-verify/blob/master/tests/smb2-delete/README.md>`_ +- `smtp-rset <https://github.com/OISF/suricata-verify/blob/master/tests/smtp-rset/README.md>`_ +- `http-auth-unrecognized <https://github.com/OISF/suricata-verify/blob/master/tests/http-auth-unrecognized/README.md>`_ + +Finding Capture Samples +^^^^^^^^^^^^^^^^^^^^^^^ + +If you can't capture traffic for the desired protocol from live traffic, or craft something up, you can try finding the type of traffic you +are interested in in public data sets. There's a thread for `Sharing good sources of sample captures +<https://forum.suricata.io/t/sharing-good-sources-of-sample-captures/1766/4>`_ in our forum. diff --git a/doc/userguide/devguide/codebase/unittests-c.rst b/doc/userguide/devguide/codebase/unittests-c.rst new file mode 100644 index 0000000..0ae7bdf --- /dev/null +++ b/doc/userguide/devguide/codebase/unittests-c.rst @@ -0,0 +1,148 @@ +************** +Unit Tests - C +************** + +Unit tests are a great way to create tests that can check the internal state +of parsers, structures and other objects. + +Tests should: + +- use ``FAIL``/``PASS`` macros +- be deterministic +- not leak memory on ``PASS`` +- not use conditions + +Unit tests are used by developers of Suricata and advanced users who would like to contribute by debugging and testing the engine. +Unit tests are small pieces (units) of code which check certain code functionalities in Suricata. If Suricata's code is modified, developers can run unit tests to see if there are any unforeseen effects on other parts of the engine's code. +Unit tests will not be compiled with Suricata by default. +If you would like to compile Suricata with unit tests, enter the following during the configure-stage:: + + ./configure --enable-unittests + +The unit tests specific command line options can be found at `Command Line Options <https://docs.suricata.io/en/suricata-6.0.3/command-line-options.html#unit-tests>`_. + +Example: +You can run tests specifically on flowbits. This is how you should do that:: + + suricata -u -U flowbit + +It is highly appreciated if you would run unit tests and report failing tests in our `issue tracker +<https://redmine.openinfosecfoundation.org/projects/suricata/issues>`_. + +If you want more info about the unittests, regular debug mode can help. This is enabled by adding the configure option:: + + --enable-debug + +Then, set the debug level from the command-line:: + + SC_LOG_LEVEL=Debug suricata -u + +This will be very verbose. You can also add the ``SC_LOG_OP_FILTER`` to limit the output, it is grep-like:: + + SC_LOG_LEVEL=Debug SC_LOG_OP_FILTER="(something|somethingelse)" suricata -u + +This example will show all lines (debug, info, and all other levels) that contain either something or something else. +Keep in mind the `log level <https://docs.suricata.io/en/latest/manpages/suricata.html#id1>`_ precedence: if you choose *Info* level, for instance, Suricata won't show messages from the other levels. + +Writing Unit Tests - C codebase +=============================== + +Suricata unit tests are somewhat different in C and in Rust. In C, they are comprised of a function with no arguments and returning 0 for failure or 1 for success. Instead of explicitly returning a value, FAIL_* and PASS macros should be used. For example: + +.. code-block:: c + + void MyUnitTest(void) + { + int n = 1; + void *p = NULL; + + FAIL_IF(n != 1); + FAIL_IF_NOT(n == 1); + FAIL_IF_NOT_NULL(p); + FAIL_IF_NULL(p); + + PASS; + } + +Each unit test needs to be registered with ``UtRegisterTest()``. Example:: + + UtRegisterTest("MyUnitTest", MyUnitTest); + +where the first argument is the name of the test, and the second argument is the function. Existing modules should already have a function that registers its unit tests. Otherwise the unit tests will need to be registered. Look for a module similar to your new module to see how best to register the unit tests or ask the development team for help. + +Examples +-------- + +From ``conf-yaml-loader.c``: + +.. code-block:: c + + /** + * Test that a configuration section is overridden but subsequent + * occurrences. + */ + static int + ConfYamlOverrideTest(void) + { + char config[] = + "%YAML 1.1\n" + "---\n" + "some-log-dir: /var/log\n" + "some-log-dir: /tmp\n" + "\n" + "parent:\n" + " child0:\n" + " key: value\n" + "parent:\n" + " child1:\n" + " key: value\n" + ; + const char *value; + + ConfCreateContextBackup(); + ConfInit(); + + FAIL_IF(ConfYamlLoadString(config, strlen(config)) != 0); + FAIL_IF_NOT(ConfGet("some-log-dir", &value)); + FAIL_IF(strcmp(value, "/tmp") != 0); + + /* Test that parent.child0 does not exist, but child1 does. */ + FAIL_IF_NOT_NULL(ConfGetNode("parent.child0")); + FAIL_IF_NOT(ConfGet("parent.child1.key", &value)); + FAIL_IF(strcmp(value, "value") != 0); + + ConfDeInit(); + ConfRestoreContextBackup(); + + PASS; + } + +In ``detect-ike-chosen-sa.c``, it is possible to see the freeing of resources (``DetectIkeChosenSaFree``) and the +function that should group all the ``UtRegisterTest`` calls: + +.. code-block:: c + + #ifdef UNITTESTS + . + . + . + static int IKEChosenSaParserTest(void) + { + DetectIkeChosenSaData *de = NULL; + de = DetectIkeChosenSaParse("alg_hash=2"); + + FAIL_IF_NULL(de); + FAIL_IF(de->sa_value != 2); + FAIL_IF(strcmp(de->sa_type, "alg_hash") != 0); + + DetectIkeChosenSaFree(NULL, de); + PASS; + } + + #endif /* UNITTESTS */ + + void IKEChosenSaRegisterTests(void) + { + #ifdef UNITTESTS + UtRegisterTest("IKEChosenSaParserTest", IKEChosenSaParserTest); + #endif /* UNITTESTS */ diff --git a/doc/userguide/devguide/codebase/unittests-rust.rst b/doc/userguide/devguide/codebase/unittests-rust.rst new file mode 100644 index 0000000..c11d6e4 --- /dev/null +++ b/doc/userguide/devguide/codebase/unittests-rust.rst @@ -0,0 +1,91 @@ +***************** +Unit tests - Rust +***************** + +Rust tests with Cargo check +=========================== + +Rust offers a built-in tool for running unit and integration tests. To do so, one makes usage of: + +.. code-block:: rust + + cargo test [options][testname][-- test-options] + +`The Cargo Book <https://doc.rust-lang.org/cargo/commands/cargo-test.html>`_ explains all options in more detail. + +For testing a specific Rust module from Suricata, it suffices to go to the ``rust`` directory and run the above command, +specifying the desired module (like ``http2``). + +.. code-block:: rust + + cargo test http2 + +The line above will make *rustc* compile the Rust side of Suricata and run unit tests in the ``http2`` rust module. + +For running all Suricata unit tests from our Rust codebase, just run ``cargo test``. + +Adding unit tests +================= + + .. note:: If you want to understand *when* to use a unit test, please read the devguide section on :doc:`testing`. + +In general, it is preferable to have the unit tests in the same file that they test. At the end of the file, after all other functions. Add a ``tests`` module, if there isn't one yet, and add the ``#[test]`` attribute before the unit test +function. It is also necessary to import (``use``) the module to test, as well as any other modules used. As seen in the example below: + +Example +------- + +From ``nfs > rpc_records.rs``: + +.. code-block:: rust + + mod tests { + use crate::nfs::rpc_records::*; + use nom::Err::Incomplete; + use nom::Needed::Size; + + #[test] + fn test_partial_input_ok() { + let buf: &[u8] = &[ + 0x80, 0x00, 0x00, 0x9c, // flags + 0x8e, 0x28, 0x02, 0x7e, // xid + 0x00, 0x00, 0x00, 0x01, // msgtype + 0x00, 0x00, 0x00, 0x02, // rpcver + 0x00, 0x00, 0x00, 0x03, // program + 0x00, 0x00, 0x00, 0x04, // progver + 0x00, 0x00, 0x00, 0x05, // procedure + ]; + let expected = RpcRequestPacketPartial { + hdr: RpcPacketHeader { + frag_is_last: true, + frag_len: 156, + xid: 2384986750, + msgtype: 1 + }, + rpcver: 2, + program: 3, + progver: 4, + procedure: 5 + }; + let r = parse_rpc_request_partial(buf); + match r { + Ok((rem, hdr)) => { + assert_eq!(rem.len(), 0); + assert_eq!(hdr, expected); + }, + _ => { panic!("failed {:?}",r); } + } + } + } + +Once that is done, Rust should recognize the new test. If you want to check a single test, run:: + + cargo test module::file_name::tests::test_name + +Where ``tests`` refers to ``mod tests``. If you know the test name is unique, you can even run:: + + cargo test test_name + +Following the same idea, it is also possible to test specific modules or submodules. For instance:: + + cargo test nfs::rpc_records |