-
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
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. |
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.
Looks good to me...
Wonder if a deeper rewriting would be beneficial...
|
||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to get rid of address
and use only s
?
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.
address
is a return parameter ... built as s
is parsed.
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.
Ok, what is this return parameter for ?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What recursion ? Between variables like EXTERNAL_NET
and their values ?
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.
See the calls in DetectAddressParseInternal
that invoke DetectAddressParse2
for example.
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 I do not get it :-/
So I would upvote code simplicity over saving an allocation...
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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: