Last modified: 2013-12-04 16:23:20 UTC
(Note: I do not fully understand the defect, so I will try to detail as much as possible. My apologies in advance for the length.) ------- Summary ------- Scribunto allows creation of invalid scripts that will fail when invoked. The invalid script appears to be related to reusing object references for module variables: local colors = {} do local FRA = {val = 1} colors.FRA = FRA colors.MTQ = FRA end p.colors = colors ------------ Reproduction ------------ * Create a module called "Module:Infobox_road/color" * Copy the latest version from enwiki: http://en.wikipedia.org/w/index.php?title=Module:Infobox_road/color&oldid=552689438 * Go to any page and run the following {{#invoke:Infobox_road/color|color}} On Linux, "Fatal exception of type ScribuntoException" is generated On Windows, "The interpreter exited with status 1" is generated On http://en.wikipedia.org/w/index.php?title=Wikipedia:Sandbox&action=edit, "'background:#cedff2;'" is generated. ---------- Workaround ---------- * Find the following section in the above script colors.FRA = FRA colors.MTQ = FRA * Delete the line "colors.MTQ = FRA" * Rerun the #invoke. The page will now generate "'background:#cedff2;'". ----- Notes ----- * The Infobox_road/color module can be reduced to the following local p = {} local colors = {} do local FRA = {val = 1} colors.FRA = FRA colors.MTQ = FRA end p.colors = colors function p.color(frame) return 'color_val' end return p This will generate the same error. Removing "colors.MTQ = FRA" will fix the issue * I do not know why I get different error messages on both machines. They are both running MediaWiki 1.21 on PHP 5.4 with the latest master of Scribunto (as of today). If this cannot be reproduced, please let me know your version of MediaWiki / Scribunto / PHP / OS and I will try to reproduce with it. * I do not know why http://en.wikipedia.org does not fail when invoking the same. My best guess is that there may be some version discrepancy with English Wikipedia in either Lua or Scribunto.
Your error only occurs in LuaStandalone, and also goes away if you comment out the line "p.colors = colors". The problem is that LuaStandalone cannot return data structures to PHP that incorporate references. While it would almost be possible to do this,[1] PHP does a shallow clone when passing the array around unless "&" is used appropriately. So for something like "p.recurse = p", when it got back to PHP $p['recurse'] !== $p although $p['recurse']['recurse'] === $p['recurse']. And if PHP does $x = $p['recurse'], now $x['recurse'] !== $x. The error doesn't occur on enwiki because LuaSandbox just silently replaces the recursive object with an empty "placeholder". Odd that it was done in two different ways there, that should probably be fixed. [1]: I looked at it back in December, Lua→PHP is easy but there are unresolved issues for PHP→Lua.
Related URL: https://gerrit.wikimedia.org/r/63565 (Gerrit Change I99c80549aebbd2ae1b7fbf8ab11f8588b40855fd)
Thanks for the explanation. I didn't realize that enwiki was running LuaSandbox. (I'll download and build this later for my own machines.) For now, I follow your logic well enough to understand why this occurs / is difficult to fix. I also appreciate your check-in to make Standalone and Sandbox behave the same. It was quite confusing looking at two different behaviors. One other question: I know you said this was silently broken in LuaSandbox, but shouldn't enwiki return incorrect data for the MTQ invocation? I tried the below on enwiki's sandbox, and MTQ is returning the same values as FRA. It almost appears as if the MTQ assignment works and the FRA object reference was cloned properly to it (i.e.: "colors.MTQ = FRA".) <!-- as per the module, both FRA and MTQ are the same for type A --> <pre>FRA:addTypesAsColor({"A"}, "background:#0079C1; color:#fff;")</pre> {{#invoke:Infobox road/color|color|country=FRA|type=A}}<br/> {{#invoke:Infobox road/color|color|country=MTQ|type=A}}<br/> <!-- as per the module, both FRA and MTQ are the same for type N --> <pre>FRA:addTypesAsColor({"N"}, "background:#006A4D; color:#fff;")</pre> {{#invoke:Infobox road/color|color|country=FRA|type=N}}<br/> {{#invoke:Infobox road/color|color|country=MTQ|type=N}}<br/>
This problem affects only PHP↔Lua. When Lua accesses FRA or MTQ, it works fine. And it even works for something like this local p = {} function p.foo() return 'ok' end p.bar = p.foo return p Although I expect PHP sees two different functions rather than two references to the same function, that doesn't really matter much. Off the top of my head, I can't think of a way where you could actually *see* this breakage on enwiki right now.
Okay. Thanks again for the explanation. So if I understand correctly (and I very well may not), the Infobox_road/color Module will not break on enwiki today (because it is Sandbox / Lua-only). However, https://gerrit.wikimedia.org/r/#/c/63565/ will force it to break in the future? (with the goal that Sandbox and Standalone should behave the same?) If so, I'll go to the talk page for Infobox_road/color and recommend a change based on this ticket. If not, and this Module behaves the same on enwiki in the future as it does today, then I'm okay with the situation as is. I'll just need to make some adjustments on my side to set up a Sandbox-like environment.
(In reply to comment #5) > > So if I understand correctly (and I very well may not), the > Infobox_road/color > Module will not break on enwiki today (because it is Sandbox / Lua-only). Basically yes, since the manner in which it breaks is not accessible. > However, https://gerrit.wikimedia.org/r/#/c/63565/ will force it to break in > the future? (with the goal that Sandbox and Standalone should behave the > same?) Yes, assuming that actually gets merged. In the future it would cause a script error rather than silently breaking, so other situations where the current breakage actually is accessible would generate proper errors. > If so, I'll go to the talk page for Infobox_road/color and recommend a change > based on this ticket. Please do. As I mentioned earlier, the proper fix is to not place that "colors" subtable in the table of functions being returned to PHP for #invoke. Should direct access to that colors table be needed by some other module loading this one with require(), you could split it to a separate module (which could probably be loaded with mw.loadData(), if it's going to be used from many #invoke calls in one page) or you could make an accessor function to return it as needed.
> Please do. As I mentioned earlier, the proper fix is to not place that "colors" > subtable in the table of functions being returned to PHP for #invoke. Ah! I now understand your earlier point. I thought that "p.colors = colors" was valid usage (it looks harmless to the untrained eye). I've posted accordingly to the Talk page: http://en.wikipedia.org/wiki/Module_talk:Infobox_road/color Thanks again!
As the original writer of the module, I can say that the offending code has been removed. Calls from other modules, in particular the to-be-written Module:Infobox road, should go through p._color. Thank you, Alexander
Change 63565 merged by jenkins-bot: Invalid data should cause an error, not a silent placeholder https://gerrit.wikimedia.org/r/63565
Change is merged, so marking this as fixed. Note that the luasandbox PHP extension is not part of the normal deployment process, so I'm not sure when this will be deployed to WMF wikis.
