Page MenuHomePhabricator

use of get_matches function returns "Requesting array item of non-array"
Closed, ResolvedPublic

Description

The get_matches function returns an array, but as of very recently I'm unable to use it like an array.

Here are long-standing filters that now throw syntax errors:

Basically we're doing match := get_matches("regex", added_lines)[1].

get_matches can return false if there are no matches, in which case it's of course invalid to treat it like an array. But up until now I think AbuseFilter gracefully failed, which is more than fine in my case (since the filter would evaluate to false anyway).

Details

Related Gerrit Patches:
mediawiki/extensions/AbuseFilter : wmf/1.34.0-wmf.16Revert "Better handling of DNONE"
mediawiki/extensions/AbuseFilter : wmf/1.34.0-wmf.16Better handling of DNONE
mediawiki/extensions/AbuseFilter : masterBetter handling of DNONE
mediawiki/extensions/AbuseFilter : wmf/1.34-wmf.16Revert "Better handling of DNONE"
mediawiki/extensions/AbuseFilter : wmf/1.34-wmf.16Better handling of DNONE

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 22 2019, 3:52 PM

Thanks for reporting, I do see some "Requesting array item of non-array" errors on logstash for get_matches... I believe a potential culprit is https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/AbuseFilter/+/478424/. I'll investigate a bit further, although it should be easy to test locally.

Yeah, that's probably the culprit. Some of the fixes done in https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/AbuseFilter/+/522763/ should have been included there, but I only discovered them later. And maybe, there are other places where we should add a check for DNONE. @MusikAnimal Could you please test locally if applying the patch above fixes the error? I'm unable to do so right now :-/

MusikAnimal triaged this task as High priority.Jul 22 2019, 8:35 PM

I can try testing your patch, sure. I'm going to bump this to High, hope that's okay. https://en.wikipedia.org/wiki/Special:AbuseFilter/944 is an important filter that we need to update every day. I see the last modification was today at 00:08, so I guess this bug was introduced this morning, or perhaps that user somehow bypassed the syntax checker? I've definitely made changes to this filter since July 16, when https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/AbuseFilter/+/478424/ was apparently deployed.

Yeah, fine. The syntax checker is bypassed because it's poorly implemented. What it does is disable shortcircuit and try to run the filter. This way, shortcircuit-only bugs will only be discovered at runtime. Other examples are T214674, T214642 and T218906, and T214643 is the wider task about syntax checking.

Also, given the nature of this bug, if the shortcircuit causing the bug is for consecutive &s, the filter wouldn't match anyway. I haven't checked the specific case, though.

Anyway, I believe that the patch above should fix the error. Otherwise I'll create another one, if there's another edge-case that was left out.

https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/AbuseFilter/+/522763/ indeed fixes the bug :) I'm not familiar with the parser code so I'm afraid I'll have to leave the +2'ing to someone else, though.

Daimona claimed this task.Jul 23 2019, 10:14 AM

https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/AbuseFilter/+/522763/ indeed fixes the bug :) I'm not familiar with the parser code so I'm afraid I'll have to leave the +2'ing to someone else, though.

Cool, thanks :-)

Meh, I just realized that the "errors" this task is about happen when trying to update the filter, or check the syntax, and not at runtime (i.e. reported on logstash). And other filters have this problem too, e.g. some filters on itwiki using get_matches. Solving T228677 will solve this one as well, and I'm totally on it. I'm keeping this task open, though, as it may be non-obvious that these tasks are related.

Of note, this one is a different issue which I coincidentally tried to fix together with the other task. The reason I'm not doing this one separately is that, even if we fix this bug, we may face worse troubles, like filters matching for no reason and potentially blocking lots of users like on itwp.

Change 527520 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] [WIP] Better handling of DNONE

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

Change 527520 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Better handling of DNONE

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

Leaving open until production deployment, just to be sure.

Change 528108 had a related patch set uploaded (by Urbanecm; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@wmf/1.34-wmf.16] Better handling of DNONE

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

Change 528108 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@wmf/1.34-wmf.16] Better handling of DNONE

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

Change 528111 had a related patch set uploaded (by Urbanecm; owner: Urbanecm):
[mediawiki/extensions/AbuseFilter@wmf/1.34-wmf.16] Revert "Better handling of DNONE"

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

Change 528111 merged by Urbanecm:
[mediawiki/extensions/AbuseFilter@wmf/1.34-wmf.16] Revert "Better handling of DNONE"

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

Change 528211 had a related patch set uploaded (by Urbanecm; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@wmf/1.34.0-wmf.16] Better handling of DNONE

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

Change 528211 merged by Urbanecm:
[mediawiki/extensions/AbuseFilter@wmf/1.34.0-wmf.16] Better handling of DNONE

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

Mentioned in SAL (#wikimedia-operations) [2019-08-05T18:32:22Z] <urbanecm@deploy1001> Synchronized php-1.34.0-wmf.16/extensions/AbuseFilter/: SWAT: 936a462: Better handling of DNONE (T214674, T228677) (duration: 00m 47s)

Change 528212 had a related patch set uploaded (by Urbanecm; owner: Urbanecm):
[mediawiki/extensions/AbuseFilter@wmf/1.34.0-wmf.16] Revert "Better handling of DNONE"

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

Change 528212 merged by Urbanecm:
[mediawiki/extensions/AbuseFilter@wmf/1.34.0-wmf.16] Revert "Better handling of DNONE"

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

Mentioned in SAL (#wikimedia-operations) [2019-08-05T18:39:44Z] <urbanecm@deploy1001> Synchronized php-1.34.0-wmf.16/extensions/AbuseFilter/: SWAT: d358f17: Revert "Better handling of DNONE" (T214674, T228677) (duration: 00m 47s)

Mentioned in SAL (#wikimedia-operations) [2019-08-06T11:21:38Z] <urbanecm@deploy1001> Synchronized php-1.34.0-wmf.16/extensions/AbuseFilter/: SWAT: 8cc96db: Better handling of DNONE (T214674, T228677) (duration: 00m 48s)

Daimona closed this task as Resolved.Aug 6 2019, 11:36 AM