fix(rumqtt): validate topic and topic filter #813

arunanshub · 2024-03-06T10:50:58Z

Type of change

Bug fix (non-breaking change which fixes an issue)

Checklist:

Formatted with cargo fmt
Make an entry to CHANGELOG.md if it's relevant to the users of the library. If it's not relevant mention why.

Fixes #595

swanandx · 2024-03-06T10:55:18Z

note that it won't fix #595 as broker still doesn't perform the checks, and doing that in separate PR is preferable. I would recommend to remove that from PR description so it won't automatically close the issue if this PR is merged.

arunanshub · 2024-03-06T10:59:42Z

note that it won't fix #595 as broker still doesn't perform the checks

It is still an ongoing work.

Since broker will perform the same task, I will implement it in this PR itself.

coveralls · 2024-03-06T11:12:04Z

Pull Request Test Coverage Report for Build 8519279552

Details

45 of 164 (27.44%) changed or added relevant lines in 13 files are covered.
No unchanged relevant lines lost coverage.
Overall first build on fix/validate-topic at 36.493%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
rumqttc/src/v5/mqttbytes/mod.rs	20	21	95.24%
rumqttd/src/protocol/v4/publish.rs	0	3	0.0%
rumqttd/src/protocol/v4/unsubscribe.rs	0	3	0.0%
rumqttd/src/protocol/v5/publish.rs	0	3	0.0%
rumqttd/src/protocol/v5/subscribe.rs	0	3	0.0%
rumqttd/src/protocol/v5/unsubscribe.rs	0	3	0.0%
rumqttc/src/v5/mqttbytes/v5/subscribe.rs	0	4	0.0%
rumqttd/src/protocol/v4/subscribe.rs	0	4	0.0%
rumqttc/src/mqttbytes/v4/subscribe.rs	0	5	0.0%
rumqttd/src/protocol/mod.rs	1	28	3.57%

Totals
Change from base Build 8432798356:	36.5%
Covered Lines:	6017
Relevant Lines:	16488

💛 - Coveralls

rumqttc/src/client.rs

rumqttc/src/mqttbytes/topic.rs

h3nill · 2024-03-15T14:28:46Z

rumqttc/src/v5/mqttbytes/mod.rs

+    (is_topic_valid || is_topic_empty)
+        && (!is_topic_empty || is_alias_given)
+        && (!is_alias_given || is_alias_valid)


imo this is harder to read in comparison to if...else branch, wdyt @swanandx?

I did some investigation. It seems that apart from the length of the function, the assembly for if-else version of validate_topic_name_and_alias remains the same. How should we proceed?

https://godbolt.org/z/WPrc4P66s

flxo

Thanks for the contribution.

Is there a benchmark somewhere? The topic validation is in the hot path and must not have any crucial performance impact.

arunanshub · 2024-03-28T16:52:46Z

Hello @flxo. The hot paths that you mention use generic functions (eg. impl AsRef<str>). According to this discussion on Reddit, in release builds the generic functions are inlined. Furthermore, according to matklad on his blog, private functions are always inlined. I believe that all of these factors should help with performance.

Also feel free to review the changes and suggest any scope of improvements, should you happen to find any.

flxo · 2024-04-02T06:36:56Z

Hello @flxo. The hot paths that you mention use generic functions (eg. impl AsRef<str>). According to this discussion on Reddit, in release builds the generic functions are inlined. Furthermore, according to matklad on his blog, private functions are always inlined. I believe that all of these factors should help with performance.

Sure - this all helps. I'm just curious if someone used e.g criterion to verify that best effort is done.

Also feel free to review the changes and suggest any scope of improvements, should you happen to find any.

Sure.

rumqttc/src/mqttbytes/topic.rs

flxo · 2024-04-02T06:40:14Z

rumqttc/src/mqttbytes/topic.rs

