MightyPirates / OpenComputers

Home of the OpenComputers mod for Minecraft.
https://oc.cil.li
Other
1.59k stars 430 forks source link

intercept_load() in boot/01_process.lua causes config file corruption #2434

Closed SDPhantom closed 7 years ago

SDPhantom commented 7 years ago

In stock programs that load config files using load() to write into an environment table, there's an issue where intercept_load() in _boot/01process.lua corrupts it. The function seems to write itself into the table which makes serializing while saving config raise an error.

Tested in OpenOS 1.6.5

payonel commented 7 years ago

I would need a sample repro to work against to correct this. load is redefined in OpenOS because our filesystem is virtualized. real-lua load will share the same env with any load called called in the new code chunk, and thus intercept_load sovles the problem by storing an upvalue of the env used on the parent and passing that to any child load call that omits the env var.

SDPhantom commented 7 years ago

I was registering a daemon with rc when it the serialization lib threw an error about trying to serialize a function. I tracked it down to the code that deals with writing its config file. I've seen this same code in edit and I don't doubt it exists elsewhere as well.

Long story short, I inspected the config table it was trying to serialize to identify the problem and injected code that reported write attempts to the config table into the config loading funtion of rc while repeating the registration process. The debug.traceback() I got back from it was pointing to line 36 of _boot/01process.lua. The data that made it into the config table is defined at line 26.

PS: Should I start testing against the 1.7.10 branch instead of 1.10? I'm currently using 1.10 to write some custom programs, though it may be possible for me to get the dev build for 1.7.10, extract OpenOS, and run it on 1.10.

payonel commented 7 years ago

If you have a repro for me to run I'd be happy to help. 1.7.10 gets updated first, but currently 1.10 is up to date with the latest OpenOS code.

SDPhantom commented 7 years ago

I'm still fairly new to using GitHub, so I'm not exactly sure what a "repro" is. I was guessing that it was steps to reproduce the issue that I explained previously. I even added links to the specific blocks of affected code in the current 1.10 branch, which I've checked and confirmed to exist in the current 1.7.10 branch.

For now, I can run a search for programs using the serialization lib to track down which ones are affected and issue a fix from there. Trying to fix it from intercept_load() looks like it's going to take more effort than I'm prepared to give studying exactly what it's trying to do and find an alternative solution. I thought I'd bring it to your attention since you'd know more about the OpenOS source than I would be able to figure within a reasonable amount of time.

SDPhantom commented 7 years ago

Issued a dirty fix for this. It lets serialization.serialize() continue running when it encounters an error and writes nil for values it can't process.

2437

I'm not certain what the implications are of having the injected load table from intercept_load() being overwritten, but I suppose since the config files aren't even able to call load(), this is just ends up being garbage data.

payonel commented 7 years ago

Yes, a repro are steps and a minimal sample script that reproduces the problem. A sample script file that I can run that fails due to this bug. But I've now started troubleshooting serialization and I believe I found the issue you are talking about. Providing a good bug report makes it so that the devs don't have to troubleshoot to FIND a sample that fails, it is best to just provide sample scripts that show the error. Showing where you see problems in the code should be auxiliary to that.

SDPhantom commented 7 years ago

From my test, just running this command at the shell prompt of a fresh install should do it.

rc example enable

OpenOS already includes the script referenced in this command at /etc/rc.d/example.lua

payonel commented 7 years ago

NOW THAT is a good repro :)

Yeah, it appears my fixes for the load() complexities has broken that..ouch

Well, it really points out how many people are using the rc library :(