Re: [PATCH v2 0/6] Improve tag checking in fsck and with transfer.fsckobjects
Hi Junio, On Wed, 10 Sep 2014, Junio C Hamano wrote: Junio C Hamano gits...@pobox.com writes: Johannes Schindelin johannes.schinde...@gmx.de writes: This patch series introduces detailed checking of tag objects when calling git fsck, and also when transfer.fsckobjects is set to true. To this end, the fsck machinery is reworked to accept the buffer and size of the object to check, and for commit and tag objects, we verify that the buffers contain an end of header (i.e. an empty line) to guarantee that our checks do not run beyond the buffer. Overall they looked sensible and I am trying to queue them on 'pu' for today's pushout. Thanks. I was expecting to see interesting interactions with the oops, fsck was not exiting with non-zero status in some cases fix by Peff with the patch 5/6 of this series that adds two new tests to t1450, but because the breakage in these two cases are both only warning-events and not error-events, I didn't prefix git fsck with test_must_fail after all. There were a couple of issues, thanks for pointing out that I missed something. Short story: hash-object, fsck *and* pack-objects parse tag objects as they are encountered. Therefore, it would be a bit hard to test because we would have to emulate broken hash-object and pack-objects to generate a pack containing an invalid tag object. As a compromise, I now test for the warnings to make sure that the code path is triggered, but do not explicitly test with a pack that would make index-pack --strict *fail*. Okay? Ciao, Dscho P.S.: I squashed your changes in slightly modified form. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/6] Improve tag checking in fsck and with transfer.fsckobjects
Hi Junio, On Wed, 10 Sep 2014, Johannes Schindelin wrote: Still unaddressed: - getting rid of struct object altogether in fsck (I felt this was quite a big task, getting much more familiar with the non-tag code paths, and I did not want to delay this patch series up any further) - ensuring that index-pack passes only NUL-terminated buffers to fsck (again, I am not familiar enough with the code, and IIRC the problematic unit test that revealed that these buffers are not always NUL-terminated exercised the unpack-objects code path, not index-pack, again nothing I wanted to let delay this patch series any further). To clarify: I am planning to address these issues later this year, but consider them not critical enough to finish the fsck-tag patch series. Please let me know if you disagree. Ciao, Dscho -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/6] Improve tag checking in fsck and with transfer.fsckobjects
Johannes Schindelin johannes.schinde...@gmx.de writes: This patch series introduces detailed checking of tag objects when calling git fsck, and also when transfer.fsckobjects is set to true. To this end, the fsck machinery is reworked to accept the buffer and size of the object to check, and for commit and tag objects, we verify that the buffers contain an end of header (i.e. an empty line) to guarantee that our checks do not run beyond the buffer. Overall they looked sensible and I am trying to queue them on 'pu' for today's pushout. Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/6] Improve tag checking in fsck and with transfer.fsckobjects
Junio C Hamano gits...@pobox.com writes:
Johannes Schindelin johannes.schinde...@gmx.de writes:
This patch series introduces detailed checking of tag objects when calling
git fsck, and also when transfer.fsckobjects is set to true.
To this end, the fsck machinery is reworked to accept the buffer and size
of the object to check, and for commit and tag objects, we verify that the
buffers contain an end of header (i.e. an empty line) to guarantee that our
checks do not run beyond the buffer.
Overall they looked sensible and I am trying to queue them on 'pu'
for today's pushout.
Thanks.
I was expecting to see interesting interactions with the oops, fsck
was not exiting with non-zero status in some cases fix by Peff with
the patch 5/6 of this series that adds two new tests to t1450, but
because the breakage in these two cases are both only warning-events
and not error-events, I didn't prefix git fsck with test_must_fail
after all.
Are there some more serious kind of breakages that the new code
actually catches as errors which the new tests are not exercising?
The second test however did need the fix I mentioned in the review
to skip ident validation when we know the buffer does not point at a
potential ident to pass, though (see below, which is what I queued
on the tip of your series as a SQUASH??? single fix-up patch).
Please do an equivalent in the individual patches that introduce
these buglets in a reroll (i.e. not like I did here, which was done
this way only because I needed to make sure that day's integration
result passes the test suite with minimum effort on my end ;-)).
Thanks.
fsck.c | 14 --
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/fsck.c b/fsck.c
index 8341a30..6e6ea25 100644
--- a/fsck.c
+++ b/fsck.c
@@ -248,14 +248,14 @@ static int require_end_of_header(const void *data,
unsigned long size,
switch (buffer[i]) {
case '\0':
return error_func(obj, FSCK_ERROR,
- invalid message: NUL at offset %d, i);
+ invalid header: NUL at offset %d, i);
case '\n':
if (i + 1 size buffer[i + 1] == '\n')
return 0;
}
}
- return error_func(obj, FSCK_ERROR, invalid buffer: missing empty
line);
+ return error_func(obj, FSCK_ERROR, invalid buffer: missing end of
header);
}
static int fsck_ident(const char **ident, struct object *obj, fsck_error
error_func)
@@ -426,14 +426,16 @@ static int fsck_tag_buffer(struct tag *tag, const char
*data,
error_func(tag-object, FSCK_WARN, invalid 'tag' name: %s,
buffer);
buffer = eol + 1;
- if (!skip_prefix(buffer, tagger , buffer)) {
+ if (!skip_prefix(buffer, tagger , buffer))
/* early tags do not contain 'tagger' lines; warn only */
- error_func(tag-object, FSCK_WARN, invalid format - expected
'tagger' line);
- }
- ret = fsck_ident(buffer, tag-object, error_func);
+ error_func(tag-object, FSCK_WARN,
+ invalid format - expected 'tagger' line);
+ else
+ ret = fsck_ident(buffer, tag-object, error_func);
done:
free(to_free);
+ strbuf_release(sb);
return ret;
}
--
2.1.0-451-g8daadf2
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