@@ -16,8 +37,9 @@ pub fn valid_topic(topic: &str) -> bool {
 /// Checks if the filter is valid
 ///
 /// <https://docs.oasis-open.org/mqtt/mqtt/v3.1.1/os/mqtt-v3.1.1-os.html#_Toc398718106>
-pub fn valid_filter(filter: &str) -> bool {
-    if filter.is_empty() {
+pub fn valid_filter(filter: impl AsRef<str>) -> bool {


I'd name this is_filter_valid

Since it is a public function, we may run into the risk of backwards incompatibility due to name change. What do you think?

rumqttc/src/v5/client.rs

rumqttd/src/protocol/mod.rs

flxo · 2024-04-02T06:47:15Z

rumqttd/src/protocol/v5/publish.rs

@@ -34,6 +35,10 @@ pub fn read(
    bytes.advance(variable_header_index);
    let topic = read_mqtt_bytes(&mut bytes)?;

+    if !valid_topic(from_utf8(&topic).map_err(|_| Error::TopicNotUtf8)?) {


Here the validation is done (again) when obtaining len. This can be a problem when len is used during encoding since the utf8 check is not cheap at all.

Have you thought about a TryFrom impl for all the packet types in here? Validate the parameters once at construction time (and maybe precalculate the length once.

…ing to MQTTv3 spec

…pic filter

…ackets

[skip ci]

…erator

refactor(v5/client): use descriptive variable name

…ghput in hotspots

arunanshub marked this pull request as draft March 6, 2024 10:51

arunanshub marked this pull request as ready for review March 14, 2024 09:50

arunanshub requested a review from swanandx March 15, 2024 08:37

h3nill reviewed Mar 15, 2024

View reviewed changes

arunanshub mentioned this pull request Mar 25, 2024

fix(rumqttc): validate filters for subscribe. #828

Merged

2 tasks

flxo reviewed Mar 28, 2024

View reviewed changes

arunanshub mentioned this pull request Mar 28, 2024

rumqttc: fix subscribe many #837

Closed

2 tasks

arunanshub requested a review from h3nill March 28, 2024 17:08

flxo reviewed Apr 2, 2024

View reviewed changes

arunanshub and others added 16 commits April 2, 2024 15:29

fix(mqttbytes): add function to validate topic name and filter accord…

2a37b52

…ing to MQTTv3 spec

fix(v5/client): validate topic name and alias

33b438c

refactor(mqttbytes): use impl AsRef<str> for function parameters

5b1fcdf

fix: implement topic validation in v3 and v5 client

df194cc

feat(rumqttd/protocol): add helper functions to validate topic and to…

4bae451

…pic filter

feat(rumqttd): add topic name and filter validation for appropriate p…

4ba27a2

…ackets

docs(changelog): mention changes in changelog

090c663

[skip ci]

refactor(mqttbytes/topic): use a better function name

bc438a2

feat(mqttbytes): accept reference to slice of Filters instead of it…

9508de7

…erator

fix(rumqttc/client): remove Clone trait requirement

165514f

perf(rumqttc): avoid creation of temporary vec while validating packet

78d45d4

refactor(rumqttc/client): remove redundant call to into_iter()

68f524a

refactor(rumqttc): use descriptive variable names

719efe8

refactor(v5/client): use descriptive variable name

build(deps): add memchr as dependency

ee01fed

perf(rumqtt): use memchr instead of str.contains for better throu…

8bdec24

…ghput in hotspots

refactor(rumqttd): avoid using unwrap

6d30795

arunanshub force-pushed the fix/validate-topic branch from 1afd63e to 6d30795 Compare April 2, 2024 10:07

de-sh force-pushed the main branch from 1fd3c83 to e63bcab Compare May 29, 2024 17:15

FSMaxB mentioned this pull request Jun 19, 2024

rumqttc: Fix: subscribe_many cannot work as implemented, always causing EmptySubscription error. #882

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(rumqtt): validate topic and topic filter #813

fix(rumqtt): validate topic and topic filter #813

arunanshub commented Mar 6, 2024 •

edited

Loading

swanandx commented Mar 6, 2024 •

edited

Loading

arunanshub commented Mar 6, 2024 •

edited

Loading

coveralls commented Mar 6, 2024 •

edited

Loading

h3nill Mar 15, 2024

arunanshub Apr 3, 2024 •

edited

Loading

flxo left a comment

arunanshub commented Mar 28, 2024

flxo commented Apr 2, 2024

flxo Apr 2, 2024

arunanshub Apr 2, 2024

flxo Apr 2, 2024

fix(rumqtt): validate topic and topic filter #813

Are you sure you want to change the base?

fix(rumqtt): validate topic and topic filter #813

Conversation

arunanshub commented Mar 6, 2024 • edited Loading

Type of change

Checklist:

swanandx commented Mar 6, 2024 • edited Loading

arunanshub commented Mar 6, 2024 • edited Loading

coveralls commented Mar 6, 2024 • edited Loading

Pull Request Test Coverage Report for Build 8519279552

Details

💛 - Coveralls

h3nill Mar 15, 2024

Choose a reason for hiding this comment

arunanshub Apr 3, 2024 • edited Loading

Choose a reason for hiding this comment

flxo left a comment

Choose a reason for hiding this comment

arunanshub commented Mar 28, 2024

flxo commented Apr 2, 2024

flxo Apr 2, 2024

Choose a reason for hiding this comment

arunanshub Apr 2, 2024

Choose a reason for hiding this comment

flxo Apr 2, 2024

Choose a reason for hiding this comment

arunanshub commented Mar 6, 2024 •

edited

Loading

swanandx commented Mar 6, 2024 •

edited

Loading

arunanshub commented Mar 6, 2024 •

edited

Loading

coveralls commented Mar 6, 2024 •

edited

Loading

arunanshub Apr 3, 2024 •

edited

Loading