Page MenuHomePhabricator

Short circuit fails with assignments
Open, Needs TriagePublic

Description

Similarly to T214642, there are several filters failing with message "Undefined variable xxx". Locally, I could get the following as the minimum failing syntax:

false & (var := 'a'); var != 'b'

Again, syntax check says everything is fine, but actual parsing fails due to short circuit. I played a bit with the parser and finally found the cause:
The first condition is of course false, and thus we enter short circuit. Then we get to "(". This would normally be handled by doLevelBraces, but when we get there mShortCircuit is true, and thus the parser calls skipOverBraces to ignore everything inside parentheses. Then the statement ends, and we get to evaluate var != 'b'. However, the variable declaration is inside brackets and was skipped, and thus the variable isn't defined.
For this case I don't really have a solution. Mostly, because the syntax shouldn't be considered valid by the syntax check: the correct way to write it would be

false & ( (var := 'a'); var != 'b' )

However, making that syntax invalid could possibly break many existing filters. Another solution could be to accept the syntax as valid and store variables inside skipOverBraces (maybe without actually computing them, and just storing the names), but that would add yet another bad syntax to AbuseFilter (and we already have T156096 and T212730) and could make skipOverBraces not so good in terms of performance save.

Event Timeline

Daimona created this task.Jan 25 2019, 10:15 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 25 2019, 10:15 AM
Daimona moved this task from Backlog to Future on the User-Daimona board.Jan 25 2019, 10:15 AM

OK, so I think an acceptable solution could be to add a check to skipOverBrace, and if we find a variable declaration, we add that variable name to the vars map, marking it as not initialized. The same as T156096#5207401 applies, i.e. we first need the new data type for non-initialized stuff.

Change 522763 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Initialize user-defined variables during shortcircuit

https://gerrit.wikimedia.org/r/522763

Daimona claimed this task.Sun, Jul 14, 9:50 AM