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

stream gap handling - v5 #2660

Closed
wants to merge 3 commits into from
Closed

stream gap handling - v5 #2660

wants to merge 3 commits into from

Conversation

jasonish
Copy link
Member

Previous PR: #2619

Changes from last PR:

  • DNS TCP gap handling.

Requesting comments on this way of handling gaps in the app-layer. There are ifdef's of older code as I'm still using them for reference.. Hence this is just an RFC.

Protocols support gap handling:

  • DNP3 - this is a simple record based protocol with a fixed start sequence.
  • DNS (TCP) - Its very likely, though not always true that a TCP segment will be the start of a request/response. So probe it like it was new.

There is one stream test case that is broken from these changes tho.

Prscript:

Allow app-layers to register that they can handle gaps.

Instead of sending a "signal" on a gap, send the gap flag on the next
segment, the app-layer "dispatcher" will check if the app-layer can
handle gaps or not, and abort if they can't, otherwise the data along
with a gap flag will be sent to the app-layer.

The idea is that the parser can then make the decision on what to do
with the gap.
On data check if the gap flag is set. If so, check if the data is at
the start of frame. This is pretty easy with DNP3 as there is a start
frame byte sequence.
On notification of a gap, reprobe the stream. If successful,
clear any existing buffer and continue on.
@inliniac
Copy link
Contributor

inliniac commented May 1, 2017

This will need a rebase after the stream updates.

On a gap, I would like to invoke the app-layer with a value of the number of skipped bytes. So protocols might be able to use to catch up. E.g. HTTP with a content-len field might see that the gap is in a file, but otherwise the HTTP tracking is unaffected.

@inliniac inliniac added needs rebase Needs rebase to master preview labels May 1, 2017
@inliniac
Copy link
Contributor

inliniac commented May 2, 2017

The gap invocation could perhaps just be a extra call to AppLayerHandleTCPData with data NULL, data_len set to the gap size and flags set to STREAM_GAP.

@jasonish
Copy link
Member Author

jasonish commented May 2, 2017

The gap invocation could perhaps just be a extra call to AppLayerHandleTCPData with data NULL, data_len set to the gap size and flags set to STREAM_GAP.

Yeah, I was wanting to get all that info into the app-layer, with the new data to skip the extra call, and state tracking required, but that would change the API across all app layer. So perhaps the extra call, and then the app-layer having to set a gap flag for the next call is better.

@jasonish
Copy link
Member Author

jasonish commented May 8, 2017

Closing. See #2696

@jasonish jasonish closed this May 8, 2017
@jasonish jasonish deleted the dev-gap-5 branch June 6, 2017 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs rebase Needs rebase to master preview
Development

Successfully merging this pull request may close these issues.

2 participants