Add clang tidy #12322

regit · 2024-12-27T08:19:14Z

This patchset adds a .clang-tidy file. It will be used by clangd to provide additional diagnostics in the code.

Some of the warnings especially the ones about using magic number are quite verbose but this may be a good idea to implement fixes to improve the situation.

Note: the forbidden functions part requires clang-20 (so compilation from git) to work

Contribution style:

I have read the contributing guide lines at
https://docs.suricata.io/en/latest/devguide/contributing/contribution-process.html

Our Contribution agreements:

I have signed the Open Information Security Foundation contribution agreement at
https://suricata.io/about/contribution-agreement/ (note: this is only required once)

Changes (if applicable):

I have updated the User Guide (in doc/userguide/) to reflect the changes made
I have updated the JSON schema (in etc/schema.json) to reflect all logging changes
(including schema descriptions)
I have created a ticket at
https://redmine.openinfosecfoundation.org/projects/suricata/issues

Link to ticket: https://redmine.openinfosecfoundation.org/issues/3837

Describe changes:

Add clang-tidy file
Fix a thread safety warning to be able to use concurrency checks

codecov · 2024-12-27T08:40:06Z

Codecov Report

Attention: Patch coverage is 89.18919% with 4 lines in your changes missing coverage. Please review.

Project coverage is 82.44%. Comparing base (bd1b9f6) to head (918fa00).
Report is 3 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #12322      +/-   ##
==========================================
- Coverage   82.49%   82.44%   -0.05%     
==========================================
  Files         912      912              
  Lines      258083   258215     +132     
==========================================
- Hits       212897   212880      -17     
- Misses      45186    45335     +149

