detect: Improve support for larger address values. #5808
Conversation
This commit improves support for large address variables. Without this commit, address size was fixed at 8196 or less. This commit permits larger sized address variables.
Codecov Report
@@ Coverage Diff @@
## master #5808 +/- ##
=======================================
Coverage 72.38% 72.38%
=======================================
Files 604 604
Lines 179369 179390 +21
=======================================
+ Hits 129837 129854 +17
- Misses 49532 49536 +4
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
|
||
| size_t address_length = strlen(s); | ||
| if (address_length > (MAX_ADDRESS_LENGTH - 1)) { | ||
| char *address = SCCalloc(1, address_length); |
There was a problem hiding this comment.
Is there a way to get rid of address and use only s ?
There was a problem hiding this comment.
address is a return parameter ... built as s is parsed.
There was a problem hiding this comment.
Ok, what is this return parameter for ?
There was a problem hiding this comment.
Recursion is possible when parsing addresses; when this happens address contains the subset of the original address to be parsed in the next step.
There was a problem hiding this comment.
What recursion ? Between variables like EXTERNAL_NET and their values ?
There was a problem hiding this comment.
See the calls in DetectAddressParseInternal that invoke DetectAddressParse2 for example.
There was a problem hiding this comment.
I think I do not get it :-/
So I would upvote code simplicity over saving an allocation...
There was a problem hiding this comment.
From what I can gather (and remember), address is really only used to tokenize the input string on the fly. Other than making a copy the only modifications I can spot are inserting \0 chars to split the string before passing the parts on to further parsing logic. If this is indeed the case another approach could be to pass arrays of chars with an explicit length around. It does look like this is quite intrusive as we call out into various functions with the address part strings as the argument.
| SCLogError(SC_ERR_ADDRESS_ENGINE_GENERIC, | ||
| "Hit the address buffer" | ||
| " limit for the supplied address. Invalidating sig. " | ||
| "Please file a bug report on this."); | ||
| goto error; | ||
| } | ||
| address[x] = s[u]; |
There was a problem hiding this comment.
I'm confused about the logic here, and why we'd need a 'full size' address here. AFAICS we use it as a temporary place to put the address from s before passing it on. But does it need to be so big for that? Or could we have a smaller (local) temp var that is just reset after loop interations?
There was a problem hiding this comment.
I suppose its because this parses anything between [...] which can have any length.
|
Merged in #6071, thanks! |
Continuation of pr #5619
This PR modifies the address rule value parsing to permit larger address values in the rule variables section.
Link to redmine ticket: 3887
Describe changes:
PRScript output (if applicable):
#suricata-verify-pr:
#suricata-verify-repo:
#suricata-verify-branch:
#suricata-update-pr:
#suricata-update-repo:
#suricata-update-branch:
#libhtp-pr:
#libhtp-repo:
#libhtp-branch: