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

Add stream following capability #9

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add stream following capability #9

wants to merge 1 commit into from

Conversation

allenluce
Copy link

I have a cluster of machines handling nearly a million packets a second. I wanted to get the HTTP responses to HTTP requests that matched a regexp. Check the commit message for details.

Super-cool utility!

The -J option will print packets following a match but only those that
are in the same TCP stream as the match.

If another packet matches during the following, the number counter is
reset. This helps mitigate problems with a match not having enough
packets after it in the same stream (which could mean ngrep prints
nothing forever after the match). This functionality can be defeated
with the -Q option.
@jpr5
Copy link
Owner

jpr5 commented Sep 8, 2017

Hot damn, nice! Thanks for the patch!

Here's a thought -- -A has always been a useful grep parity option, but back in my netops days I'd regularly pass a higher number to it when looking at busy flows, to increase the chances of actually catching the subsequent packets from the stream I was interested in. Not sure why it didn't occur to me to do the logical equivalent of your patch, I guess maybe I compensated for it with more restrictive BPF filters as I zeroed in on it.. dunno.

Anyway, what do you think about modifying -A's behavior instead of creating a new one with -J? Can you think of a use case where -A's behavior is ever better/desired over -J's?

@jpr5 jpr5 self-assigned this Sep 8, 2017
@allenluce
Copy link
Author

While I haven't personally needed it, I could see -A being useful to grab DNS transactions or other non-TCP stuff. Maybe if -A worked as today when it matches in a non-TCP packet but follows the stream when it is TCP?

@jpr5 jpr5 added this to the 1.47.1 milestone Sep 11, 2017
@jpr5
Copy link
Owner

jpr5 commented Sep 11, 2017

I think changing -A only some of the time has the potential to be surprising (ergo not good). So I think I'll stick with a new flag. Appreciate the help thinking it through.

Will merge sometime this week, when I hunt down the SIP trailing chars bug in #10.

@jpr5
Copy link
Owner

jpr5 commented Sep 16, 2017

So -A should be the anchoring behavior - show packets following a match. Your -J and -Q are modifiers of that behavior. So -J shouldn't take an option - otherwise you could specify -A and -J at the same time and that doesn't make sense.

So now I'm thinking, what might be the most memorable/relevant/defensible option names for your -J and -Q, as a complement to -A. Ideas?


if (keep_matching && follow_stream) {
if (follow_ip_src == NULL) {
follow_ip_src = ip_src;
Copy link
Owner

Choose a reason for hiding this comment

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

How much have you tested this functionality?

You introduce the global pointer follow_ip_src and set it to ip_src, which itself is a stack-based variable in the caller process() - thus ip_src deallocates with every call. This would make for a dangling global pointer on the next call, possibly creating a crash or a secvuln. I can see it maybe working anyway, either due to things just lining up with the same stack address for ip_src, or subsequent matches still happening due to a port match.

Do I misunderstand?

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

Successfully merging this pull request may close these issues.

2 participants