Mqtt rust keywords 4863 v3 #11430

catenacyber · 2024-07-04T14:46:26Z

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

Describe changes:

mqtt: move keywords to pure rust

SV_BRANCH=OISF/suricata-verify#1920

#11374 with needed rebase

@satta approved last version 👍

THashInitConfig may not allocate array and increase memuse. Such a failure leads to THashShutdown which should not decrease the memuse. Ticket: 7135

So that MQTTTypeCode::CONNECT does not become c_o_n_n_e_c_t

As needed for MQTTTypeCode which accepts both CONNECT uppercase and unassigned lowercase

for easier later matching

Ticket: 4863 On the way, convert some keywords to use the first-class integer support. And imports in pure rust the support for multi-buffer.

There was a typo that version 3 was tested twice

as is done for other keywords

codecov · 2024-07-04T18:25:54Z

Codecov Report

Attention: Patch coverage is 87.98283% with 140 lines in your changes missing coverage. Please review.

Project coverage is 82.41%. Comparing base (a7af371) to head (6487ea5).
Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #11430      +/-   ##
==========================================
- Coverage   82.44%   82.41%   -0.03%     
==========================================
  Files         938      922      -16     
  Lines      248086   247933     -153     
==========================================
- Hits       204534   204343     -191     
- Misses      43552    43590      +38

Flag	Coverage Δ
fuzzcorpus	`60.14% <85.51%> (+0.04%)`	⬆️
livemode	`18.76% <33.02%> (+0.04%)`	⬆️
pcap	`43.71% <34.54%> (-0.07%)`	⬇️
suricata-verify	`61.39% <79.12%> (-0.03%)`	⬇️
unittests	`59.39% <54.93%> (-0.05%)`	⬇️

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

suricata-qa · 2024-07-04T20:25:03Z

Information: QA ran without warnings.

Pipeline 21374

victorjulien · 2024-07-05T12:12:23Z

rust/src/detect/mod.rs

@@ -105,6 +105,44 @@ extern {
    ) -> *mut c_void;
 }

+// needed for calls to DetectAppLayerMultiRegister
+pub const SIG_FLAG_TOSERVER: u32 = 0x80000; // BIT_U32(19)


this is an accident waiting to happen, can we get to a single place to define it?

Removing the need for it in rust, by using a helper function as for regular single buffers

victorjulien · 2024-07-05T12:13:20Z

rust/src/mqtt/detect.rs