Flag	Coverage Δ
fuzzcorpus	`60.41% <32.43%> (-0.08%)`	⬇️
livemode	`19.40% <0.00%> (-0.01%)`	⬇️
pcap	`44.31% <32.43%> (-0.06%)`	⬇️
suricata-verify	`63.17% <89.18%> (-0.03%)`	⬇️
unittests	`58.08% <35.13%> (-0.03%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

suricata-qa · 2024-12-27T10:45:03Z

Information: QA ran without warnings.

Pipeline 24053

regit · 2024-12-27T20:49:43Z

There is a build issue on non Linux. Cooking a fix.

suricata-qa · 2024-12-28T01:10:03Z

WARNING:

field	baseline	test	%
	SURI_TLPW2_autofp_stats_chk
.flow.end.tcp_state.last_ack	0	1	-

Pipeline 24055

victorjulien · 2025-01-10T15:54:30Z

I'm looking at how I can test this, and I'm struggling to make it work w/o getting flooded with warnings.

E.g. trying a single file

clang-tidy --quiet src/suricata.c -- -I. -Isrc -I rust/gen|head
5773 warnings and 20 errors generated.
Error while processing /home/victor/sync/devel/eidps/src/suricata.c.
error: too many errors emitted, stopping now [clang-diagnostic-error]
/home/victor/sync/devel/eidps/src/conf.h:85:1: error: unknown type name 'bool' [clang-diagnostic-error]
   85 | bool ConfNodeHasChildren(const ConfNode *node);
      | ^
/home/victor/sync/devel/eidps/src/flow-queue.h:28:10: warning: circular header file dependency detected while including 'flow.h', please check the include path [misc-header-include-cycle]
   28 | #include "flow.h"
      |          ^
/home/victor/sync/devel/eidps/src/flow.h:537:10: note: 'flow-queue.h' included from here
  537 | #include "flow-queue.h"
      |          ^

regit · 2025-01-10T19:37:37Z

I'm looking at how I can test this, and I'm struggling to make it work w/o getting flooded with warnings.

E.g. trying a single file

clang-tidy --quiet src/suricata.c -- -I. -Isrc -I rust/gen|head
5773 warnings and 20 errors generated.
Error while processing /home/victor/sync/devel/eidps/src/suricata.c.
error: too many errors emitted, stopping now [clang-diagnostic-error]
/home/victor/sync/devel/eidps/src/conf.h:85:1: error: unknown type name 'bool' [clang-diagnostic-error]
   85 | bool ConfNodeHasChildren(const ConfNode *node);
      | ^
/home/victor/sync/devel/eidps/src/flow-queue.h:28:10: warning: circular header file dependency detected while including 'flow.h', please check the include path [misc-header-include-cycle]
   28 | #include "flow.h"
      |          ^
/home/victor/sync/devel/eidps/src/flow.h:537:10: note: 'flow-queue.h' included from here
  537 | #include "flow-queue.h"
      |          ^

I'm not using it in the command line but via the editor. If you have LSP via clangd activated (should be the default), it is going to pick the clang-tidy file without configuration and add some analysis to the file as warning.

victorjulien · 2025-01-10T21:31:48Z

Hmm I would really like to figure out how else it can be made useful. If the clangd method can work, it should also be usable directly somehow.

By adding this file, code analysis by clang-tidy is now available in LSP compatible editor using clangd. The CustomFunctions option that adds Suricata banned functions in the error list is only available with the (at the time of writing) future clang 20. Ticket: 3837

The exit() function is not thread safe and triggers a warning from clang tidy for all FatalError() and FatalErrorAtInit() calls. This patch uses quick_exit instead to only flush the critical IO and not call the static destructors (which are the non thread safe part).

This is generated at compile time so let's ignore.

Direct call to clang-tidy have more checks than through clangd and directly including headers is not in Suricata code style so it triggers a lot of unwanted errors.

clang-tidy did detect the -1 return value was not compatible with TmEcode enum.

It is complaining about circular import that are handled at build time. We have one single warning of the sort so let's silent it.

regit · 2025-01-11T19:38:41Z

Hmm I would really like to figure out how else it can be made useful. If the clangd method can work, it should also be usable directly somehow.

Discovered 2 things:

clang-tidy displays the total number of warnings for the directory
there is some checks used only in clang-tidy and not through clangd

After small fixes on non Suricata accurate checks, we have around 76 warnings in suricata.c.

This is reported by clang-tidy as non standard.

bpf_map_data was allocated in the function and not freed. Found by clang-tidy.

Spotted by clang-tidy.

Patched via `clang-tidy --quiet src/detect-http2.c --fix`

Including a C file is bug prone.

Default format style is not using clang-format so switching to 'file' to use clang-format.

Patch obtained via `clang-tidy --quiet src/reputation.c --fix`. On modification has not been applied: reputation.c:326:9: warning: macro 'SREP_SHORTNAME_LEN' defines an integral constant; prefer an enum instead [modernize-macro-to-enum] 326 | #define SREP_SHORTNAME_LEN 32 The change is not really making sense for a single value.

Patch generated by clang-tidy. Most hunks are `else after return` but there is also an unused parameter removed from FTPDataParse() function.

regit · 2025-01-11T21:32:31Z

@victorjulien I've pushed some more commits on top of the MR. I've played a bit with the --fix option of clang-tidy and it produces some interesting patches.

There is some checks that we could disable but I wanted you and the team to potentially take a look at them and take a decision.

One annoying one is unused-parameters. It is useful in some cases but very verbose when there is API usage.
The documentation for the check propose an interesting solution but it needs C23.

This cause clang-tidy to exit with error if we encounter the warning about a potential memory leak.

regit · 2025-01-11T22:00:59Z

I'm stopping playing with it. This is ready for review.

suricata-qa · 2025-01-11T23:50:03Z

Information: QA ran without warnings.

Pipeline 24169

suricata-qa · 2025-01-12T02:10:03Z

Information: QA ran without warnings.

Pipeline 24170

victorjulien · 2025-01-12T08:15:33Z

Most the of the fixes look sane to me, though the linter annotations are a bit obnoxious I think.

How are invoking things? I'm getting warnings like unknown bool type which suggest that I'm missing something basic, like includes not getting picked up.

victorjulien · 2025-01-12T12:31:20Z

Making sure autoconf.h is used (by removing HAVE_CONFIG_H guards) helps a lot for me.

I can see this be very useful, also in CI. So I guess I'd like to see a basic config that passes on our current code (with some fixes if needed). Then we can start an iterative process of adding more checks or making them stricter.

victorjulien · 2025-01-12T12:32:51Z

Wonder also if we can just scan + fix just a single check at a time? E.g. this one would be nice to do now

/home/victor/sync/devel/eidps/src/app-layer-dnp3.c:82:30: warning: macro argument should be enclosed in parentheses [bugprone-macro-parentheses]
   82 | #define DNP3_OBJ_PREFIX(x) ((x >> 4) & 0x7)
      |                              ^
      |                              ()
/home/victor/sync/devel/eidps/src/app-layer-dnp3.c:85:29: warning: macro argument should be enclosed in parentheses [bugprone-macro-parentheses]
   85 | #define DNP3_OBJ_RANGE(x)  (x & 0xf)
      |                             ^
      |                             ()

victorjulien · 2025-01-12T12:33:24Z

Similar for this class of suggestions

/home/victor/sync/devel/eidps/src/app-layer-dnp3.c:343:5: warning: do not use 'else' after 'return' [readability-else-after-return]
  343 |     else {
      |     ^~~~~~
  344 |         return (blocks * DNP3_BLOCK_SIZE);
      |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  345 |     }
      |     ~

victorjulien · 2025-01-12T12:36:38Z

Also liking

rust/gen/rust-bindings.h:6727:13: warning: function 'HTTP2MimicHttp1Request' has a definition with different parameter names [readability-inconsistent-declaration-parameter-name]
 6727 | extern void HTTP2MimicHttp1Request(void *orig_state, void *new_state);
      |             ^
/home/victor/sync/devel/eidps/src/app-layer-http2.c:74:6: note: the definition seen here
   74 | void HTTP2MimicHttp1Request(void *alstate_orig, void *h2s)
      |      ^
rust/gen/rust-bindings.h:6727:13: note: differing parameters are named here: ('orig_state', 'new_state'), in definition: ('alstate_orig', 'h2s')
 6727 | extern void HTTP2MimicHttp1Request(void *orig_state, void *new_state); 
      |             ^                            ~~~~~~~~~~        ~~~~~~~~~
      |                                          alstate_orig      h2s

regit · 2025-01-12T13:34:10Z

Do you have a compile_database.json ? (Can use bear of you don't have). Having it fixes most of the include issue.Le 12 janv. 2025 09:15, Victor Julien ***@***.***> a écrit : Most the of the fixes look sane to me, though the linter annotations are a bit obnoxious I think. How are invoking things? I'm getting warnings like unknown bool type which suggest that I'm missing something basic, like includes not getting picked up. —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: ***@***.***>

regit requested review from victorjulien and a team as code owners December 27, 2024 08:19

regit force-pushed the clang-tidy branch from e949001 to 0132180 Compare December 27, 2024 20:49

regit marked this pull request as draft December 27, 2024 20:49

regit marked this pull request as ready for review January 2, 2025 08:53

regit added 6 commits January 11, 2025 10:08

gitignore: add compile database

ca67296

This is generated at compile time so let's ignore.

devel: silent a clang-tidy warning

e41d299

Direct call to clang-tidy have more checks than through clangd and directly including headers is not in Suricata code style so it triggers a lot of unwanted errors.

af-packet: code cleaning

49e80bc

clang-tidy did detect the -1 return value was not compatible with TmEcode enum.

util/memcp: silent clang-tidy error

d713906

It is complaining about circular import that are handled at build time. We have one single warning of the sort so let's silent it.

regit added 3 commits January 11, 2025 20:54

util/mem: add missing variables to declaration

86f85ac

This is reported by clang-tidy as non standard.

util/ebpf: fix memory leak in error handling

3c03346

bpf_map_data was allocated in the function and not freed. Found by clang-tidy.

util/lua-sandbox: fix duplicate include

f41076e

Spotted by clang-tidy.

regit force-pushed the clang-tidy branch from 0132180 to 935b63c Compare January 11, 2025 21:21

regit added 6 commits January 11, 2025 22:29

detect/http2: fix readability named parameter

c4d79b7

Patched via `clang-tidy --quiet src/detect-http2.c --fix`

detect/http2: remove not directly used include

707f6a5

detect/http2: silent clang-tidy warning

d6e9eb3

Including a C file is bug prone.

clang-tidy: make sure clang format is applied

ae037dd

Default format style is not using clang-format so switching to 'file' to use clang-format.

app-layer/ftp: apply clang-tidy

75eb6fb

Patch generated by clang-tidy. Most hunks are `else after return` but there is also an unused parameter removed from FTPDataParse() function.

regit force-pushed the clang-tidy branch from 935b63c to 75eb6fb Compare January 11, 2025 21:30

clang-tidy: error on Malloc check

918fa00

This cause clang-tidy to exit with error if we encounter the warning about a potential memory leak.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add clang tidy #12322

Add clang tidy #12322

regit commented Dec 27, 2024 •

edited

Loading

codecov bot commented Dec 27, 2024 •

edited

Loading

suricata-qa commented Dec 27, 2024

regit commented Dec 27, 2024

suricata-qa commented Dec 28, 2024

victorjulien commented Jan 10, 2025

regit commented Jan 10, 2025

victorjulien commented Jan 10, 2025

regit commented Jan 11, 2025 •

edited

Loading

regit commented Jan 11, 2025

regit commented Jan 11, 2025

suricata-qa commented Jan 11, 2025

suricata-qa commented Jan 12, 2025

victorjulien commented Jan 12, 2025

victorjulien commented Jan 12, 2025

victorjulien commented Jan 12, 2025

victorjulien commented Jan 12, 2025

victorjulien commented Jan 12, 2025

regit commented Jan 12, 2025 via email

Add clang tidy #12322

Are you sure you want to change the base?

Add clang tidy #12322

Conversation

regit commented Dec 27, 2024 • edited Loading

Contribution style:

Our Contribution agreements:

Changes (if applicable):

codecov bot commented Dec 27, 2024 • edited Loading

Codecov Report

suricata-qa commented Dec 27, 2024

regit commented Dec 27, 2024

suricata-qa commented Dec 28, 2024

victorjulien commented Jan 10, 2025

regit commented Jan 10, 2025

victorjulien commented Jan 10, 2025

regit commented Jan 11, 2025 • edited Loading

regit commented Jan 11, 2025

regit commented Jan 11, 2025

suricata-qa commented Jan 11, 2025

suricata-qa commented Jan 12, 2025

victorjulien commented Jan 12, 2025

victorjulien commented Jan 12, 2025

victorjulien commented Jan 12, 2025

victorjulien commented Jan 12, 2025

victorjulien commented Jan 12, 2025

regit commented Jan 12, 2025 via email

regit commented Dec 27, 2024 •

edited

Loading

codecov bot commented Dec 27, 2024 •

edited

Loading

regit commented Jan 11, 2025 •

edited

Loading