@matheusbsbs / matheus-bagliano / commits / 23a173a761
Fork 0

fsck: require an actual buffer for non-blobs

The fsck_object() function takes in a buffer, but also a "struct object". The rules for using these vary between types: - for a commit, we'll use the provided buffer; if it's NULL, we'll fall back to get_commit_buffer(), which loads from either an in-memory cache or from disk. If the latter fails, we'd die(), which is non-ideal for fsck. - for a tag, a NULL buffer will fall back to loading the object from disk (and failure would lead to an fsck error) - for a tree, we _never_ look at the provided buffer, and always use tree->buffer - for a blob, we usually don't look at the buffer at all, unless it has been marked as a .gitmodule file. In that case we check the buffer given to us, or assume a NULL buffer is a very large blob (and complain about it) This is much more complex than it needs to be. It turns out that nobody ever feeds a NULL buffer that isn't a blob: - git-fsck calls fsck_object() only from fsck_obj(). That in turn is called by one of: - fsck_obj_buffer(), which is a callback to verify_pack(), which unpacks everything except large blobs into a buffer (see pack-check.c, lines 131-141). - fsck_loose(), which hits a BUG() on non-blobs with a NULL buffer (builtin/fsck.c, lines 639-640) And in either case, we'll have just called parse_object_buffer() anyway, which would segfault on a NULL buffer for commits or tags (not for trees, but it would install a NULL tree->buffer which would later cause a segfault) - git-index-pack asserts that the buffer is non-NULL unless the object is a blob (see builtin/index-pack.c, line 832) - git-unpack-objects always writes a non-NULL buffer into its obj_buffer hash, which is then fed to fsck_object(). (There is actually a funny thing here where it does not store blob buffers at all, nor does it call fsck on them; it does check any needed blobs via fsck_finish() though). Let's make the rules simpler, which reduces the amount of code and gives us more flexibility in refactoring the fsck code. The new rules are: - only blobs are allowed to pass a NULL buffer - we always use the provided buffer, never pulling information from the object struct We don't have to adjust any callers, because they were already adhering to these. Note that we do drop a few fsck identifiers for missing tags, but that was all dead code (because nobody passed a NULL tag buffer). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Oct 18, 2019 at 00:54 UTC 23a173a761c9ed9a1e90167386e8b908728f27c0
2 files changed +14 -43
fsck.c
+9 -42
@@ -49,13 +49,11 @@ static struct oidset gitmodules_done = OIDSET_INIT;
49 FUNC(MISSING_SPACE_BEFORE_EMAIL, ERROR) \
50 FUNC(MISSING_TAG, ERROR) \
51 FUNC(MISSING_TAG_ENTRY, ERROR) \
52 - FUNC(MISSING_TAG_OBJECT, ERROR) \
52 FUNC(MISSING_TREE, ERROR) \
53 FUNC(MISSING_TREE_OBJECT, ERROR) \
54 FUNC(MISSING_TYPE, ERROR) \
55 FUNC(MISSING_TYPE_ENTRY, ERROR) \
56 FUNC(MULTIPLE_AUTHORS, ERROR) \
58 - FUNC(TAG_OBJECT_NOT_TAG, ERROR) \
57 FUNC(TREE_NOT_SORTED, ERROR) \
58 FUNC(UNKNOWN_TYPE, ERROR) \
59 FUNC(ZERO_PADDED_DATE, ERROR) \
@@ -541,7 +539,9 @@ static int verify_ordered(unsigned mode1, const char *name1, unsigned mode2, con
539 return c1 < c2 ? 0 : TREE_UNORDERED;
540 }
541
544 -static int fsck_tree(struct tree *item, struct fsck_options *options)
542 +static int fsck_tree(struct tree *item,
543 + const char *buffer, unsigned long size,
544 + struct fsck_options *options)
545 {
546 int retval = 0;
547 int has_null_sha1 = 0;
@@ -558,7 +558,7 @@ static int fsck_tree(struct tree *item, struct fsck_options *options)
558 unsigned o_mode;
559 const char *o_name;
560
561 - if (init_tree_desc_gently(&desc, item->buffer, item->size)) {
561 + if (init_tree_desc_gently(&desc, buffer, size)) {
562 retval += report(options, &item->object, FSCK_MSG_BAD_TREE, "cannot be parsed as a tree");
563 return retval;
564 }
@@ -733,8 +733,8 @@ static int fsck_ident(const char **ident, struct object *obj, struct fsck_option
733 return 0;
734 }
735
736 -static int fsck_commit_buffer(struct commit *commit, const char *buffer,
737 - unsigned long size, struct fsck_options *options)
736 +static int fsck_commit(struct commit *commit, const char *buffer,
737 + unsigned long size, struct fsck_options *options)
738 {
739 struct object_id tree_oid, oid;
740 unsigned author_count;
@@ -788,47 +788,15 @@ static int fsck_commit_buffer(struct commit *commit, const char *buffer,
788 return 0;
789 }
790
791 -static int fsck_commit(struct commit *commit, const char *data,
792 - unsigned long size, struct fsck_options *options)
793 -{
794 - const char *buffer = data ? data : get_commit_buffer(commit, &size);
795 - int ret = fsck_commit_buffer(commit, buffer, size, options);
796 - if (!data)
797 - unuse_commit_buffer(commit, buffer);
798 - return ret;
799 -}
800 -
801 -static int fsck_tag(struct tag *tag, const char *data,
791 +static int fsck_tag(struct tag *tag, const char *buffer,
792 unsigned long size, struct fsck_options *options)
793 {
794 struct object_id oid;
795 int ret = 0;
806 - const char *buffer;
807 - char *to_free = NULL, *eol;
796 + char *eol;
797 struct strbuf sb = STRBUF_INIT;
798 const char *p;
799
811 - if (data)
812 - buffer = data;
813 - else {
814 - enum object_type type;
815 -
816 - buffer = to_free =
817 - read_object_file(&tag->object.oid, &type, &size);
818 - if (!buffer)
819 - return report(options, &tag->object,
820 - FSCK_MSG_MISSING_TAG_OBJECT,
821 - "cannot read tag object");
822 -
823 - if (type != OBJ_TAG) {
824 - ret = report(options, &tag->object,
825 - FSCK_MSG_TAG_OBJECT_NOT_TAG,
826 - "expected tag got %s",
827 - type_name(type));
828 - goto done;
829 - }
830 - }
831 -
800 ret = verify_headers(buffer, size, &tag->object, options);
801 if (ret)
802 goto done;
@@ -889,7 +857,6 @@ static int fsck_tag(struct tag *tag, const char *data,
857
858 done:
859 strbuf_release(&sb);
892 - free(to_free);
860 return ret;
861 }
862
@@ -979,7 +946,7 @@ int fsck_object(struct object *obj, void *data, unsigned long size,
946 if (obj->type == OBJ_BLOB)
947 return fsck_blob((struct blob *)obj, data, size, options);
948 if (obj->type == OBJ_TREE)
982 - return fsck_tree((struct tree *) obj, options);
949 + return fsck_tree((struct tree *) obj, data, size, options);
950 if (obj->type == OBJ_COMMIT)
951 return fsck_commit((struct commit *) obj, (const char *) data,
952 size, options);
fsck.h
+5 -1
@@ -52,7 +52,11 @@ struct fsck_options {
52 * 0 everything OK
53 */
54 int fsck_walk(struct object *obj, void *data, struct fsck_options *options);
55 -/* If NULL is passed for data, we assume the object is local and read it. */
55 +
56 +/*
57 + * Blob objects my pass a NULL data pointer, which indicates they are too large
58 + * to fit in memory. All other types must pass a real buffer.
59 + */
60 int fsck_object(struct object *obj, void *data, unsigned long size,
61 struct fsck_options *options);
62