-pub unsafe extern "C" fn rs_mqtt_tx_get_connect_clientid(
-    tx: &MQTTTransaction, buffer: *mut *const u8, buffer_len: *mut u32,
-) -> u8 {
+unsafe extern "C" fn mqtt_tx_get_connect_clientid(


style: function naming

@jasonish how do I prevent this function from being exported in headers ?

Sorry, they are not exported.

Victor, how should I name these internal rust functions ?

cf

warning: function `SCMqttGetConnectClientId` should have a snake case name --> src/mqtt/detect.rs:56:22 | 56 | unsafe extern "C" fn SCMqttGetConnectClientId( | ^^^^^^^^^^^^^^^^^^^^^^^^ help: convert the identifier to snake case: `scmqtt_get_connect_client_id` | = note: `#[warn(non_snake_case)]` on by default

Why are they unsafe extern "C" if not exported?

Because they are callbacks, called from C

That makes them exported, right?

No, if only used via function pointer from the same file, they are not exported. No need to follow C naming conventions as they do not live in that name space. pub extern is when you enter the C name space.

I see, thanks for the explanation. So for these we can use the rust style then, agree?

I see, thanks for the explanation. So for these we can use the rust style then, agree?

Yes, and so does clippy.. If pub extern it doesn't complain about style, but if just extern C it will. It also means you can avoid the stutter, name it like a pure rust function instead of long prefixes if you like.

victorjulien · 2024-07-05T12:15:45Z

rust/src/mqtt/detect.rs

-pub unsafe extern "C" fn rs_mqtt_tx_get_subscribe_topic(
-    tx: &MQTTTransaction, i: u32, buf: *mut *const u8, len: *mut u32,
-) -> u8 {
+static mut UNSUB_TOPIC_MATCH_LIMIT: isize = 100;


why all these all uppercase? Can just be styled like regular globals.

warning: static variable Limit should have an upper case name
--> src/mqtt/detect.rs:263:12
|
263 | static mut Limit: isize = 100;
| ^^^^^ help: convert the identifier to upper case: LIMIT
|
= note: #[warn(non_upper_case_globals)] on by default

Hmm I see. I don't really like that all uppercase has a very different meaning in our C code (constant / macro) and that we're starting to mix these things too.

@jasonish some thoughts here ?

Generally I'm in favor of the style promoted by the language as it just makes things easier for everyone IMO.

It's not really a warning I'd want to suppress globally. If it was a constant we'd probably want it upper case.

victorjulien · 2024-07-05T12:18:01Z

src/tests/detect-mqtt.c

+ *  \retval 1 on success
+ *  \retval 0 on failure
+ */
+static int MQTTProtocolVersionTestParse01(void)


these could all be SV tests, it seems

Why ?

This is just signatures parsing.
It could rather be unit tests of the parsing function, which is just the generic integer one already tested...

Also, the keyword is already tested in SV with mqtt-pub-rules and mqtt-unsub-rules

the tests only test that the rules parse w/o error, nothing internal is checked. So it's not really unittesting.

I think the intent was the parsing of the keyword parameters... Proposing something in next version

victorjulien

Nice work. Some comments inline. Function naming comment applies to much of the PR.

catenacyber · 2024-07-10T12:04:07Z

Continued in #11467

catenacyber added 8 commits July 4, 2024 13:58

util/thash: decrease memuse if array was allocated

e4c20d0

THashInitConfig may not allocate array and increase memuse. Such a failure leads to THashShutdown which should not decrease the memuse. Ticket: 7135

rust/derive: transform all uppercase names the right way

6cfb5b8

So that MQTTTypeCode::CONNECT does not become c_o_n_n_e_c_t

rust/derive: string enumeration become case insensitive

6b350ae

As needed for MQTTTypeCode which accepts both CONNECT uppercase and unassigned lowercase

mqtt: parse and store raw connect flags

e27aaa4

for easier later matching

detect/mqtt: move keywords to rust

fa070b5

Ticket: 4863 On the way, convert some keywords to use the first-class integer support. And imports in pure rust the support for multi-buffer.

mqtt: test version 5

73f2629

There was a typo that version 3 was tested twice

mqtt/detect: iterate all tx's messages for reason code

791e5f8

as is done for other keywords

mqtt: detection on empty topics

6487ea5

catenacyber requested review from jasonish, jufajardini and victorjulien as code owners July 4, 2024 14:46

catenacyber mentioned this pull request Jul 4, 2024

Mqtt rust keywords 4863 v2 #11374

Closed

victorjulien reviewed Jul 5, 2024

View reviewed changes

victorjulien requested changes Jul 5, 2024

View reviewed changes

catenacyber mentioned this pull request Jul 10, 2024

Mqtt rust keywords 4863 v4 #11467

Closed

catenacyber closed this Jul 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mqtt rust keywords 4863 v3 #11430

Mqtt rust keywords 4863 v3 #11430

catenacyber commented Jul 4, 2024

codecov bot commented Jul 4, 2024 •

edited

Loading

suricata-qa commented Jul 4, 2024

victorjulien Jul 5, 2024

catenacyber Jul 10, 2024

victorjulien Jul 5, 2024

catenacyber Jul 10, 2024

catenacyber Jul 10, 2024

catenacyber Jul 10, 2024

victorjulien Jul 10, 2024

catenacyber Jul 10, 2024

victorjulien Jul 10, 2024

jasonish Jul 10, 2024

victorjulien Jul 10, 2024

jasonish Jul 10, 2024

victorjulien Jul 5, 2024

catenacyber Jul 10, 2024

victorjulien Jul 10, 2024

catenacyber Jul 10, 2024

jasonish Jul 11, 2024

victorjulien Jul 5, 2024

catenacyber Jul 10, 2024

catenacyber Jul 10, 2024

victorjulien Jul 10, 2024

catenacyber Jul 10, 2024

victorjulien left a comment

catenacyber commented Jul 10, 2024

Mqtt rust keywords 4863 v3 #11430

Mqtt rust keywords 4863 v3 #11430

Conversation

catenacyber commented Jul 4, 2024

codecov bot commented Jul 4, 2024 • edited Loading

Codecov Report

suricata-qa commented Jul 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

victorjulien left a comment

Choose a reason for hiding this comment

catenacyber commented Jul 10, 2024

codecov bot commented Jul 4, 2024 •

edited

Loading