Conversation
by making tx parsing and creation more easily available, without needing a dns state. Dns event NotResponse is now set on the right tx, and not the one before. Also debug log for Z-flag on request says "request" instead of "response" Also rustfmt dns.rs
Ticket: 5773
Ticket: 5773
Ticket: 5773
Now a flow alproto can be changed by a call to AppLayerParserParse when HTTP2 forces the flow to turn into DOH2.
Ticket: 5773 Handles both directions the same way for data if content type is application/dns-message
So as to consume less memory for HTTP2Transaction
|
ERROR: ERROR: QA failed on build_asan. Pipeline 21273 |
|
Passed my QA. Ran this PR with SV PR OISF/suricata-verify#1734. Local pipeline 4966, run 511. |
| * in the list that are not for us. */ | ||
| engine = engine->next; | ||
| continue; | ||
| if (engine->alproto != 0) { |
There was a problem hiding this comment.
ok (is written 0 in master)
| } | ||
|
|
||
| #[derive(Debug, Default)] | ||
| pub struct DohHttp2Tx { |
There was a problem hiding this comment.
can you add field description comments, esp on the index to these 2 fields
| js.open_array("query")?; | ||
| for i in 0..0xFFFF { | ||
| let mut jsa = JsonBuilder::try_new_object()?; | ||
| if !SCDnsLogJsonQuery(dtx, i, 0xFFFFFFFFFFFFFFFF, &mut jsa) { |
There was a problem hiding this comment.
can we use a named constant for this?
There was a problem hiding this comment.
I wait for stabilization of #11305 to use what will come of it...
| return AppLayerRequestProtocolChange(f, 443, ALPROTO_TLS); | ||
| } | ||
|
|
||
| void AppLayerForceProtocolChange(Flow *f, AppProto new_proto) |
There was a problem hiding this comment.
can this get a comment explaining the logic of only setting f->alproto to new_proto?
There was a problem hiding this comment.
alproto_ts and alproto_tc are also set to new_proto
There was a problem hiding this comment.
adding a comment to the function anyways
| return false; | ||
| } | ||
|
|
||
| // whether a signature AppProto matches a flow (or signature) AppProto |
There was a problem hiding this comment.
can you add a comment here on the interplay between doh2 and http2 and sigs using either?
| // Get inner transaction for engine | ||
| void *DetectGetInnerTx(void *tx_ptr, AppProto alproto, AppProto engine_alproto, uint8_t flow_flags) | ||
| { | ||
| if (alproto == ALPROTO_DOH2) { |
There was a problem hiding this comment.
is this expected to be the common case for calls to this function? Or should we put the non-doh2 first to improve branch prediction?
There was a problem hiding this comment.
The logic is not the same as there is an else if
Adding unlikely
victorjulien
left a comment
victorjulien
left a comment
There was a problem hiding this comment.
Nice work so far. Some comments inline.
|
Information: QA ran without warnings. Pipeline 21346 |
|
Continued in #11428 |
3 participants
Link to redmine ticket:
https://redmine.openinfosecfoundation.org/issues/5773
Describe changes:
SV_BRANCH=OISF/suricata-verify#1734
#11369 with greener CI.
@victorjulien should I squash the 2 last commits in ?
They are are help to help you review your last comments...
Draft because of TODO :