Improve &expire_func compatibility type-checking #1505
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously, incompatible &expire_funcs could mistakenly be used, such as when using that attribute on the unspecified table()/set() initializations/assignments, resulting in invalid function calls that eventually crash Zeek.
timwoj
approved these changes
Apr 14, 2021
Now covers more forms of both valid and invalid &expire_funcs
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes GH-1503
Previously, incompatible &expire_funcs could mistakenly be used, such as
when using that attribute on the unspecified table()/set()
initializations/assignments, resulting in invalid function calls that
eventually crash Zeek.
Ideally, type-checking could catch this at parse-time always, but found that may not generally be possible and a run-time type-check may have to happen in rare scenarios (but the common ones still always catch parse-time). Going from common to rare:
When using unspecified
table()expression in initializations, the full type/attribute info isn't available to be caught in the previousCheckAttr()location, have to wait formake_varlogic to propagate from unspecifiedtable()->ID->TableValctor ->TableVal::SetAttrs()If using unspecified
table()in run-time assignment, theTableCoerceExprcould likely be expanded to help catch more at parse-time, but eventually it will useTableValctor ->TableVal::SetAttrs()The
TableVal::SetAttrs()method is technically there for anyone to call directly (e.g. external plugins).So this patch addresses the common denominator:
TableVal::SetAttrs()must not be allowed to set an incompatible expire function.It's possible that some checks are now redundant and can give multiple error messages for the same issue, but I find that to be ok: don't want to get rid of the
Attributes::CheckAttr()check, because I'm not sure if it's still helping find some types incompatible expire functions at parse-time rather than run-time (e.g. when full type info actually is available for assignments).