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

dcerpc: convert transaction list to vecdeque for UDP #7756

Closed

Conversation

catenacyber
Copy link
Contributor

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

Describe changes:

  • dcerpc: use VecDeque for the UDP parser (as was already done for the TCP one)

@codecov
Copy link

codecov bot commented Aug 24, 2022

Codecov Report

Merging #7756 (86ee4d7) into master (9353b07) will increase coverage by 0.10%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #7756      +/-   ##
==========================================
+ Coverage   75.98%   76.08%   +0.10%     
==========================================
  Files         661      661              
  Lines      185764   185766       +2     
==========================================
+ Hits       141152   141340     +188     
+ Misses      44612    44426     -186     
Flag Coverage Δ
fuzzcorpus 61.01% <ø> (+0.18%) ⬆️
suricata-verify 52.54% <ø> (+0.02%) ⬆️
unittests 60.70% <ø> (-0.01%) ⬇️

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

self.transactions.push(ntx);
otx = self.transactions.last_mut();
self.transactions.push_back(ntx);
otx = self.transactions.back_mut();
Copy link
Member

Choose a reason for hiding this comment

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

From what I understand, Deque because of it's structure, is good to be used in cases that require pushing and popping from the front. That's what I saw in one of Jason's commit messages as well. However, all I see are too many push_back calls in the code and just one pop_front call (in dns).
So, I'm interested in learning why using Deque helps in all the other protos where it exists (including dcerpc) in terms of efficiency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

VecDeque is good for use when pushing in the back and popping from the front (first in first out)

I think the implementation kind of transforms remove(0) into pop_front that is why we do not see pop_front

This is especially used by AppLayerParserTransactionsCleanup which uses get_tx_iterator

Is it clearer ?

Copy link
Member

Choose a reason for hiding this comment

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

I think the implementation kind of transforms remove(0) into pop_front that is why we do not see pop_front

I see. Could you please point to the part of code where this is happening?

This is especially used by AppLayerParserTransactionsCleanup which uses get_tx_iterator

Is it clearer ?

Not really. Tried to see this part and it was too magical FFI, I suppose. But, I do get the concept. Thank you. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the implementation kind of transforms remove(0) into pop_front that is why we do not see pop_front

I see. Could you please point to the part of code where this is happening?

Line 104 :

self.transactions.remove(index);

self.transactions.remove(index);

@suricata-qa
Copy link

WARNING:

field test baseline %
tlpw2_autofp_stats_chk
.flow.spare 1985105 1812994 109.49%
.flow.memuse 582377728 550537728 105.78%
tlpw1_stats_chk
.tcp.rst 131628 105279 125.03%
generic_stats_chk
.capture.kernel_drops 6148222 5654519 108.73%
.flow.end.state.new 18648 14867 125.43%
.flow.end.tcp_state.syn_sent 1207 183 659.56%
.flow.end.tcp_state.syn_recv 73 52 140.38%
.flow.end.tcp_liberal 100800 90436 111.46%
.tcp.segment_memcap_drop 2882 11729 24.57%
.tcp.reassembly_gap 159626 114099 139.9%
.tcp.insert_data_normal_fail 2736 11358 24.09%
.app_layer.error.http.parser 106 55 192.73%
.app_layer.error.smtp.gap 109 61 178.69%
.app_layer.error.tls.gap 70385 60833 115.7%

Pipeline 8725
WARNING: THERE IS A KNOWN BAD BASELINE WITH PACKET DROPS. bE MINDFUL OF ANY RESULTS.

Copy link
Member

@jasonish jasonish left a comment

Choose a reason for hiding this comment

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

Looks good!

@victorjulien victorjulien mentioned this pull request Sep 1, 2022
This was referenced Sep 8, 2022
@victorjulien
Copy link
Member

Merged in #7839, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants