New DNP3_Callbacks do not include handlers for rejected link or transport data

[jadamcrain]

Notifications for these types of "bad data" will be required to close the TCP session.

If the parser "feed" function returns an error for these that's a suitable alternative.

[pesco]

Adam Crain [email protected] writes:

Notifications for these types of "bad data" will be required to close
the TCP session.

You're right; I overlooked the link layer because I still have to
implement those validations.

For the transport layer, I take it you mean invalid segment series
(unexpected sequence numbers and thelike). I can tie these in as
semantic actions on the "tfun" parser.

If the parser "feed" function returns an error for these that's a
suitable alternative.

I think it's better to do it in response to callbacks; it seems to give
nicer separation of concerns. The protocol itself as represented by the
stream processor is quite happy to consume anything, so feed never
fails.

[jadamcrain]

WRT to the transport function, I was thinking of reassembly buffer overflows in particular, but it would be good to be notified of any kind of invalid segment series like you say.

If there are callbacks for any kind of protocol violation, I agree that we can remove the requirement that feed return an error code.

[pesco]

Now that you mention it, buffer overflows might actually be one thing where returning an error from feed is more appropriate, since the parser could be stuck in the sense that it would be incorrect to ignore the input (which was not necessarily invalid) but it can't process it.

[pesco]

5e2cf2b adds.

 void (*link_invalid)(void *env, const DNP3_Frame *frame);

[pesco]

Unfortunately, the transport layer callbacks are not possible without either throwing out the way the transport function is implemented or making an extension to hammer that I had considered before to solve the thread-safety issue:

h_parse__p(parser, input,len, (void *)userdata)

where the userdata pointer is passed to semantic actions that don't have other data associated with them, i.e. that are created with

h_action(parser, action, NULL)

Without a path to inject semantic data into the parse, the statically-defined semantic actions cannot find the callbacks / Dissector instance associated with the current parse.

[jadamcrain]

OK. The StreamProcessor is in full control of what gets written when the proxy is in it's normal operating mode.

Let's keep this open just to track it, but there are probably a lot more valuable things to focus on in the remaining hours. I suspect the elfbac integration will be taking your time.

[pesco]

It's another one of those issues that will benefit when we can synthesize our own output. (We will only produce sane segment series.)

[pesco]

Looks like I partly spoke too soon! (Misread my own code.) I can add a callback to report whenever the transport layer discards frames, though it won't tell you why it did so. I'd also not guarantee that it won't call it for every frame individually.

    void (*transport_discard)(void *env, size_t n);     // n = number of bytes

Note that n refers to bytes at the link layer, i.e. that count includes segment and frame headers.

Cf. 02eb157.

Is that enough to close the issue?

[jadamcrain]

"I'd also not guarantee that it won't call it for every frame individually."

Can you clarify this? It is only called for invalid frames, correct?

I don't care if it is invoked multiple times, so long as it only happens when invalid series is present.

[pesco]

Yes, but it might be called for each invalid segment in a series, or any combination of them. Though I'm trying to have it called only once, I'd like to not make that a guarantee.

[pesco]

That should have read "each segment in an invalid series", to be precise.

[jadamcrain]

I'm ok w/ that.