git/commit.c, branch v2.25.4

Merge branch 'jk/cleanup-object-parsing-and-fsck'

2019-12-01T17:04:28Z

Crufty code and logic accumulated over time around the object parsing and low-level object access used in "git fsck" have been cleaned up. * jk/cleanup-object-parsing-and-fsck: (23 commits) fsck: accept an oid instead of a "struct tree" for fsck_tree() fsck: accept an oid instead of a "struct commit" for fsck_commit() fsck: accept an oid instead of a "struct tag" for fsck_tag() fsck: rename vague "oid" local variables fsck: don't require an object struct in verify_headers() fsck: don't require an object struct for fsck_ident() fsck: drop blob struct from fsck_finish() fsck: accept an oid instead of a "struct blob" for fsck_blob() fsck: don't require an object struct for report() fsck: only require an oid for skiplist functions fsck: only provide oid/type in fsck_error callback fsck: don't require object structs for display functions fsck: use oids rather than objects for object_name API fsck_describe_object(): build on our get_object_name() primitive fsck: unify object-name code fsck: require an actual buffer for non-blobs fsck: stop checking tag->tagged fsck: stop checking commit->parent counts fsck: stop checking commit->tree value commit, tag: don't set parsed bit for parse failures ...

Merge branch 'pw/post-commit-from-sequencer'

2019-11-10T09:02:12Z

"rebase -i" ceased to run post-commit hook by mistake in an earlier update, which has been corrected. * pw/post-commit-from-sequencer: sequencer: run post-commit hook move run_commit_hook() to libgit and use it there sequencer.h fix placement of #endif t3404: remove uneeded calls to set_fake_editor t3404: set $EDITOR in subshell t3404: remove unnecessary subshell

commit, tag: don't set parsed bit for parse failures

2019-10-28T05:04:49Z

If we can't parse a commit, then parse_commit() will return an error code. But it _also_ sets the "parsed" flag, which tells us not to bother trying to re-parse the object. That means that subsequent parses have no idea that the information in the struct may be bogus. I.e., doing this: parse_commit(commit); ... if (parse_commit(commit) < 0) die("commit is broken"); will never trigger the die(). The second parse_commit() will see the "parsed" flag and quietly return success. There are two obvious ways to fix this: 1. Stop setting "parsed" until we've successfully parsed. 2. Keep a second "corrupt" flag to indicate that we saw an error (and when the parsed flag is set, return 0/-1 depending on the corrupt flag). This patch does option 1. The obvious downside versus option 2 is that we might continually re-parse a broken object. But in practice, corruption like this is rare, and we typically die() or return an error in the caller. So it's OK not to worry about optimizing for corruption. And it's much simpler: we don't need to use an extra bit in the object struct, and callers which check the "parsed" flag don't need to learn about the corrupt bit, too. There's no new test here, because this case is already covered in t5318. Note that we do need to update the expected message there, because we now detect the problem in the return from "parse_commit()", and not with a separate check for a NULL tree. In fact, we can now ditch that explicit tree check entirely, as we're covered robustly by this change (and the previous recent change to treat a NULL tree as a parse error). We'll also give tags the same treatment. I don't know offhand of any cases where the problem can be triggered (it implies somebody ignoring a parse error earlier in the process), but consistently returning an error should cause the least surprise. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano

parse_commit_buffer(): treat lookup_tree() failure as parse error

2019-10-21T02:15:23Z

If parsing a commit yields a valid tree oid, but we've seen that same oid as a non-tree in the same process, the resulting commit struct will end up with a NULL tree pointer, but not otherwise report a parsing failure. That's perhaps convenient for callers which want to operate on even partially corrupt commits (e.g., by still looking at the parents). But it leaves a potential trap for most callers, who now have to manually check for a NULL tree. Most do not, and it's likely that there are possible segfaults lurking. I say "possible" because there are many candidates, and I don't think it's worth following through on reproducing them when we can just fix them all in one spot. And certainly we _have_ seen real-world cases, such as the one fixed by 806278dead (commit-graph.c: handle corrupt/missing trees, 2019-09-05). Note that we can't quite drop the check in the caller added by that commit yet, as there's some subtlety with repeated parsings (which will be addressed in a future commit). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano

parse_commit_buffer(): treat lookup_commit() failure as parse error

2019-10-21T02:15:23Z

While parsing the parents of a commit, if we are able to parse an actual oid but lookup_commit() fails on it (because we previously saw it in this process as a different object type), we silently omit the parent and do not report any error to the caller. The caller has no way of knowing this happened, because even an empty parent list is a valid parse result. As a result, it's possible to fool our "rev-list" connectivity check into accepting a corrupted set of objects. There's a test for this case already in t6102, but unfortunately it has a slight error. It creates a broken commit with a parent line pointing to a blob, and then checks that rev-list notices the problem in two cases: 1. the "lone" case: we traverse the broken commit by itself (here we try to actually load the blob from disk and find out that it's not a commit) 2. the "seen" case: we parse the blob earlier in the process, and then when calling lookup_commit() we realize immediately that it's not a commit The "seen" variant for this test mistakenly parsed another commit instead of the blob, meaning that we were actually just testing the "lone" case again. Changing that reveals the breakage (and shows that this fixes it). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano

move run_commit_hook() to libgit and use it there

2019-10-16T01:30:51Z

This function was declared in commit.h but was implemented in builtin/commit.c so was not part of libgit. Move it to libgit so we can use it in the sequencer. This simplifies the implementation of run_prepare_commit_msg_hook() and will be used in the next commit. Signed-off-by: Phillip Wood Signed-off-by: Junio C Hamano

Merge branch 'tb/commit-graph-harden'

2019-10-07T02:32:58Z

The code to parse and use the commit-graph file has been made more robust against corrupted input. * tb/commit-graph-harden: commit-graph.c: handle corrupt/missing trees commit-graph.c: handle commit parsing errors t/t5318: introduce failing 'git commit-graph write' tests

Merge branch 'mh/release-commit-memory-fix'

2019-09-30T04:19:25Z

Leakfix. * mh/release-commit-memory-fix: commit: free the right buffer in release_commit_memory

commit-graph.c: handle corrupt/missing trees

2019-09-09T17:55:59Z

Apply similar treatment as in the previous commit to handle an unchecked call to 'get_commit_tree_oid()'. Previously, a NULL return value from this function would be immediately dereferenced with '->hash', and then cause a segfault. Before dereferencing to access the 'hash' member, check the return value of 'get_commit_tree_oid()' to make sure that it is not NULL. To make this check correct, a related change is also needed in 'commit.c', which is to check the return value of 'get_commit_tree' before taking its address. If 'get_commit_tree' returns NULL, we encounter an undefined behavior when taking the address of the return value of 'get_commit_tree' and then taking '->object.oid'. (On my system, this is memory address 0x8, which is obviously wrong). Fix this by making sure that 'get_commit_tree' returns something non-NULL before digging through a structure that is not there, thus preventing a segfault down the line in the commit graph code. Signed-off-by: Taylor Blau Signed-off-by: Junio C Hamano

commit: free the right buffer in release_commit_memory

2019-08-26T17:53:25Z

The index field in the commit object is used to find the buffer corresponding to that commit in the buffer_slab. Resetting it first means free_commit_buffer is not going to free the right buffer. Signed-off-by: Mike Hommey Signed-off-by: Junio C Hamano