Unvanquished Forum

I made did some cleanup to command system in Spearmint, may want to do similar in Unvanquished.

Based on this recent unv/engine-upgrade commit, I think it's all relevant.

• Have VM commands store a real function (example: CL_GameCommand), instead of hacks for NULL function (the checking if each VM wants to run the command / don't warn if command already exists).

• Only let VMs run commands they add using trap_AddCommand. (Caused by the above change; offtopic note: stops q3/teamarena ui commands from working because not added using a syscall so I cannot be committed to ioq3. :sadface: )

• Move forwarding 'unhandled' commands to server into cgame. (cgame adds the commands; so I think it should handle them. Allows engine code to be cleaner.)

• Auto remove commands added by a VM on VM shutdown (Remove all commands that use the VM's command function, example: CL_GameCommand.) It fixes switching mods/cgame and having non-existing command in tab-completion.

• Don't let VMs remove commands added by another VM. (Only remove if command uses VM's command function, example: CL_GameCommand.)

Note: My implementation of the last two assumes VMs only use one function for their commands (example: CL_GameCommand). Might want to skip if you plan to have VM give engine a different function address for each command (not possible for QVMs, possible for DLLs, I don't know anything about PNaCl).

My implementation: main commit and a probably irrelevant fix for server's new behavior of printing unknown commands added when removing CL_ForwardCommandToServer.

Feel free to use or ignore this.

As I said on IRC that commit is a bad commit taht doesn't even work, I shouldn't code when asleep.

ZTM wrote:

• Have VM commands store a real function (example: CL_GameCommand), instead of hacks for NULL function (the checking if each VM wants to run the command / don't warn if command already exists).

Yes, that's probably how I should have implemented it, I'll change it.

ZTM wrote:

• Only let VMs run commands they add using trap_AddCommand. (Caused by the above change; offtopic note: stops q3/teamarena ui commands from working because not added using a syscall so I cannot be committed to ioq3. :sadface: )

That's a good idea to have more security, however it'll wait until we have only NaCl (I don't want to refactor this twice). Put in the TODO list.

ZTM wrote:

• Move forwarding 'unhandled' commands to server into cgame. (cgame adds the commands; so I think it should handle them. Allows engine code to be cleaner.)

That too.

ZTM wrote:

• Auto remove commands added by a VM on VM shutdown (Remove all commands that use the VM's command function, example: CL_GameCommand.) It fixes switching mods/cgame and having non-existing command in tab-completion.

That's planned: commands have flags such as CGAME or GAME and we can batch remove command with a given flag. I'm not sure I'll implement it for QVM's or wait for NaCl.

ZTM wrote:

• Don't let VMs remove commands added by another VM. (Only remove if command uses VM's command function, example: CL_GameCommand.)

Indeed it will improve security and I'd like to do the same for Cvars. It should be somewhat easy to implement using command environments. It will require changes to the gamelogic (for example team-specific configuration files won't work with this because exec will be considered "unsafe") so I'll save it for later.

Our trap calls are currently very unsafe (reading from a pointer given as an arg to a trap call without checking it is in the qvm memory, ...) but I think we'll be able to improve security a lot with NaCl that forces copies.

Thanks for the comments, here is a granger for you

kangz wrote:

Our trap calls are currently very unsafe (reading from a pointer given as an arg to a trap call without checking it is in the qvm memory, ...) but I think we'll be able to improve security a lot with NaCl that forces copies.

That's why I put in a lot of VM_Check*() calls and added an overflow buffer at the end of the VM data space. (It's possible that a few were missed and/or added since without checks.)

My point was that it wasn't enforced and it is easy to miss them (and I think I've seen several missing VM_Check). NaCl doesn't have this problem because it copies everything (or mmap but for very special cases and we know where).

kangz wrote:

My point was that it wasn't enforced and it is easy to miss them (and I think I've seen several missing VM_Check). NaCl doesn't have this problem because it copies everything (or mmap but for very special cases and we know where).

Any which you want to add, by all means do so – even were we to switch to pNaCl next month, I'd still consider it worth it since, for a start, it'd be possible for me to cherry-pick onto the debian branch and do a maintenance release.