Stricter object dependency checking during creation #3633
Conversation
|
Questions:
|
| bool valid = true; | ||
|
|
||
| if (git_object__strict_input_validation) { | ||
| if (git_object_lookup(&obj, repo, id, type) < 0) |
There was a problem hiding this comment.
Do we want to maybe git_odb_read_header instead, to check for the type? Seems much more cheap than a full object lookup.
There was a problem hiding this comment.
Yes, that's true, but what would be really nice is to have an object_lookup_type or something that would use the cached object if it was in the cache and then did a odb_read_header if it wasn't. One sec.
There was a problem hiding this comment.
Okay, I did indeed add git_object__lookup_type which will use the cache when possible and then go to git_odb_read_header when the requested object is not in the cache. Thanks, @vmg !
When `GIT_OPT_ENABLE_STRICT_OBJECT_CREATION` is turned on, validate the tree and parent ids given to commit creation functions.
When `GIT_OPT_ENABLE_STRICT_OBJECT_CREATION` is turned on, validate the tree and parent ids given to treebuilder insertion.
e2f9b6e to
83574d7
Compare
When `GIT_OPT_ENABLE_STRICT_OBJECT_CREATION` is turned on, validate the index entries given to `git_index_add`.
This allows lighter weight validation in `git_object__is_valid` that does not require reading the entire object.
Use legitimate (existing) object IDs in tests so that we have the ability to turn on strict object validation when running tests.
83574d7 to
f2dddf5
Compare
|
Okay, after some thought, I think that it should be turned on by default. I think that most consumers are willing to sacrifice a quick lookup for safety. Those of us that are in an environment where you're confident about the dependencies of the objects that you're writing then you can turn this setting off. As a result, I pulled reference creation target validation into the same check. A lot of tests assumed that they could just spam random (or cleverly named) object IDs into the index. I updated these tests to use legitimate object IDs so that we can turn this off by default. Any objections to this being on by default? |
Stricter object dependency checking during creation
The documentation for `git_treebuilder_insert` erroneously states that we do not validate that the entry being inserted exists. We do, as of #3633. Update the documentation to reflect the new reality.
At present, we are very loose about the creation of objects that are based on other objects; for example, when calling
git_commit_create, we do not check that the dependent objects (its tree, its commits) are actually valid objects, or that they are the requisite type.This PR adds a new option
GIT_OPT_ENABLE_STRICT_OBJECT_CREATION, that will turn on validation of the referenced objects: that they exist, and that they are the required type.This validates:
git_commit_create: that the tree is a tree, and exists, and that the parents are commits and existgit_index_add: that the index entry object exists and is of the specified type, andgit_treebuilder_insert: that the specified entry exists and is of the specified typeThese checks are still optional, and default to off, for performance.
/cc @vmg