New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CSM fixes: load mods after flavours & add flavour to block mod loading #6738

Merged
merged 6 commits into from Dec 11, 2017

Conversation

@nerzhul
Member

nerzhul commented Dec 5, 2017

This is a WIP i will integrate more features after
It will supersede #6718

  • Load mods after flavour reception
  • Split builtin & mods loading
  • Add a flavour to disable mod loading
@paramat

This comment has been minimized.

Show comment
Hide comment
@paramat

paramat Dec 5, 2017

Member

Thanks, appreciated.

Member

paramat commented Dec 5, 2017

Thanks, appreciated.

@rubenwardy rubenwardy added the Feature label Dec 7, 2017

@paramat

This comment has been minimized.

Show comment
Hide comment
@paramat

paramat Dec 7, 2017

Member

Send client mods from server

Good to see this =)
Maybe it's best to spilt server-sent CSM off into a separate PR? Since that will need time and lots of testing due to security issues.

Member

paramat commented Dec 7, 2017

Send client mods from server

Good to see this =)
Maybe it's best to spilt server-sent CSM off into a separate PR? Since that will need time and lots of testing due to security issues.

@nerzhul

This comment has been minimized.

Show comment
Hide comment
@nerzhul

nerzhul Dec 7, 2017

Member

yes i can split that, then just test this PR and merge it :)

Member

nerzhul commented Dec 7, 2017

yes i can split that, then just test this PR and merge it :)

@paramat

This comment has been minimized.

Show comment
Hide comment
@paramat

paramat Dec 7, 2017

Member

Ok i'll test this.

Member

paramat commented Dec 7, 2017

Ok i'll test this.

@Ekdohibs

This is a good PR, but to be really useful, the builtin mod code needs to be checked for integrity; so I'll +0.5 it for now.

// Now we have flavours, load mods if it's enabled
// Note: this should be moved after mods receptions from server instead
loadMods();

This comment has been minimized.

@Ekdohibs

Ekdohibs Dec 7, 2017

Member

It might be good to leave a note in the documentation that if the server doesn't send flavour limits, then no client mods will load.

@Ekdohibs

Ekdohibs Dec 7, 2017

Member

It might be good to leave a note in the documentation that if the server doesn't send flavour limits, then no client mods will load.

This comment has been minimized.

@nerzhul

nerzhul Dec 7, 2017

Member

@Ekdohibs exact, it's also a interesting side effect, but i should move that outside of flavour, after pushing mods from server packet (when feature was added

@nerzhul

nerzhul Dec 7, 2017

Member

@Ekdohibs exact, it's also a interesting side effect, but i should move that outside of flavour, after pushing mods from server packet (when feature was added

This comment has been minimized.

@sfan5

sfan5 Dec 7, 2017

Member

With the current implementation the server can also force client-side mods to be loaded twice.
The client should probably only accept the CSMFlavourLimits packet once.

@sfan5

sfan5 Dec 7, 2017

Member

With the current implementation the server can also force client-side mods to be loaded twice.
The client should probably only accept the CSMFlavourLimits packet once.

This comment has been minimized.

@nerzhul

nerzhul Dec 7, 2017

Member

@sfan5 exact you are right i will permit to accept packet twice but not loading mods, it will permit to adjust flavours dynamically at runtime if needed later (we can add a SSM interface to manipulate the flag and change flavours)

@nerzhul

nerzhul Dec 7, 2017

Member

@sfan5 exact you are right i will permit to accept packet twice but not loading mods, it will permit to adjust flavours dynamically at runtime if needed later (we can add a SSM interface to manipulate the flag and change flavours)

