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

mqtt: Improve handling of multiple PDU parsing #10262

Closed
wants to merge 1 commit into from

Conversation

jlucovsky
Copy link
Contributor

Continuation of #10236

Issue: 6592

Link to redmine ticket: 6592

Describe changes:

  • Parse PDU instead of entire stream

Updates:

  • Reflect remaining byte count (per review comment)
  • s-v test pr update

Provide values to any of the below to override the defaults.

To use a pull request use a branch name like pr/N where N is the
pull request number.

Alternatively, SV_BRANCH may also be a link to an
OISF/suricata-verify pull-request.

SV_REPO=
SV_BRANCH=pr/1609
SU_REPO=
SU_BRANCH=
LIBHTP_REPO=
LIBHTP_BRANCH=

@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 17783

Copy link

codecov bot commented Jan 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c3b3c11) 82.28% compared to head (a3612eb) 82.32%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #10262      +/-   ##
==========================================
+ Coverage   82.28%   82.32%   +0.03%     
==========================================
  Files         977      977              
  Lines      271950   271950              
==========================================
+ Hits       223784   223885     +101     
+ Misses      48166    48065     -101     
Flag Coverage Δ
fuzzcorpus 63.52% <100.00%> (+0.12%) ⬆️
suricata-verify 61.50% <100.00%> (-0.03%) ⬇️
unittests 62.82% <0.00%> (ø)

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

@glongo
Copy link
Contributor

glongo commented Jan 29, 2024

According to Victor's suggestion (#10058 (comment)):

the frame should be created earlier, and not depend on the parser returning ok. It should be created as soon as we start parsing the data, and then be updated with the length when it's complete

For reference, you can look here for reference:

7366506#diff-d988869639f4f6ed35c7047322b592a33a7421d22c38aa6137d456967b3656d3R162-R200

match parse_message(current, self.protocol_version, self.max_msg_len) {
Ok((rem, msg)) => {
let _pdu = Frame::new(
flow,
&stream_slice,
input,
current,
current.len() as i64,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it not be current.len()-rem.len() ? (and same below)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will adjust in new pr

@jlucovsky
Copy link
Contributor Author

Continued in #10279

@jlucovsky jlucovsky closed this Jan 30, 2024
@jlucovsky jlucovsky deleted the 6592/3 branch April 24, 2024 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants