Re: [crossfire] Hardening plugin system
> If its a choice of: > > a) when callback to set value is used, we set the value and then mark a > flag that the object has been modified, and when function is finished, > we check sanity of object, > > or > > b) when callback to set value is made, we check the validity > > I'd personally choose b - detecting the error right when it happens is > always better - it makes it easier to fix the problem. Actually, i'd chose b too - i think i meant that, when plugin finished, server can send fix_player or fix_object to the client, meaning the plugin can just not care. > but at some level, it can never be 100% foolproof. The fact the server > crashes occasionally is proof that even the C code isn't foolproof. You > get the complications of some value being valid for one object type, but > not another, and I don't think there is any real way that can ever be > checked (but that same problem occurs in maps also - mapmaker could set > some value to something invalid) True. Guess there should be checks at many levels in the plugin code :) (server <-> common plugin lib <-> Pyhton plugin for instance) Nicolas ___ crossfire mailing list crossfire@metalforge.org http://mailman.metalforge.org/mailman/listinfo/crossfire
Re: [crossfire] Hardening plugin system
Nicolas Weeger wrote: Presumably for that last point to work, all the functions the change values in the plugin code would need to set some flag. Otherwise, I don't see how the server can know an object changes. Exactly. But remember plugin isn't supposed to access object's fields directly but use callbacks which can set such a flag. The question is then why not just check for the sanity at that time? If its a choice of: a) when callback to set value is used, we set the value and then mark a flag that the object has been modified, and when function is finished, we check sanity of object, or b) when callback to set value is made, we check the validity I'd personally choose b - detecting the error right when it happens is always better - it makes it easier to fix the problem. True, but for instance in the case of the Python plugin at what point? In the Python-C wrappers, ie Python plugin side, in the plugin_common code, or in the server-side code? 3rd case is the more foolproof, but can be a pain to propagate back (how will the C/Python code know the value is invalid? Special integer value for return? Parameter by reference?) Depends on the value. In your initial post, you mentioned scripts setting the Str value to 50. Well, there is a MAX_STAT defined, which the Python-C wrappers could check (or plugin common code). Some things are certainly harder- not everything has a clear define for min/max value. but at some level, it can never be 100% foolproof. The fact the server crashes occasionally is proof that even the C code isn't foolproof. You get the complications of some value being valid for one object type, but not another, and I don't think there is any real way that can ever be checked (but that same problem occurs in maps also - mapmaker could set some value to something invalid) ___ crossfire mailing list crossfire@metalforge.org http://mailman.metalforge.org/mailman/listinfo/crossfire
Re: [crossfire] Hardening plugin system
> Presumably for that last point to work, all the functions the change > values in the plugin code would need to set some flag. Otherwise, I > don't see how the server can know an object changes. Exactly. But remember plugin isn't supposed to access object's fields directly but use callbacks which can set such a flag. > I personally do think that the plugin should do basic checking - making > sure set values are valid, etc. I don't think that stuff is too hard. > And I think that may catch errors object comparison can't do. True, but for instance in the case of the Python plugin at what point? In the Python-C wrappers, ie Python plugin side, in the plugin_common code, or in the server-side code? 3rd case is the more foolproof, but can be a pain to propagate back (how will the C/Python code know the value is invalid? Special integer value for return? Parameter by reference?) > Another thing (unrelated to this) is plugin verification of > compatibility. More than once on my own working copy, I've done a make > but not done a make install, only to track it down to the plugin being > out of date. That could possibly be done. I'd then suggest sending those values at plugin init, or something like that - maybe have the plugin init function take a "long struct_size, struct* plugin_info". The structure contains basic info (version, some sizes), and the struct_size ensures you're compatible at least with the structure sent :) Nicolas ___ crossfire mailing list crossfire@metalforge.org http://mailman.metalforge.org/mailman/listinfo/crossfire
Re: [crossfire] Hardening plugin system
Nicolas Weeger wrote: Hello. Currently, a plugin can easily crash the server, which doesn't check parameters (just call a function with a NULL pointer, nice crash guaranteed). Also, server doesn't checks parameters and such, which can lead to invalid values (Str of 50 for a player...). So should that be fixed in a way a plugin can *not* crash the server through a callback, or send invalid parameters? Note that preventing a plugin ever crashing the server is hard, since the plugin itself can crash leading to server crash (and that isn't something easy to avoid i think). IMO "no" is an acceptable answer. We can after all "trust" a plugin to do the right thing. If we choose the "yes, let's harden the plugin system" option, here's my suggestion for hardening: * when plugin requests an object/map/archetype, server keeps the pointer in an array and sends the index, which is used in subsequent functions * this way, it's easy to check the pointer's validity - check index, if inbounds ok, else issue * when the plugin returns, server knows what objects were affected (array can contain a "set_parameter" field, set when a setter is called), and can send updates to player accordingly - should also simplify plugin's code. Also can recalculate stuff when object is removed, whatever. Presumably for that last point to work, all the functions the change values in the plugin code would need to set some flag. Otherwise, I don't see how the server can know an object changes. For example, plugin is called with an object whose object pointer is deadbeef. The plugin makes some changes (say changes name) and returns. The object pointer is still deadbeef. Unless the server keeps a copy of the object, and then runs a comparison, I don't see how the server can really know the object has changed. And keeping a copy, running a comparison, and updating it would seem to be a fairly costly operation (maybe not a big deal, but I could potentially see that as preventing really wide spread use of the plugin if the performance is bad). I personally do think that the plugin should do basic checking - making sure set values are valid, etc. I don't think that stuff is too hard. And I think that may catch errors object comparison can't do. For example, if a field in the object type is an sint16, the valid value is -32768 to 32767. If something tries to set the valid beyond that, it is bad. However, object comparison won't catch that - some plugin sets the value to 123,456. The value gets truncated, so is a legal value, but probably no what the plugin wants, and thus causes problems. OTOH, if the actual plugin code checks the valid for the -32768 to 32767 range, it can log an error right there. Another thing (unrelated to this) is plugin verification of compatibility. More than once on my own working copy, I've done a make but not done a make install, only to track it down to the plugin being out of date. What I think could be done is that the plugin code has code someplace like: #define PY_OB_SIZE sizeof(object) #define PY_PL_SIZE sizeof(player) etc for major structures. The server could have similar code, but different name. In the server init code, it could initialize some global variables to those values. The plugin could then compare the size it thinks the structures should be with what the server thinks they are, and if any difference, logs an error and either exits (because it is sure to crash otherwise), or disables itself. This isn't foolproof - if fields are moved in the structure but size remains the same, that won't catch the difference (if really paranoid, could add some offset checks). However, I think the case of fields moving about is low - the more common case is new fields being added or old fields being removed, and sizeof checks should catch that. ___ crossfire mailing list crossfire@metalforge.org http://mailman.metalforge.org/mailman/listinfo/crossfire
[crossfire] Hardening plugin system
Hello. Currently, a plugin can easily crash the server, which doesn't check parameters (just call a function with a NULL pointer, nice crash guaranteed). Also, server doesn't checks parameters and such, which can lead to invalid values (Str of 50 for a player...). So should that be fixed in a way a plugin can *not* crash the server through a callback, or send invalid parameters? Note that preventing a plugin ever crashing the server is hard, since the plugin itself can crash leading to server crash (and that isn't something easy to avoid i think). IMO "no" is an acceptable answer. We can after all "trust" a plugin to do the right thing. If we choose the "yes, let's harden the plugin system" option, here's my suggestion for hardening: * when plugin requests an object/map/archetype, server keeps the pointer in an array and sends the index, which is used in subsequent functions * this way, it's easy to check the pointer's validity - check index, if inbounds ok, else issue * when the plugin returns, server knows what objects were affected (array can contain a "set_parameter" field, set when a setter is called), and can send updates to player accordingly - should also simplify plugin's code. Also can recalculate stuff when object is removed, whatever. Just my 2 cents :) Nicolas ___ crossfire mailing list crossfire@metalforge.org http://mailman.metalforge.org/mailman/listinfo/crossfire
