Allow users to access IP/TCP/UDP headers #1031
Conversation
|
@brianrob could you please review? This is the last bit I need for my own ETL parsing tool which interprets DNS data. Thanks! :) |
|
@lupino3 thanks for submitting. I will need a bit of time to look through this, but consider this an ack. |
|
Sure, thanks @brianrob. Note that the diff algorithm is fooled by my addition of two extra properties. I didn't change a lot in the body of the existing The only changes there are storing the parsed packet, IP/TCP/UDP headers in a private class attribute, which is then returned by the corresponding property. I also added a Hope the change makes sense! Thanks. |
|
We're working to clean-up old open PRs in this repo. This PR is greater than 1 year old. If you would like to continue working on this PR, please add a comment within the next 7 days so that we can start discussion on next steps. Otherwise, we will close this PR. Please feel free to open a new PR or issue if you'd like to re-open this discussion at a later date. |
|
@brianrob this is a very old PR, I haven't worked on this project for 5 years :) That being said, I think this feature is useful and I think it should be merged. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@lupino3 thanks for your response. I've triggered the CI for validation. Also, one additional question in the code review. |
| @@ -695,6 +635,119 @@ public override object PayloadValue(int index) | |||
|
|
|||
| protected override internal void SetState(object newState) { m_state = (MicrosoftWindowsNDISPacketCaptureTraceEventParserState)newState; } | |||
| private MicrosoftWindowsNDISPacketCaptureTraceEventParserState m_state; | |||
|
|
|||
| private bool parsed = false; | |||
There was a problem hiding this comment.
Do you expect parsed to be set to false for each new event? In most cases, you'll just have a single instance of this object and it will be attached to a raw payload and used as a parser. Thus, if you set parsed = true, it will remain true for the next event as well.
There was a problem hiding this comment.
I see, I didn't take that into account at all! I see now the notice in the TraceEvent documentation. Do you think this PR looks good if I simply remove parsed, or would you recommend further changes?
There was a problem hiding this comment.
I think the idea of exposing the data is reasonable. The biggest concern I have is the lifetime. I think the way to address this would be to clone the TraceEvent object and then reference it from the networking objects rather than passing a byte*. This would ensure that the lifetime of the referenced data is correct. When you call Clone on the TraceEvent object, it copies the buffer into separate memory that will live longer than the callback. Then it's safe to do the kind of stuff that you're doing here. So, I think the right path forward would be to have an API on the event itself that gets you an intermediate object that owns a clone of the TraceEvent and then each of the structured objects that you create that take a byte* can just keep a reference to the clone. Once they all go out of scope, the clone will be finalized. Alternatively, you could keep track and dispose of the clone when done.
There was a problem hiding this comment.
Would copying the relevant portion of buffer into the *Header object itself make sense?
This way I don't have to manage the lifetime of TraceEvent, and the copy can be done when necessary (i.e., when the user accesses the corresponding property).
There was a problem hiding this comment.
Yes, this would be fine as well.
This will allow users to access packet metadata without needing to parse the "parsedPacket" string, and will also allow them to access the packet data present after the headers for further computation, because the header addresses are now exposed. The IPHeader, TCPHeader, UDPHeader structs have been moved outside PacketFragmentArgs to allow definition of properties with the same name. They have been converted from structs to class to make them into reference types and therefore have the homonymic properties return null if the corresponding header was not successfully parsed by ParsePacket(). I am proposing this change because I need to parse DNS request/response in packets, and it is almost impossible to do it with the current class structure without duplicating the code of FIndIpHeader(), and I think that it's much cleaner to expose the headers themselves rather than just exposing FindIpHeader() and having the users duplicate the parsing logic.
This will allow users to access packet metadata without needing to parse
the "parsedPacket" string, and will also allow them to access the packet
data present after the headers for further computation, because the
header addresses are now exposed.
The IPHeader, TCPHeader, UDPHeader structs have been moved outside
PacketFragmentArgs to allow definition of properties with the same name.
They have been converted from structs to class to make them into
reference types and therefore have the homonymic properties return null
if the corresponding header was not successfully parsed by
ParsePacket().
I am proposing this change because I need to parse DNS request/response
in packets, and it is almost impossible to do it with the current class
structure without duplicating the code of FIndIpHeader(), and I think
that it's much cleaner to expose the headers themselves rather than just
exposing FindIpHeader() and having the users duplicate the parsing
logic.