Recursive data structures are silently broken in LuaSandbox, cause an error in LuaStandalone, when sent to PHP
Closed, ResolvedPublic

Description

Author: gnosygnu

Description:
(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"
  • 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.

Version: master
Severity: normal

bzimport set Reference to bz48393.
bzimport created this task.Via LegacyMay 12 2013, 10:52 PM
Anomie added a comment.Via ConduitMay 13 2013, 4:56 PM

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.

gerritbot added a comment.Via ConduitMay 13 2013, 8:28 PM

Related URL: https://gerrit.wikimedia.org/r/63565 (Gerrit Change I99c80549aebbd2ae1b7fbf8ab11f8588b40855fd)

bzimport added a comment.Via ConduitMay 14 2013, 2:31 AM

gnosygnu wrote:

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/>

Anomie added a comment.Via ConduitMay 14 2013, 1:53 PM

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.

bzimport added a comment.Via ConduitMay 15 2013, 3:02 AM

gnosygnu wrote:

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.

Anomie added a comment.Via ConduitMay 15 2013, 2:04 PM

(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.

bzimport added a comment.Via ConduitMay 16 2013, 2:18 AM

gnosygnu wrote:

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!

happy5214 added a comment.Via ConduitMay 16 2013, 8:35 AM

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

gerritbot added a comment.Via ConduitSep 8 2013, 10:58 PM

Change 63565 merged by jenkins-bot:
Invalid data should cause an error, not a silent placeholder

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

Anomie added a comment.Via ConduitSep 9 2013, 3:50 PM

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.

Add Comment

Column Prototype
This is a very early prototype of a persistent column. It is not expected to work yet, and leaving it open will activate other new features which will break things. Press "\" (backslash) on your keyboard to close it now.