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

disable_analyzer doesn't work on TCP_ApplicationAnalyzer's #532

Closed
vpax opened this issue Aug 9, 2019 · 3 comments

Comments

@vpax
Copy link
Contributor

commented Aug 9, 2019

disable_analyzer walks a connection's Analyzer tree looking for the analyzer to remove, but this doesn't include TCP_ApplicationAnalyzer's because those are not really in the tree, but rather are separate children of the TCP_Analyzer.

More generally, I keep running into problems because Analyzer methods don't work consistently on all subclasses of Analyzer - really confusing. Either this should be regularized, or terminology adjusted so that the coder isn't misled.

@jsiwek jsiwek added this to Unassigned / Todo in Release 3.1.0 via automation Aug 9, 2019

@jsiwek jsiwek added this to the 3.1.0 milestone Aug 9, 2019

@jsiwek

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

Do you have a specific example to reproduce what you are seeing?

Asking because HTTP_Analyzer is a type of TCP_ApplicationAnalyzer, but this appears to work:

event http_reply(c: connection, version: string, code: count, reason: string)
	{ disable_analyzer(c$id, current_analyzer(), T); }

Though this does not work:

event protocol_confirmation(c: connection, atype: Analyzer::Tag, aid: count) &priority=10
	{ disable_analyzer(c$id, aid, T); }

But that's because Analyzer::RemoveChildAnalyzer is only looking for the analyzer within the children list, while the protocol confirmation is happening at a time when the analyzer is only in the new_children list and waiting to be moved into children. Simple to patch that method to check both lists, but not sure if that's the same bug being reported.

@vpax

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2019

I don't have an easy example, as this was from debugging my own plugin. However, maybe you can clarify for me about new_children. What I saw was that my analyzer, which I was adding via TCP_Analyzer::AddChildPacketAnalyzer, gets put into the TCP_Analyzer's packet_children list. I don't see how the generic Analyzer::RemoveChildAnalyzer code is going to know about that list and be able to loop over it (though that's certainly the semantics I'd like, ideally).

@jsiwek jsiwek self-assigned this Aug 10, 2019

jsiwek added a commit that referenced this issue Aug 10, 2019

GH-532: improve disable_analyzer BIF
- Add an extra "prevent" parameter (default value of false), which
  helps prevent the same analyzer type from being attached in the
  future.  It's useful in situations where you want to disable early
  on, but a DPD signature may still trigger later and re-attach
  the same analyzer.  E.g. when not using this flag, but calling
  disable_analyzer() inside an http_request event, will remove the
  HTTP analyzer that was attached due to well-known-port, but a later
  DPD signature match from upon seeing the HTTP reply will end up
  attaching another HTTP analyzer.  More surprising is that upon
  re-attaching that analyzer, you'll get the same http_request as
  before since the DPD buffer will get replayed into the new analyzer.

- Fixes disable_analyzer() to work when called even earlier, like
  within the protocol_confirmation event.  At that time, the
  Analyzer tree may have not properly added the new analyzer into
  Analyzer::children yet, but rather the temporary waiting list,
  Analyzer::new_children.  Analyzer::RemoveChildAnalyzer previously
  did not inspect the later list.

- Fixes disable_analyzer() when called on an analyzer added to the
  tree via TCP_Analyzer::AddChildPacketAnalyzer.  TCP_Analyzer
  keeps track of such children in its own list,
  TCP_Analyzer::packet_children, which the previous
  Analyzer::RemoveChildAnalyzer implementation didn't inspect.
@jsiwek

This comment has been minimized.

Copy link
Member

commented Aug 10, 2019

However, maybe you can clarify for me about new_children.

I was looking at the way analyzers generally get added to the tree from either a DPD signature or well-known port. Those call Analyzer::AddChildAnalyzer which doesn't directly put the new analyzer in the children list, but rather new_children (seems that's to prevent iterator invalidation if called while looping over children). In the DPD case, I was seeing the disable_analyzer call fail to find the analyzer within the protocol_confirmation event since it was still in the new_children list.

So sounds like I found a different bug than you.

What I saw was that my analyzer, which I was adding via TCP_Analyzer::AddChildPacketAnalyzer, gets put into the TCP_Analyzer's packet_children list. I don't see how the generic Analyzer::RemoveChildAnalyzer code is going to know about that list

Yeah, the packet_children were not considered in RemoveChildAnalyzer. Would be great if you can confirm the patch in #534 fixes it for you. I tested it using the conn-size analyzer which also uses TCP_Analyzer::AddChildPacketAnalyzer like you do.

@jsiwek jsiwek moved this from Unassigned / Todo to Pending Review / Merge in Release 3.1.0 Aug 12, 2019

rsmmr added a commit that referenced this issue Aug 16, 2019

Merge remote-tracking branch 'origin/topic/jsiwek/gh-532-improve-disa…
…ble-analyzer'

Includes fix for potential iterator invalidation during iteration.

* origin/topic/jsiwek/gh-532-improve-disable-analyzer:
  GH-532: improve disable_analyzer BIF

@rsmmr rsmmr closed this in #534 Aug 16, 2019

Release 3.1.0 automation moved this from Pending Review / Merge to Done Aug 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants
You can’t perform that action at this time.