-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
applayer/htp: convert to new FAIL/PASS API /v5 #11973
base: master
Are you sure you want to change the base?
applayer/htp: convert to new FAIL/PASS API /v5 #11973
Conversation
NOTE: This PR may contain new authors. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #11973 +/- ##
==========================================
+ Coverage 82.77% 82.84% +0.06%
==========================================
Files 910 910
Lines 249016 248530 -486
==========================================
- Hits 206134 205890 -244
+ Misses 42882 42640 -242
Flags with carried forward coverage won't be shown. Click here to find out more. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is a very big file, with lots of tests. You're brave for picking up this one!
Test HTPBodyReassemblyTest01
still has a mixed style: it hasn't been fully converted to the FAIL/PASS
style.
I think I've covered all cases where we need some improvement, still, so you can get a good understanding of what the next steps are.
There is one thing that I asked someone else to do a sanity check. If you are idly waiting, feel free to claim another task, in this case, ok?
FAIL_IF(bstr_ptr(tx_ud->request_uri_normalized)[0] != '/' || | ||
bstr_ptr(tx_ud->request_uri_normalized)[1] != '%' || | ||
bstr_ptr(tx_ud->request_uri_normalized)[2] != '0' || | ||
bstr_ptr(tx_ud->request_uri_normalized)[3] != '0') | ||
{ | ||
printf("normalized uri \""); | ||
PrintRawUriFp(stdout, bstr_ptr(tx_ud->request_uri_normalized), bstr_len(tx_ud->request_uri_normalized)); | ||
printf("\": "); | ||
goto end; | ||
} | ||
} | ||
bstr_ptr(tx_ud->request_uri_normalized)[3] != '0'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's break each of these into their own FAIL
... macros
printf("Could not get config for: %s\n", addr); | ||
goto end; | ||
} | ||
FAIL_IF_NULL(user_data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the tests passed, I suppose there's no harm here. But wanted to point out that this changes the original format a bit, since we don't jump to end
in this case, even if user_data
was NULL
if (inet_pton(AF_INET, addr, buf) == 1) { | ||
(void)SCRadixFindKeyIPV4BestMatch(buf, cfgtree, &user_data); | ||
if (user_data != NULL) { | ||
HTPCfgRec *htp_cfg_rec = user_data; | ||
htp = htp_cfg_rec->cfg; | ||
SCLogDebug("LIBHTP using config: %p", htp); | ||
} | ||
if (htp == NULL) { | ||
printf("Could not get config for: %s\n", addr); | ||
goto end; | ||
} | ||
FAIL_IF_NULL(user_data); | ||
HTPCfgRec *htp_cfg_rec = user_data; | ||
htp = htp_cfg_rec->cfg; | ||
SCLogDebug("LIBHTP using config: %p", htp); | ||
|
||
FAIL_IF_NULL(htp); | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we can simplify this to just check:
FAIL_IF_NOT(inet_pton(AF_INET, addr, buf) == 1)
since the else case will simply be a FAIL
. What do you think, @inashivb ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to check user data and htp config too. So, yes to your suggestion, followed by the rest of the matches too. The block could look like:
FAIL_IF(inet_pton(AF_INET, addr, buf) != 1)
(void)SCRadixFindKeyIPV4BestMatch(buf, cfgtree, &user_data);
if (user_data != NULL) {
HTPCfgRec *htp_cfg_rec = user_data;
htp = htp_cfg_rec->cfg;
SCLogDebug("LIBHTP using config: %p", htp);
FAIL_IF_NULL(htp);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then maybe... Would this until make sense?
FAIL_IF(inet_pton(AF_INET, addr, buf) != 1);
(void)SCRadixFindKeyIPV4BestMatch(buf, cfgtree, &user_data);
FAIL_IF_NULL(user_data);
HTPCfgRec *htp_cfg_rec = user_data;
FAIL_IF_NULL(htp);
htp = htp_cfg_rec->cfg;
SCLogDebug("LIBHTP using config: %p", htp);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah this was my first choice but then I was trying to make it replicate the current code.. I guess this works though. Let's do this! :)
FAIL_IF(bstr_ptr(tx_ud->request_uri_normalized)[0] != '/' || | ||
bstr_ptr(tx_ud->request_uri_normalized)[1] != '?' || | ||
bstr_ptr(tx_ud->request_uri_normalized)[2] != 'a' || | ||
bstr_ptr(tx_ud->request_uri_normalized)[3] != '=' || | ||
bstr_ptr(tx_ud->request_uri_normalized)[4] != '%' || | ||
bstr_ptr(tx_ud->request_uri_normalized)[5] != '0' || | ||
bstr_ptr(tx_ud->request_uri_normalized)[6] != '0') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as with other case :)
goto end; | ||
} | ||
FAIL_IF_NULL(tx); | ||
FAIL_IF_NULL(tx_ud); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check doesn't seem necessary here (also comparing with other similar tests in this file)
Thankyou @jufajardini ..It was first time naivety! I did not realize how much work was required, almost gave up but am glad i stuck through. |
Hey @jufajardini , did the sanity test pass? |
Yep! See Shivani's comment on the thread where we were discussing the check formats :) You can submit a new version of this work whenever you have the time ^^ |
Ticket: #6935
Make sure these boxes are checked accordingly before submitting your Pull Request -- thank you.
Contribution style:
https://docs.suricata.io/en/latest/devguide/contributing/contribution-process.html
Our Contribution agreements:
https://suricata.io/about/contribution-agreement/ (note: this is only required once)
Changes (if applicable):
(including schema descriptions)
https://redmine.openinfosecfoundation.org/projects/suricata/issues
Link to ticket: https://redmine.openinfosecfoundation.org/issues/
Describe changes:
-Previous pr applayer/htp: convert to new FAIL/PASS API v-4 #11960
Provide values to any of the below to override the defaults.
link to the pull request in the respective
_BRANCH
variable.SV_REPO=
SV_BRANCH=
SU_REPO=
SU_BRANCH=
LIBHTP_REPO=
LIBHTP_BRANCH=