{
// Load builtin
scanModIntoMemory(BUILTIN_MOD_NAME, getBuiltinLuaPath());
// If modding is not enabled, don't load mods, just builtin
m_script->loadModFromMemory(BUILTIN_MOD_NAME);

This comment has been minimized.

@Ekdohibs

Ekdohibs Dec 7, 2017

Member

If builtin mod is loaded even if other mods are not, builtin mod needs to be checked for integrity (that means, there should be a hash that builtin is checked against; and the compiling pipeline needs to be changed to compute this hash and define it for C++ to know) -- the point being that it should not be possible to modify the builtin mod code without recompiling.

@Ekdohibs

Ekdohibs Dec 7, 2017

Member

If builtin mod is loaded even if other mods are not, builtin mod needs to be checked for integrity (that means, there should be a hash that builtin is checked against; and the compiling pipeline needs to be changed to compute this hash and define it for C++ to know) -- the point being that it should not be possible to modify the builtin mod code without recompiling.

This comment has been minimized.

@nerzhul

nerzhul Dec 7, 2017

Member

@Ekdohibs i approve

@nerzhul

nerzhul Dec 7, 2017

Member

@Ekdohibs i approve

This comment has been minimized.

@paramat

paramat Dec 7, 2017

Member

Yes please.

@paramat

paramat Dec 7, 2017

Member

Yes please.

This comment has been minimized.

@sfan5

sfan5 Dec 7, 2017

Member

I further suggest not checking the builtin integrity unless absolutely necessary (requested by server), so people can still modify builtin in other cases.

@sfan5

sfan5 Dec 7, 2017

Member

I further suggest not checking the builtin integrity unless absolutely necessary (requested by server), so people can still modify builtin in other cases.

This comment has been minimized.

@nerzhul

nerzhul Dec 7, 2017

Member

@sfan5 can you precise the conditions you think about ?

@nerzhul

nerzhul Dec 7, 2017

Member

@sfan5 can you precise the conditions you think about ?

This comment has been minimized.

@sfan5

sfan5 Dec 7, 2017

Member

what I mean is: only check builtin integrity if the server indicates CSM_FL_LOAD_CLIENT_MODS

@sfan5

sfan5 Dec 7, 2017

Member

what I mean is: only check builtin integrity if the server indicates CSM_FL_LOAD_CLIENT_MODS

This comment has been minimized.

@nerzhul

nerzhul Dec 7, 2017

Member

then if server blocks CSM mods loading, verify the integrity, we agree ?
I just want to be clear about what to code :)
Note: builtin will be loaded before flavour are received permitting to have lua pre-inits when client connects to server. I will add the check when loading mods, disconnecting player with error on failure.

@nerzhul

nerzhul Dec 7, 2017

Member

then if server blocks CSM mods loading, verify the integrity, we agree ?
I just want to be clear about what to code :)
Note: builtin will be loaded before flavour are received permitting to have lua pre-inits when client connects to server. I will add the check when loading mods, disconnecting player with error on failure.

This comment has been minimized.

@sfan5

sfan5 Dec 7, 2017

Member

Yes that sounds fine

@sfan5

sfan5 Dec 7, 2017

Member

Yes that sounds fine

@nerzhul nerzhul removed the WIP label Dec 9, 2017

@nerzhul

This comment has been minimized.

Show comment
Hide comment
@nerzhul

nerzhul Dec 9, 2017

Member

I suggest to merge it as-is to have the first improvement, i need more time to work on builtin integrity check and i tend to prefer having first this improvement and add the next a little bit later

Member

nerzhul commented Dec 9, 2017

I suggest to merge it as-is to have the first improvement, i need more time to work on builtin integrity check and i tend to prefer having first this improvement and add the next a little bit later

@SmallJoker

Works. 👍 as soon the comment is addressed.

@nerzhul

This comment has been minimized.

Show comment
Hide comment
@nerzhul

nerzhul Dec 9, 2017

Member

@SmallJoker it's now fixed

Member

nerzhul commented Dec 9, 2017

@SmallJoker it's now fixed

Show outdated Hide outdated src/network/networkprotocol.h Outdated
@paramat

This comment has been minimized.

Show comment
Hide comment
@paramat

paramat Dec 11, 2017

Member

Thanks, i support the concept, no objections.

Member

paramat commented Dec 11, 2017

Thanks, i support the concept, no objections.

@nerzhul nerzhul merged commit 308bb69 into minetest:master Dec 11, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@nerzhul nerzhul deleted the nerzhul:csm_fixes branch Dec 11, 2017

@BluebirdGreycoat

This comment has been minimized.

Show comment
Hide comment
@BluebirdGreycoat

BluebirdGreycoat Dec 29, 2017

Contributor

Thank you all for this.

Contributor

BluebirdGreycoat commented Dec 29, 2017

Thank you all for this.

@nerzhul nerzhul added this to Done in Minetest 5.0.0 Jan 4, 2018

t0ny2 pushed a commit to t0ny2/minetest that referenced this pull request Jan 23, 2018

CSM fixes: load mods after flavours & add flavour to block mod loading (
minetest#6738)

* CSM fixes: load mods after flavours & add flavour to block mod loading

* Don't permit to load mods twice

* Prepare builtin integrity global algorithm

* Add missing doc & use a nicer byteflag for LOAD_CLIENT_MODS flavour

* flag typo fix

* Invert CSM_FL_LOOKUP_NODES & CSM_FL_LOAD_CLIENT_MODS ids
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment