Harden EventFilter validation and event delivery isolation - #1952
Merged
Conversation
…FilterOperand EventContentFilter.decodeOperands cast each decoded operand to FilterOperand unchecked. An operand that decodes to some other structure raised ClassCastException, and one with an unknown encoding id raised UaSerializationException. Neither is a UaException, so both escaped the per-event filter evaluation in MonitoredEventItem. Decode each operand explicitly and report either failure as a UaException with Bad_FilterOperandInvalid, which the event delivery path already handles.
EventContentFilter.validate recorded Good for every ElementOperand. Part 4 §7.7.4.2 requires the index to be greater than the index of the element it is part of and to reference an existing element. Filters that violated this were accepted at CreateMonitoredItems and only failed later, per event, when the operand was resolved. Report such operands as Bad_FilterOperandInvalid in the ContentFilterElementResult so the filter is rejected up front.
The ElementOperand guard in DefaultOperatorContext tracks the elements on the current evaluation path, which detects cycles but does not prevent an element referenced from several places in an acyclic filter from being evaluated once per path. A chain of And elements that each reference the next element twice costs 2^n evaluations, since And only short-circuits on FALSE. Element evaluation has no side effects within a single event, so cache each element's result, or the UaException it raised, for the lifetime of the operator context and reuse it on later references.
MonitoredEventItem.installFilter decided whether the filter could be evaluated from the select clause and element status codes alone. An element whose operator validated but whose operand was reported as Bad_FilterOperandInvalid still armed the filter, and the operand failure then surfaced on every event instead of in the EventFilterResult. Treat a bad operand status code the same as a bad element status code: the item is created with the EventFilterResult describing the error and the filter is not evaluated.
MonitoredEventItem.onEvent caught UaException only. Any other exception raised while evaluating the where clause or selecting event fields propagated to the caller delivering the event. Catch Exception so a failure evaluating one item's filter is logged against that item and the delivery loop continues.
ServerEventNotifier.fire notified listeners in a plain loop. An exception thrown by one listener stopped delivery to every listener registered after it and propagated to the code firing the event. Catch and log exceptions per listener so each registered listener is notified independently of the others. Notifier-scope resolution and per-listener scope filtering are unchanged.
DefaultConditionManager.deliverRefresh already isolated snapshot replay and RefreshEnd delivery per item, but delivered the RefreshStart marker in an unguarded loop. A failure delivering RefreshStart to one item aborted the refresh before the remaining items received their marker. Guard RefreshStart delivery per item the same way. An item whose RefreshStart delivery fails is not treated as started and receives no further refresh traffic; the other items receive a full bracket. Add tests pinning the per-item isolation of replay and RefreshEnd delivery and the release of the per-Subscription refresh guard.
This file contains hidden or 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
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.
Several independent defects in the server-side EventFilter delivery path let a single monitored item's filter interfere with event delivery to other items, or let filters that Part 4 says are invalid through validation only to fail on every event afterwards.
EventContentFilter.decodeOperandscast each decoded operand toFilterOperandunchecked, so an operand that decodes to some other structure raisedClassCastExceptionand one with an unknown encoding id raisedUaSerializationException. Neither is aUaException, so both escapedMonitoredEventItem.onEvent. Related to that,installFilteronly looked at select clause and element status codes when deciding whether to arm the filter, so an element whose operand had been reported asBad_FilterOperandInvalidstill armed. AndServerEventNotifier.firenotified listeners in a plain loop, so an exception from one listener stopped delivery to every listener registered after it and propagated to the caller.Operands are now decoded explicitly and reported as
Bad_FilterOperandInvalid, operand status codes participate in the arming decision,onEventcontains any exception from evaluation, andfireisolates each listener while leaving notifier-scope resolution and per-listener scope filtering unchanged. The item result for a filter that validates with errors is unchanged: the item is created with Good and theEventFilterResultcarries the per-clause codes, as before.Two further issues in
ElementOperandhandling are addressed.validaterecorded Good for everyElementOperand; it now enforces Part 4 §7.7.4.2 (the index must be greater than the containing element's index and reference an existing element) so these filters are rejected at CreateMonitoredItems. And the cycle guard added in #1780 tracks only the current evaluation path, so an element referenced from several places in an acyclic filter was evaluated once per path, which is exponential for a chain ofAndelements that each reference the next element twice.DefaultOperatorContextnow caches each element's result, or theUaExceptionit raised, for the lifetime of one event evaluation. This is sound because element evaluation has no side effects within a single event.On this branch
DefaultConditionManager.deliverRefreshalready isolated snapshot replay and RefreshEnd delivery per item but delivered RefreshStart in an unguarded loop; it now guards RefreshStart the same way, and tests pin the per-item isolation of the whole bracket and the release of the per-Subscription refresh guard. Tests also cover event items whose filter validates with errors throughSubscriptionManagercreation and the SetMonitoringMode registration toggle, and a throwing in-scope listener alongside scope filtering inServerEventNotifier.fire. TheonRefreshMarker, overflow, andonConditionEventOverflowpaths were reviewed: they only reachselect, which convertsUaExceptionto a null field, and their callers already guard withcatch (Throwable)orcatch (Exception), so no change was needed there.Verification:
spotless:apply,clean compile, and the fullsdk-servertest suite pass, along with the integration tests for event filter operators, event routing, ConditionRefresh, and the new event delivery isolation test. Each new unit test was also run against the unmodified production code to confirm it fails there.The same fixes are opened separately against
main, where the surrounding delivery code has diverged.