Skip to content
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

Check json item definitions consistency #4687

Merged
merged 6 commits into from
Nov 28, 2013

Conversation

BevapDin
Copy link
Contributor

This adds some checks for consistent item type definitions and references, (only done with --verifyjson).
If a check fails, a debug message is printed.

Checks that components/tools & result of recipes refer to existing item types in item_controller (prevents the "char_smoke" in #4680).

Same for each item in each item group (prevents things like #4683, #4567).

Checks that it_ammo::casing, it_tool::reverts_to, it_comest::tool, it_comest::container are defined item types (or "NULL" / "null") - (prevents things like #3721, #3360).

Checks various ammo references - they should be listed in ammo_name() and checks that at least one item type of this ammo exists. This is skipped for tools with no max_charges (folding bicycle uses ammo "none", most other tools use "NULL").

Checks that item material (m1, m2) refer to known materials.
material_type::find_material will return a NULL pointer on invalid materials, can be seen when a sealed 3l-jar of kompot (or any mutagen) is put into an acid pool - will cause a segfault ("Tried to get invalid material: ..."). This can easily happen: a zombie dies and drops jar_kompot, a spitter creates an acid pool.

As a fix for the material bug: changed the material for mutagens from "NULL" to "null", as this is the material id used in materials.json.

jar_kompot has material "water", pudding & yoghurt have material "milk" - both materials are not listed in materials.json - this will trigger the same bug as above. Therefor material_type::find_material returns now a pointer to a null material instead of a null pointer.

@narc0tiq
Copy link
Contributor

Note that --jsonverify was a great big security hole for Jenkins because of how it worked -- it effectively meant anyone could execute arbitrary code as part of the make process, which executed automatically for every single new commit to any pull request. It has been replaced with a separate executable named chkjson, which only depends on its own source and json.cpp.

As a result, any changes outside of json.cpp or chkjson.cpp will not be automatically executed during the build process. Furthermore, introducing any extra dependencies to chkjson.cpp will make the attack surface that much larger and would almost certainly be a bad idea.

I will not allow anyone who comes along to blow away the Jenkins just because he's feeling a little clever today and "wants to see what will happen".

@BevapDin
Copy link
Contributor Author

Sorry for that, I thought that the --jsonverify would only be called manually as a help for modders. These functions were intended to be run separate and only on explicit invocation.
I'm really sorry if that caused any trouble. I'll better delete this than.

@BevapDin BevapDin closed this Nov 25, 2013
@kevingranade
Copy link
Member

We very much want these checks, but maybe hooked up to a new flag if having
jenkins run them is problematic. I was hoping we could run a set of json
tests on pr because so many of them are json only changes now.
Could we have a known good exe run on the new json? Eg the latest mainline
build?
Regardless, I'd like any json consistency checks to run at startup on all
debug builds by default, and release builds too as long as they don't take
too long.

@narc0tiq
Copy link
Contributor

@BevapDin -- The consistency checks are good, and as I explained, the Jenkins job is not using --jsonverify anymore; the rest of that comment was context, for clarity about why it would be a bad idea for it to do so, and was only tangential to the subject here.

@kevingranade -- using known-good source is what we need to be doing, yes; and I agree consistency checks would be wonderful to have on every single JSON-changing PR, for obvious reasons.

I'm gonna hit "Reopen" here. It was never my intent to discourage obviously good automated checks from being included.

@narc0tiq narc0tiq reopened this Nov 26, 2013
@kevingranade kevingranade merged commit 3709ffe into CleverRaven:master Nov 28, 2013
@BevapDin BevapDin deleted the check-json-consistency branch December 6, 2013 00:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants