* [PATCH 2/3] fsck: handle bad trees like other errors
From: David Turner @ 2016-09-27 0:11 UTC (permalink / raw)
To: git; +Cc: David Turner
In-Reply-To: <1474935093-26757-1-git-send-email-dturner@twosigma.com>
Instead of dying when fsck hits a malformed tree object, log the error
like any other and continue. Now fsck can tell the user which tree is
bad, too.
Signed-off-by: David Turner <dturner@twosigma.com>
---
fsck.c | 18 +++--
t/t1450-fsck.sh | 17 ++++-
t/t1450/bad-objects/.gitattributes | 1 +
.../307e300745b82417cc1a903f875c7d22e45ef907 | Bin 0 -> 137 bytes
.../f506a346749bb96f52d8605ffba9fb93d46b5ffd | Bin 0 -> 45 bytes
tree-walk.c | 83 ++++++++++++++++++---
tree-walk.h | 8 ++
7 files changed, 108 insertions(+), 19 deletions(-)
create mode 100644 t/t1450/bad-objects/.gitattributes
create mode 100644 t/t1450/bad-objects/307e300745b82417cc1a903f875c7d22e45ef907
create mode 100644 t/t1450/bad-objects/f506a346749bb96f52d8605ffba9fb93d46b5ffd
diff --git a/fsck.c b/fsck.c
index c9cf3de..4a3069e 100644
--- a/fsck.c
+++ b/fsck.c
@@ -347,8 +347,9 @@ static int fsck_walk_tree(struct tree *tree, void *data, struct fsck_options *op
return -1;
name = get_object_name(options, &tree->object);
- init_tree_desc(&desc, tree->buffer, tree->size);
- while (tree_entry(&desc, &entry)) {
+ if (init_tree_desc_gently(&desc, tree->buffer, tree->size))
+ return -1;
+ while (tree_entry_gently(&desc, &entry)) {
struct object *obj;
int result;
@@ -520,7 +521,7 @@ static int verify_ordered(unsigned mode1, const char *name1, unsigned mode2, con
static int fsck_tree(struct tree *item, struct fsck_options *options)
{
- int retval;
+ int retval = 0;
int has_null_sha1 = 0;
int has_full_path = 0;
int has_empty_name = 0;
@@ -535,7 +536,10 @@ static int fsck_tree(struct tree *item, struct fsck_options *options)
unsigned o_mode;
const char *o_name;
- init_tree_desc(&desc, item->buffer, item->size);
+ if (init_tree_desc_gently(&desc, item->buffer, item->size)) {
+ retval += report(options, &item->object, FSCK_MSG_BAD_TREE, "cannot be parsed as a tree");
+ return retval;
+ }
o_mode = 0;
o_name = NULL;
@@ -556,7 +560,10 @@ static int fsck_tree(struct tree *item, struct fsck_options *options)
is_hfs_dotgit(name) ||
is_ntfs_dotgit(name));
has_zero_pad |= *(char *)desc.buffer == '0';
- update_tree_entry(&desc);
+ if (update_tree_entry_gently(&desc)) {
+ retval += report(options, &item->object, FSCK_MSG_BAD_TREE, "cannot be parsed as a tree");
+ break;
+ }
switch (mode) {
/*
@@ -597,7 +604,6 @@ static int fsck_tree(struct tree *item, struct fsck_options *options)
o_name = name;
}
- retval = 0;
if (has_null_sha1)
retval += report(options, &item->object, FSCK_MSG_NULL_SHA1, "contains entries pointing to null sha1");
if (has_full_path)
diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index 8f52da2..f456963 100755
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -188,8 +188,7 @@ test_expect_success 'commit with NUL in header' '
grep "error in commit $new.*unterminated header: NUL at offset" out
'
-test_expect_success 'malformatted tree object' '
- test_when_finished "git update-ref -d refs/tags/wrong" &&
+test_expect_success 'tree object with duplicate entries' '
test_when_finished "remove_object \$T" &&
T=$(
GIT_INDEX_FILE=test-index &&
@@ -208,6 +207,20 @@ test_expect_success 'malformatted tree object' '
grep "error in tree .*contains duplicate file entries" out
'
+test_expect_success 'unparseable tree object' '
+ test_when_finished "git update-ref -d refs/heads/wrong" &&
+ test_when_finished "remove_object 307e300745b82417cc1a903f875c7d22e45ef907" &&
+ test_when_finished "remove_object f506a346749bb96f52d8605ffba9fb93d46b5ffd" &&
+ mkdir -p .git/objects/30 mkdir -p .git/objects/f5 &&
+ cp ../t1450/bad-objects/307e300745b82417cc1a903f875c7d22e45ef907 .git/objects/30/7e300745b82417cc1a903f875c7d22e45ef907 &&
+ cp ../t1450/bad-objects/f506a346749bb96f52d8605ffba9fb93d46b5ffd .git/objects/f5/06a346749bb96f52d8605ffba9fb93d46b5ffd &&
+ git update-ref refs/heads/wrong 307e300745b82417cc1a903f875c7d22e45ef907 &&
+ test_must_fail git fsck 2>out &&
+ grep "warning: empty filename in tree entry" out &&
+ grep "f506a346749bb96f52d8605ffba9fb93d46b5ffd" out &&
+ ! grep "fatal: empty filename in tree entry" out
+'
+
test_expect_success 'tag pointing to nonexistent' '
cat >invalid-tag <<-\EOF &&
object ffffffffffffffffffffffffffffffffffffffff
diff --git a/t/t1450/bad-objects/.gitattributes b/t/t1450/bad-objects/.gitattributes
new file mode 100644
index 0000000..a173f27
--- /dev/null
+++ b/t/t1450/bad-objects/.gitattributes
@@ -0,0 +1 @@
+[0-9a-f]*[0-9a-f] -diff
diff --git a/t/t1450/bad-objects/307e300745b82417cc1a903f875c7d22e45ef907 b/t/t1450/bad-objects/307e300745b82417cc1a903f875c7d22e45ef907
new file mode 100644
index 0000000000000000000000000000000000000000..6e23d625531856540364837ad76f8ce620b16102
GIT binary patch
literal 137
zcmV;40CxX)0iBLP4#FT1MO|}>xqxP{K%K-G7aqY2Fa=r?TM`QO`l9IxT>bpTd;bq<
zo?`(?@=&t(5HuRwDbp)rCKL48T@30F*ivBXoHE>+6SkHqWq8;vI(XK+_zc%2ZT1z{
r`<|zi#~Vo1WD<KV;fM-R48P6NfPd&6hj%O!a2o3h-{;~3ChI@mcIrR_
literal 0
HcmV?d00001
diff --git a/t/t1450/bad-objects/f506a346749bb96f52d8605ffba9fb93d46b5ffd b/t/t1450/bad-objects/f506a346749bb96f52d8605ffba9fb93d46b5ffd
new file mode 100644
index 0000000000000000000000000000000000000000..9111a7fc3c8578906e13c930a0fbd3cae047762e
GIT binary patch
literal 45
zcmb=Jqpj)X8)~pA!NA18z}PS_p~CF@#W%j<>n*Fxv)5_&?<#!Z>Hoon;loq@NdS%f
B6F2|>
literal 0
HcmV?d00001
diff --git a/tree-walk.c b/tree-walk.c
index ba544cf..0fb830b 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -22,33 +22,60 @@ static const char *get_mode(const char *str, unsigned int *modep)
return str;
}
-static void decode_tree_entry(struct tree_desc *desc, const char *buf, unsigned long size)
+static int decode_tree_entry(struct tree_desc *desc, const char *buf, unsigned long size, struct strbuf *err)
{
const char *path;
unsigned int mode, len;
- if (size < 23 || buf[size - 21])
- die("too-short tree object");
+ if (size < 23 || buf[size - 21]) {
+ strbuf_addstr(err, "too-short tree object");
+ return -1;
+ }
path = get_mode(buf, &mode);
- if (!path)
- die("malformed mode in tree entry for tree");
- if (!*path)
- die("empty filename in tree entry for tree");
+ if (!path) {
+ strbuf_addstr(err, "malformed mode in tree entry");
+ return -1;
+ }
+ if (!*path) {
+ strbuf_addstr(err, "empty filename in tree entry");
+ return -1;
+ }
len = strlen(path) + 1;
/* Initialize the descriptor entry */
desc->entry.path = path;
desc->entry.mode = canon_mode(mode);
desc->entry.oid = (const struct object_id *)(path + len);
+
+ return 0;
}
-void init_tree_desc(struct tree_desc *desc, const void *buffer, unsigned long size)
+static int init_tree_desc_internal(struct tree_desc *desc, const void *buffer, unsigned long size, struct strbuf *err)
{
desc->buffer = buffer;
desc->size = size;
if (size)
- decode_tree_entry(desc, buffer, size);
+ return decode_tree_entry(desc, buffer, size, err);
+ return 0;
+}
+
+void init_tree_desc(struct tree_desc *desc, const void *buffer, unsigned long size)
+{
+ struct strbuf err = STRBUF_INIT;
+ if (init_tree_desc_internal(desc, buffer, size, &err))
+ die("%s", err.buf);
+ strbuf_release(&err);
+}
+
+int init_tree_desc_gently(struct tree_desc *desc, const void *buffer, unsigned long size)
+{
+ struct strbuf err = STRBUF_INIT;
+ int result = init_tree_desc_internal(desc, buffer, size, &err);
+ if (result)
+ warning("%s", err.buf);
+ strbuf_release(&err);
+ return result;
}
void *fill_tree_descriptor(struct tree_desc *desc, const unsigned char *sha1)
@@ -75,7 +102,7 @@ static void entry_extract(struct tree_desc *t, struct name_entry *a)
*a = t->entry;
}
-void update_tree_entry(struct tree_desc *desc)
+static int update_tree_entry_internal(struct tree_desc *desc, struct strbuf *err)
{
const void *buf = desc->buffer;
const unsigned char *end = desc->entry.oid->hash + 20;
@@ -89,7 +116,30 @@ void update_tree_entry(struct tree_desc *desc)
desc->buffer = buf;
desc->size = size;
if (size)
- decode_tree_entry(desc, buf, size);
+ return decode_tree_entry(desc, buf, size, err);
+ return 0;
+}
+
+void update_tree_entry(struct tree_desc *desc)
+{
+ struct strbuf err = STRBUF_INIT;
+ if (update_tree_entry_internal(desc, &err))
+ die("%s", err.buf);
+ strbuf_release(&err);
+}
+
+int update_tree_entry_gently(struct tree_desc *desc)
+{
+ struct strbuf err = STRBUF_INIT;
+ if (update_tree_entry_internal(desc, &err)) {
+ warning("%s", err.buf);
+ strbuf_release(&err);
+ /* Stop processing this tree after error */
+ desc->size = 0;
+ return -1;
+ }
+ strbuf_release(&err);
+ return 0;
}
int tree_entry(struct tree_desc *desc, struct name_entry *entry)
@@ -102,6 +152,17 @@ int tree_entry(struct tree_desc *desc, struct name_entry *entry)
return 1;
}
+int tree_entry_gently(struct tree_desc *desc, struct name_entry *entry)
+{
+ if (!desc->size)
+ return 0;
+
+ *entry = desc->entry;
+ if (update_tree_entry_gently(desc))
+ return 0;
+ return 1;
+}
+
void setup_traverse_info(struct traverse_info *info, const char *base)
{
int pathlen = strlen(base);
diff --git a/tree-walk.h b/tree-walk.h
index 97a7d69..68bb78b 100644
--- a/tree-walk.h
+++ b/tree-walk.h
@@ -25,14 +25,22 @@ static inline int tree_entry_len(const struct name_entry *ne)
return (const char *)ne->oid - ne->path - 1;
}
+/*
+ * The _gently versions of these functions warn and return false on a
+ * corrupt tree entry rather than dying,
+ */
+
void update_tree_entry(struct tree_desc *);
+int update_tree_entry_gently(struct tree_desc *);
void init_tree_desc(struct tree_desc *desc, const void *buf, unsigned long size);
+int init_tree_desc_gently(struct tree_desc *desc, const void *buf, unsigned long size);
/*
* Helper function that does both tree_entry_extract() and update_tree_entry()
* and returns true for success
*/
int tree_entry(struct tree_desc *, struct name_entry *);
+int tree_entry_gently(struct tree_desc *, struct name_entry *);
void *fill_tree_descriptor(struct tree_desc *desc, const unsigned char *sha1);
--
2.8.0.rc4.22.g8ae061a
^ permalink raw reply related
* [PATCH 3/3] add David Turner's Two Sigma address
From: David Turner @ 2016-09-27 0:11 UTC (permalink / raw)
To: git; +Cc: David Turner
In-Reply-To: <1474935093-26757-1-git-send-email-dturner@twosigma.com>
From: David Turner <novalis@novalis.org>
Signed-off-by: David Turner <novalis@novalis.org>
---
.mailmap | 1 +
1 file changed, 1 insertion(+)
diff --git a/.mailmap b/.mailmap
index 9441a54..9cc33e9 100644
--- a/.mailmap
+++ b/.mailmap
@@ -48,6 +48,7 @@ David KÃ¥gedal <davidk@lysator.liu.se>
David Reiss <dreiss@facebook.com> <dreiss@dreiss-vmware.(none)>
David S. Miller <davem@davemloft.net>
David Turner <novalis@novalis.org> <dturner@twopensource.com>
+David Turner <novalis@novalis.org> <dturner@twosigma.com>
Deskin Miller <deskinm@umich.edu>
Dirk Süsserott <newsletter@dirk.my1.cc>
Eric Blake <eblake@redhat.com> <ebb9@byu.net>
--
2.8.0.rc4.22.g8ae061a
^ permalink raw reply related
* [PATCH 1/3] tree-walk: be more specific about corrupt tree errors
From: David Turner @ 2016-09-27 0:11 UTC (permalink / raw)
To: git; +Cc: Jeff King, David Turner
From: Jeff King <peff@peff.net>
When the tree-walker runs into an error, it just calls
die(), and the message is always "corrupt tree file".
However, we are actually covering several cases here; let's
give the user a hint about what happened.
Let's also avoid using the word "corrupt", which makes it
seem like the data bit-rotted on disk. Our sha1 check would
already have found that. These errors are ones of data that
is malformed in the first place.
Signed-off-by: David Turner <dturner@twosigma.com>
Signed-off-by: Jeff King <peff@peff.net>
---
t/t1007-hash-object.sh | 15 +++++++++++++--
t/t1007/.gitattributes | 1 +
t/t1007/tree-with-empty-filename | Bin 0 -> 28 bytes
t/t1007/tree-with-malformed-mode | Bin 0 -> 39 bytes
tree-walk.c | 12 +++++++-----
5 files changed, 21 insertions(+), 7 deletions(-)
create mode 100644 t/t1007/.gitattributes
create mode 100644 t/t1007/tree-with-empty-filename
create mode 100644 t/t1007/tree-with-malformed-mode
diff --git a/t/t1007-hash-object.sh b/t/t1007-hash-object.sh
index acca9ac..f21848b 100755
--- a/t/t1007-hash-object.sh
+++ b/t/t1007-hash-object.sh
@@ -183,9 +183,20 @@ for args in "-w --stdin-paths" "--stdin-paths -w"; do
pop_repo
done
-test_expect_success 'corrupt tree' '
+test_expect_success 'too-short tree' '
echo abc >malformed-tree &&
- test_must_fail git hash-object -t tree malformed-tree
+ test_must_fail git hash-object -t tree malformed-tree 2>err &&
+ grep "too-short tree object" err
+'
+
+test_expect_success 'malformed mode in tree' '
+ test_must_fail git hash-object -t tree ../t1007/tree-with-malformed-mode 2>err &&
+ grep "malformed mode in tree entry" err
+'
+
+test_expect_success 'empty filename in tree' '
+ test_must_fail git hash-object -t tree ../t1007/tree-with-empty-filename 2>err &&
+ grep "empty filename in tree entry" err
'
test_expect_success 'corrupt commit' '
diff --git a/t/t1007/.gitattributes b/t/t1007/.gitattributes
new file mode 100644
index 0000000..7352ef5
--- /dev/null
+++ b/t/t1007/.gitattributes
@@ -0,0 +1 @@
+tree-with-* -diff
diff --git a/t/t1007/tree-with-empty-filename b/t/t1007/tree-with-empty-filename
new file mode 100644
index 0000000000000000000000000000000000000000..aeb1ceb20e485eebd0acbb81c974d1c6fedcc1fe
GIT binary patch
literal 28
kcmXpsFfcPQQDAsB_tET47q2;ccWbUIkGgT_Nl)-Z0Hx{;SO5S3
literal 0
HcmV?d00001
diff --git a/t/t1007/tree-with-malformed-mode b/t/t1007/tree-with-malformed-mode
new file mode 100644
index 0000000000000000000000000000000000000000..24aa84d60ef8e269fb0b29c67b5208639b9da3ae
GIT binary patch
literal 39
vcmYewPcJRb%}+^HNXyJg%}dNpWq3CC(d<nZuQ_{nYpyGgx^d`9Pw+$lU*Quk
literal 0
HcmV?d00001
diff --git a/tree-walk.c b/tree-walk.c
index ce27842..ba544cf 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -27,12 +27,14 @@ static void decode_tree_entry(struct tree_desc *desc, const char *buf, unsigned
const char *path;
unsigned int mode, len;
- if (size < 24 || buf[size - 21])
- die("corrupt tree file");
+ if (size < 23 || buf[size - 21])
+ die("too-short tree object");
path = get_mode(buf, &mode);
- if (!path || !*path)
- die("corrupt tree file");
+ if (!path)
+ die("malformed mode in tree entry for tree");
+ if (!*path)
+ die("empty filename in tree entry for tree");
len = strlen(path) + 1;
/* Initialize the descriptor entry */
@@ -81,7 +83,7 @@ void update_tree_entry(struct tree_desc *desc)
unsigned long len = end - (const unsigned char *)buf;
if (size < len)
- die("corrupt tree file");
+ die("too-short tree file");
buf = end;
size -= len;
desc->buffer = buf;
--
2.8.0.rc4.22.g8ae061a
^ permalink raw reply related
* Re: [PATCH 3/3] add David Turner's Two Sigma address
From: David Turner @ 2016-09-27 0:13 UTC (permalink / raw)
To: David Turner; +Cc: git
In-Reply-To: <1474935093-26757-3-git-send-email-dturner@twosigma.com>
Sorry for the bad subject line, this is of course v2 of the series.
On Mon, 2016-09-26 at 20:11 -0400, David Turner wrote:
> From: David Turner <novalis@novalis.org>
>
> Signed-off-by: David Turner <novalis@novalis.org>
> ---
> .mailmap | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/.mailmap b/.mailmap
> index 9441a54..9cc33e9 100644
> --- a/.mailmap
> +++ b/.mailmap
> @@ -48,6 +48,7 @@ David Kågedal <davidk@lysator.liu.se>
> David Reiss <dreiss@facebook.com> <dreiss@dreiss-vmware.(none)>
> David S. Miller <davem@davemloft.net>
> David Turner <novalis@novalis.org> <dturner@twopensource.com>
> +David Turner <novalis@novalis.org> <dturner@twosigma.com>
> Deskin Miller <deskinm@umich.edu>
> Dirk Süsserott <newsletter@dirk.my1.cc>
> Eric Blake <eblake@redhat.com> <ebb9@byu.net>
^ permalink raw reply
* Re: [PATCH 0/2] tree-walk improvements
From: Junio C Hamano @ 2016-09-27 0:35 UTC (permalink / raw)
To: David Turner; +Cc: git, peff, mhagger
In-Reply-To: <1474921343.13374.1.camel@frank>
David Turner <novalis@novalis.org> writes:
> Because truncated, to me, means "something that has been cut off". Here,
> the recorded length is too short, so it's probably not the case that
> something was cut off -- it was never right to begin with.
That's perfectly sensible. Thanks.
^ permalink raw reply
* [PATCH 0/2] Locally alias "latin-1" to "ISO-8859-1"
From: Junio C Hamano @ 2016-09-27 1:22 UTC (permalink / raw)
To: git
Some systems do not seem to ship "latin-1" as a valid locale, even
though they happilly accept more modern official name "ISO-8859-1".
Naturally, "iconv -f iso-8859-1" succeeds while "iconv -f latin-1"
fails on such a system.
We already have in utf8.c to accomodate overly strict iconv_open()
that does not like various spellings of UTF-8 when our users spell
it differently from the most official "UTF-8" form. Piggyback on
the mechanism and teach outselves that "latin-1" used to be the way
to say "ISO-8859-1".
I feel dirty for doing it this way, but I found it the easiest
workaround to apply recent patches we saw on the mailing list.
Junio C Hamano (2):
utf8: refactor code to decide fallback encoding
utf8: accept "latin-1" as ISO-8859-1
utf8.c | 36 +++++++++++++++++++++++++-----------
1 file changed, 25 insertions(+), 11 deletions(-)
--
2.10.0-556-g5bbc40b
^ permalink raw reply
* [PATCH 1/2] utf8: refactor code to decide fallback encoding
From: Junio C Hamano @ 2016-09-27 1:22 UTC (permalink / raw)
To: git
In-Reply-To: <20160927012211.9378-1-gitster@pobox.com>
The codepath we use to call iconv_open() has a provision to use a
fallback encoding when it fails, hoping that "UTF-8" being spelled
differently could be the reason why the library function did not
like the encoding names we gave it. Essentially, we turn what we
have observed to be used as variants of "UTF-8" (e.g. "utf8") into
the most official spelling and use that as a fallback.
We do the same thing for input and output encoding. Introduce a
helper function to do just one side and call that twice.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
utf8.c | 29 ++++++++++++++++++-----------
1 file changed, 18 insertions(+), 11 deletions(-)
diff --git a/utf8.c b/utf8.c
index 00e10c8..550e785 100644
--- a/utf8.c
+++ b/utf8.c
@@ -489,6 +489,21 @@ char *reencode_string_iconv(const char *in, size_t insz, iconv_t conv, int *outs
return out;
}
+static const char *fallback_encoding(const char *name)
+{
+ /*
+ * Some platforms do not have the variously spelled variants of
+ * UTF-8, so let's fall back to trying the most official
+ * spelling. We do so only as a fallback in case the platform
+ * does understand the user's spelling, but not our official
+ * one.
+ */
+ if (is_encoding_utf8(name))
+ return "UTF-8";
+
+ return name;
+}
+
char *reencode_string_len(const char *in, int insz,
const char *out_encoding, const char *in_encoding,
int *outsz)
@@ -501,17 +516,9 @@ char *reencode_string_len(const char *in, int insz,
conv = iconv_open(out_encoding, in_encoding);
if (conv == (iconv_t) -1) {
- /*
- * Some platforms do not have the variously spelled variants of
- * UTF-8, so let's fall back to trying the most official
- * spelling. We do so only as a fallback in case the platform
- * does understand the user's spelling, but not our official
- * one.
- */
- if (is_encoding_utf8(in_encoding))
- in_encoding = "UTF-8";
- if (is_encoding_utf8(out_encoding))
- out_encoding = "UTF-8";
+ in_encoding = fallback_encoding(in_encoding);
+ out_encoding = fallback_encoding(out_encoding);
+
conv = iconv_open(out_encoding, in_encoding);
if (conv == (iconv_t) -1)
return NULL;
--
2.10.0-556-g5bbc40b
^ permalink raw reply related
* [PATCH 2/2] utf8: accept "latin-1" as ISO-8859-1
From: Junio C Hamano @ 2016-09-27 1:22 UTC (permalink / raw)
To: git
In-Reply-To: <20160927012211.9378-1-gitster@pobox.com>
Even though latin-1 is still seen in e-mail headers, some platforms
only install ISO-8859-1. "iconv -f ISO-8859-1" succeeds, while
"iconv -f latin-1" fails on such a system.
Using the same fallback_encoding() mechanism factored out in the
previous step, teach ourselves that "ISO-8859-1" has a better chance
of being accepted than "latin-1".
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
utf8.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/utf8.c b/utf8.c
index 550e785..0c8e011 100644
--- a/utf8.c
+++ b/utf8.c
@@ -501,6 +501,13 @@ static const char *fallback_encoding(const char *name)
if (is_encoding_utf8(name))
return "UTF-8";
+ /*
+ * Even though latin-1 is still seen in e-mail
+ * headers, some platforms only install ISO-8859-1.
+ */
+ if (!strcasecmp(name, "latin-1"))
+ return "ISO-8859-1";
+
return name;
}
--
2.10.0-556-g5bbc40b
^ permalink raw reply related
* Re: Stack read out-of-bounds in parse_sha1_header_extended using git 2.10.0
From: Gustavo Grieco @ 2016-09-27 2:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqtwd2sf9t.fsf@gitster.mtv.corp.google.com>
Btw, this other test case will trigger a similar issue, but in another line of code:
To reproduce:
$ git init ; mkdir -p .git/objects/b2 ; printf 'eJwNwoENgDAIBECkDsII5Z8CHagLGPePXu59zjHGRIOZG3OzI/lnRc4KemXDPdYSml6iQ+4ATIZ+nAEK4g==' | base64 -d > .git/objects/b2/93584ddd61af21260be75ee9f73e9d53f08cd0
Then:
$ git fsck
notice: HEAD points to an unborn branch (master)
=================================================================
==24569==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffe7645fda0 at pc 0x0000006fe799 bp 0x7ffe7645fc40 sp 0x7ffe7645fc30
READ of size 1 at 0x7ffe7645fda0 thread T0
#0 0x6fe798 in parse_sha1_header_extended /home/g/Work/Code/git-2.10.0/sha1_file.c:1714
...
It will be nice to test the current patch.
----- Original Message -----
> Junio C Hamano <gitster@pobox.com> writes:
>
> > I am inclined to say that it has no security implications. You have
> > to be able to write a bogus loose object in an object store you
> > already have write access to in the first place, in order to cause
> > this ...
>
> Note that you could social-engineer others to fetch from you and
> feed a small enough update that results in loose objects created in
> their repositories, without you having a direct write access to the
> repository.
>
> The codepath under discussion in this thread however cannot be used
> as an attack vector via that route, because the "fetch from
> elsewhere" codepath runs verification of the incoming data stream
> before storing the results (either in loose object files, or in a
> packfile) on disk.
>
>
^ permalink raw reply
* Possible integer overflow parsing malformed objects in git 2.10.0
From: Gustavo Grieco @ 2016-09-27 2:30 UTC (permalink / raw)
To: git
In-Reply-To: <1825523389.8224664.1474812766424.JavaMail.zimbra@imag.fr>
Hi,
We found a malformed object file that triggers an allocation with a negative size when parsed in git 2.10.0. It can be caused by an integer overflow somewhere, so it is better to verify how the code got such value. It was tested on ArchLinux x86_64. To reproduce, first recompile git with ASAN support and then execute:
$ git init ; mkdir -p .git/objects/b2 ; printf 'eJyVT8ERAjEIXKiEBpyBHJdcCroGHAvQjyX49m1ZtmADQjL68uMnZFnYZU/HfRfb3Gtz17Y07etqXhX6ul9uAnCJh6DCAKxUCWABok9J2PN8jYn42iwqYA2OYoKRzVAY67mYgIOfQP8WOthUKubNt6V6/yn5YSPEowsxKGPk0Jdq6ZLKxJYX2LTjYTNi52WTAN4RVyPd' | base64 -d > .git/objects/b2/93584ddd61af21260be75ee9f73e9d53f08cd0
Finally you can trigger the bug using several commands from git (other commands that parses all objects will work too), for instance:
$ git fsck
The ASAN report is here:
==24709==WARNING: AddressSanitizer failed to allocate 0xffffffffffffff65 bytes
==24709==AddressSanitizer's allocator is terminating the process instead of returning 0
==24709==If you don't like this behavior set allocator_may_return_null=1
==24709==AddressSanitizer CHECK failed: /build/gcc-multilib/src/gcc/libsanitizer/sanitizer_common/sanitizer_allocator.cc:145 "((0)) != (0)" (0x0, 0x0)
#0 0x7f571ae467aa in AsanCheckFailed /build/gcc-multilib/src/gcc/libsanitizer/asan/asan_rtl.cc:65
#1 0x7f571ae4d163 in __sanitizer::CheckFailed(char const*, int, char const*, unsigned long long, unsigned long long) /build/gcc-multilib/src/gcc/libsanitizer/sanitizer_common/sanitizer_common.cc:157
#2 0x7f571ae4b326 in __sanitizer::ReportAllocatorCannotReturnNull() /build/gcc-multilib/src/gcc/libsanitizer/sanitizer_common/sanitizer_allocator.cc:145
#3 0x7f571ad9b2f4 in __sanitizer::CombinedAllocator<__sanitizer::SizeClassAllocator64<105553116266496ul, 4398046511104ul, 0ul, __sanitizer::SizeClassMap<17ul, 128ul, 16ul>, __asan::AsanMapUnmapCallback>, __sanitizer::SizeClassAllocatorLocalCache<__sanitizer::SizeClassAllocator64<105553116266496ul, 4398046511104ul, 0ul, __sanitizer::SizeClassMap<17ul, 128ul, 16ul>, __asan::AsanMapUnmapCallback> >, __sanitizer::LargeMmapAllocator<__asan::AsanMapUnmapCallback> >::ReturnNullOrDie() /build/gcc-multilib/src/gcc/libsanitizer/sanitizer_common/sanitizer_allocator.h:1315
#4 0x7f571ad9b2f4 in __asan::Allocator::Allocate(unsigned long, unsigned long, __sanitizer::BufferedStackTrace*, __asan::AllocType, bool) /build/gcc-multilib/src/gcc/libsanitizer/asan/asan_allocator.cc:357
#5 0x7f571ad9b2f4 in __asan::asan_malloc(unsigned long, __sanitizer::BufferedStackTrace*) /build/gcc-multilib/src/gcc/libsanitizer/asan/asan_allocator.cc:716
#6 0x7f571ae3ce24 in __interceptor_malloc /build/gcc-multilib/src/gcc/libsanitizer/asan/asan_malloc_linux.cc:63
#7 0x767816 in do_xmalloc /home/g/Work/Code/git-2.10.0/wrapper.c:59
#8 0x76794c in do_xmallocz /home/g/Work/Code/git-2.10.0/wrapper.c:99
#9 0x7679bd in xmallocz /home/g/Work/Code/git-2.10.0/wrapper.c:107
#10 0x6fe36c in unpack_sha1_rest /home/g/Work/Code/git-2.10.0/sha1_file.c:1625
#11 0x6feb40 in unpack_sha1_file /home/g/Work/Code/git-2.10.0/sha1_file.c:1751
#12 0x703fe0 in read_object /home/g/Work/Code/git-2.10.0/sha1_file.c:2811
#13 0x70410a in read_sha1_file_extended /home/g/Work/Code/git-2.10.0/sha1_file.c:2834
#14 0x647676 in read_sha1_file /home/g/Work/Code/git-2.10.0/cache.h:1056
#15 0x648545 in parse_object /home/g/Work/Code/git-2.10.0/object.c:269
#16 0x48d46d in fsck_sha1 builtin/fsck.c:367
#17 0x48da47 in fsck_loose builtin/fsck.c:493
#18 0x707514 in for_each_file_in_obj_subdir /home/g/Work/Code/git-2.10.0/sha1_file.c:3477
#19 0x70775b in for_each_loose_file_in_objdir_buf /home/g/Work/Code/git-2.10.0/sha1_file.c:3512
#20 0x707885 in for_each_loose_file_in_objdir /home/g/Work/Code/git-2.10.0/sha1_file.c:3532
#21 0x48dc1d in fsck_object_dir builtin/fsck.c:521
#22 0x48e2e6 in cmd_fsck builtin/fsck.c:644
#23 0x407a8f in run_builtin /home/g/Work/Code/git-2.10.0/git.c:352
#24 0x407e35 in handle_builtin /home/g/Work/Code/git-2.10.0/git.c:539
#25 0x408175 in run_argv /home/g/Work/Code/git-2.10.0/git.c:593
#26 0x408458 in cmd_main /home/g/Work/Code/git-2.10.0/git.c:665
#27 0x53fc70 in main /home/g/Work/Code/git-2.10.0/common-main.c:40
#28 0x7f5719f46290 in __libc_start_main (/usr/lib/libc.so.6+0x20290)
#29 0x405209 in _start (/home/g/Work/Code/git-2.10.0/git+0x405209)
This test case was found using QuickFuzz.
Regards,
Gustavo.
^ permalink raw reply
* RE: git-upload-pack hangs
From: Jason Pyeron @ 2016-09-27 3:45 UTC (permalink / raw)
To: git
In-Reply-To: <62E3FC352BE4428A90D7E4E9B137A9FB@black7>
This is a very, very first draft.
It is allowing IIS to work right now.
I still need to address chunked issues, where there is no content length (see http://www.gossamer-threads.com/lists/apache/users/373042)
Any comments, sugestions?
-Jason
--- ./origsrc/git-v2.8.3/http-backend.c 2016-05-18 18:32:41.000000000 -0400
+++ ./src/git-v2.8.3/http-backend.c 2016-09-26 22:52:02.636135000 -0400
@@ -279,14 +279,17 @@
{
size_t len = 0, alloc = 8192;
unsigned char *buf = xmalloc(alloc);
+ /* get request size */
+ size_t req_len = git_env_ulong("CONTENT_LENGTH", -1);
if (max_request_buffer < alloc)
max_request_buffer = alloc;
- while (1) {
+ while (req_len>0 || req_len==-1 ) {
+ ssize_t maxread=alloc>req_len && req_len!=-1?req_len:alloc;
ssize_t cnt;
- cnt = read_in_full(fd, buf + len, alloc - len);
+ cnt = read_in_full(fd, buf + len, maxread - len);
if (cnt < 0) {
free(buf);
return -1;
@@ -294,13 +297,19 @@
/* partial read from read_in_full means we hit EOF */
len += cnt;
- if (len < alloc) {
+ if (len < maxread) {
*out = buf;
return len;
}
+ if (req_len>0) {
+ req_len -= cnt;
+ if (req_len<0)
+ req_len=0;
+ }
+
/* otherwise, grow and try again (if we can) */
- if (alloc == max_request_buffer)
+ if (alloc == max_request_buffer && maxread == alloc)
die("request was larger than our maximum size (%lu);"
" try setting GIT_HTTP_MAX_REQUEST_BUFFER",
max_request_buffer);
@@ -310,6 +319,10 @@
alloc = max_request_buffer;
REALLOC_ARRAY(buf, alloc);
}
+
+ free(buf);
+
+ return len;
}
static void inflate_request(const char *prog_name, int out, int buffer_input)
> -----Original Message-----
> From: git-owner@vger.kernel.org
> [mailto:git-owner@vger.kernel.org] On Behalf Of Jason Pyeron
> Sent: Monday, September 26, 2016 09:26
> To: git@vger.kernel.org
> Subject: RE: git-upload-pack hangs
>
> > -----Original Message-----
> > From: Jason Pyeron
> > Sent: Monday, September 26, 2016 01:51
> >
> > git is hanging on clone. I am runnig (cygwin) git 2.8.3 on
> > IIS7 (windows server 2012 R2).
> >
> > Where can I start to perform additional debugging?
> >
>
> Reading this thread, it seems plausible as a cause since it
> aligns with my testing.
>
> http://www.spinics.net/lists/git/msg279437.html [ and
> http://www.spinics.net/lists/git/attachments/binQFGHirNLw3.bin ]
>
> I will start to trudge into the code to see if this (or
> similar) has been applied and if not, does it fix it.
>
> > Selected items I have read, but they did not help:
> >
> > http://unix.stackexchange.com/questions/98959/git-upload-pack-
> > hangs-indefinitely
> >
> > https://sparethought.wordpress.com/2012/12/06/setting-git-to-w
> ork-behind-ntlm-authenticated-proxy-cntlm-to-the-rescue/
> >
> > https://sourceforge.net/p/cntlm/bugs/24/
> >
> > invocation of the clone:
> >
> > jpyeron.adm@SERVER /tmp
> > $ GIT_TRACE=1 GIT_CURL_VERBOSE=true git clone
> > http://SERVER.domain.com/git/test.git
> > 01:23:37.020476 git.c:350 trace: built-in: git
> > 'clone' 'http://SERVER.domain.com/git/test.git'
> > Cloning into 'test'...
> > 01:23:37.206046 run-command.c:336 trace: run_command:
> > 'git-remote-http' 'origin' 'http://SERVER.domain.com/git/test.git'
> > * STATE: INIT => CONNECT handle 0x60009a140; line 1397
> > (connection #-5000)
> > * Couldn't find host SERVER.domain.com in the .netrc file;
> > using defaults
> > * Added connection 0. The cache now contains 1 members
> > * Trying ::1...
> > * TCP_NODELAY set
> > * STATE: CONNECT => WAITCONNECT handle 0x60009a140; line 1450
> > (connection #0)
> > * Connected to SERVER.domain.com (::1) port 80 (#0)
> > * STATE: WAITCONNECT => SENDPROTOCONNECT handle 0x60009a140;
> > line 1557 (connection #0)
> > * Marked for [keep alive]: HTTP default
> > * STATE: SENDPROTOCONNECT => DO handle 0x60009a140; line 1575
> > (connection #0)
> > > GET /git/test.git/info/refs?service=git-upload-pack HTTP/1.1
> > Host: SERVER.domain.com
> > User-Agent: git/2.8.3
> > Accept: */*
> > Accept-Encoding: gzip
> > Accept-Language: en-US, *;q=0.9
> > Pragma: no-cache
> >
> > * STATE: DO => DO_DONE handle 0x60009a140; line 1654 (connection #0)
> > * STATE: DO_DONE => WAITPERFORM handle 0x60009a140; line 1781
> > (connection #0)
> > * STATE: WAITPERFORM => PERFORM handle 0x60009a140; line 1791
> > (connection #0)
> > * HTTP 1.1 or later with persistent connection, pipelining supported
> > < HTTP/1.1 200 OK
> > < Cache-Control: no-cache, max-age=0, must-revalidate
> > < Pragma: no-cache
> > < Content-Type: application/x-git-upload-pack-advertisement
> > < Expires: Fri, 01 Jan 1980 00:00:00 GMT
> > * Server Microsoft-IIS/8.5 is not blacklisted
> > < Server: Microsoft-IIS/8.5
> > < X-Powered-By: ASP.NET
> > < Date: Mon, 26 Sep 2016 05:23:37 GMT
> > * Marked for [closure]: Connection: close used
> > < Connection: close
> > < Content-Length: 310
> > <
> > * STATE: PERFORM => DONE handle 0x60009a140; line 1955
> (connection #0)
> > * multi_done
> > * Curl_http_done: called premature == 0
> > * Closing connection 0
> > * The cache now contains 0 members
> > 01:23:37.688252 run-command.c:336 trace: run_command:
> > 'fetch-pack' '--stateless-rpc' '--stdin' '--lock-pack'
> > '--thin' '--check-self-contained-and-connected' '--cloning'
> > 'http://SERVER.domain.com/git/test.git/'
> > 01:23:37.717168 exec_cmd.c:120 trace: exec: 'git'
> > 'fetch-pack' '--stateless-rpc' '--stdin' '--lock-pack'
> > '--thin' '--check-self-contained-and-connected' '--cloning'
> > 'http://SERVER.domain.com/git/test.git/'
> > 01:23:37.749820 git.c:350 trace: built-in: git
> > 'fetch-pack' '--stateless-rpc' '--stdin' '--lock-pack'
> > '--thin' '--check-self-contained-and-connected' '--cloning'
> > 'http://SERVER.domain.com/git/test.git/'
> > * STATE: INIT => CONNECT handle 0x60009a140; line 1397
> > (connection #-5000)
> > * Couldn't find host SERVER.domain.com in the .netrc file;
> > using defaults
> > * Added connection 1. The cache now contains 1 members
> > * Hostname SERVER.domain.com was found in DNS cache
> > * Trying ::1...
> > * TCP_NODELAY set
> > * STATE: CONNECT => WAITCONNECT handle 0x60009a140; line 1450
> > (connection #1)
> > * Connected to SERVER.domain.com (::1) port 80 (#1)
> > * STATE: WAITCONNECT => SENDPROTOCONNECT handle 0x60009a140;
> > line 1557 (connection #1)
> > * Marked for [keep alive]: HTTP default
> > * STATE: SENDPROTOCONNECT => DO handle 0x60009a140; line 1575
> > (connection #1)
> > > POST /git/test.git/git-upload-pack HTTP/1.1
> > Host: SERVER.domain.com
> > User-Agent: git/2.8.3
> > Accept-Encoding: gzip
> > Content-Type: application/x-git-upload-pack-request
> > Accept: application/x-git-upload-pack-result
> > Content-Length: 140
> >
> > * upload completely sent off: 140 out of 140 bytes
> > * STATE: DO => DO_DONE handle 0x60009a140; line 1654 (connection #1)
> > * STATE: DO_DONE => WAITPERFORM handle 0x60009a140; line 1781
> > (connection #1)
> > * STATE: WAITPERFORM => PERFORM handle 0x60009a140; line 1791
> > (connection #1)
>
> --
> -=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
> - -
> - Jason Pyeron PD Inc. http://www.pdinc.us -
> - Principal Consultant 10 West 24th Street #100 -
> - +1 (443) 269-1555 x333 Baltimore, Maryland 21218 -
> - -
> -=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
>
>
>
^ permalink raw reply
* Re: [PATCH 1/3] tree-walk: be more specific about corrupt tree errors
From: Junio C Hamano @ 2016-09-27 4:01 UTC (permalink / raw)
To: David Turner; +Cc: git, Jeff King
In-Reply-To: <1474935093-26757-1-git-send-email-dturner@twosigma.com>
David Turner <dturner@twosigma.com> writes:
> From: Jeff King <peff@peff.net>
>
> When the tree-walker runs into an error, it just calls
> die(), and the message is always "corrupt tree file".
> However, we are actually covering several cases here; let's
> give the user a hint about what happened.
>
> Let's also avoid using the word "corrupt", which makes it
> seem like the data bit-rotted on disk. Our sha1 check would
> already have found that. These errors are ones of data that
> is malformed in the first place.
>
> Signed-off-by: David Turner <dturner@twosigma.com>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> t/t1007-hash-object.sh | 15 +++++++++++++--
> t/t1007/.gitattributes | 1 +
> t/t1007/tree-with-empty-filename | Bin 0 -> 28 bytes
> t/t1007/tree-with-malformed-mode | Bin 0 -> 39 bytes
> tree-walk.c | 12 +++++++-----
> 5 files changed, 21 insertions(+), 7 deletions(-)
> create mode 100644 t/t1007/.gitattributes
> create mode 100644 t/t1007/tree-with-empty-filename
> create mode 100644 t/t1007/tree-with-malformed-mode
I hate to report this, but this alone, or together with 2/2, when
merged to 'pu', I cannot get them to pass the tests in my automated
integration tests, even though they seem to pass when the problematic
tests are run manually. I do not see offhand anything suspicious
(like something that may be racy) in these two patches but I haven't
figured out where it goes wrong.
If somebody manages to find breakages in today's 'pu', please (1) do
not be too alarmed, and (2) help figure out where things are broken.
Thanks.
^ permalink raw reply
* [PATCH] xdiff: rename "struct group" to "struct xdlgroup"
From: Jeff King @ 2016-09-27 4:37 UTC (permalink / raw)
To: Michael Haggerty; +Cc: git
Commit e8adf23 (xdl_change_compact(): introduce the concept
of a change group, 2016-08-22) added a "struct group" type
to xdiff/xdiffi.c. But the POSIX system header "grp.h"
already defines "struct group" (it is part of the getgrnam
interface). This happens to work because the new type is
local to xdiffi.c, and the xdiff code includes a relatively
small set of system headers. But it will break compilation
if xdiff ever switches to using git-compat-util.h. It can
also probably cause confusion with tools that look at the
whole code base, like coccinelle or ctags.
Let's resolve by giving the xdiff variant a scoped name,
which is closer to other xdiff types anyway (e.g.,
xdlfile_t, though note that xdiff is fond if typedefs when
Git usually is not).
Signed-off-by: Jeff King <peff@peff.net>
---
I didn't rename the functions, which have no conflict, but that would
also be closer to xdiff's usual style. I don't know how far it is worth
going; maybe this patch is even already too far.
I noticed because I have a patch series which switches xdiff
to git-compat-util, to try to use the st_* macros there.
xdiff/xdiffi.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c
index 67c1ccc..760fbb6 100644
--- a/xdiff/xdiffi.c
+++ b/xdiff/xdiffi.c
@@ -708,7 +708,7 @@ static int score_cmp(struct split_score *s1, struct split_score *s2)
* Note that loops that are testing for changed lines in xdf->rchg do not need
* index bounding since the array is prepared with a zero at position -1 and N.
*/
-struct group {
+struct xdlgroup {
/*
* The index of the first changed line in the group, or the index of
* the unchanged line above which the (empty) group is located.
@@ -725,7 +725,7 @@ struct group {
/*
* Initialize g to point at the first group in xdf.
*/
-static void group_init(xdfile_t *xdf, struct group *g)
+static void group_init(xdfile_t *xdf, struct xdlgroup *g)
{
g->start = g->end = 0;
while (xdf->rchg[g->end])
@@ -736,7 +736,7 @@ static void group_init(xdfile_t *xdf, struct group *g)
* Move g to describe the next (possibly empty) group in xdf and return 0. If g
* is already at the end of the file, do nothing and return -1.
*/
-static inline int group_next(xdfile_t *xdf, struct group *g)
+static inline int group_next(xdfile_t *xdf, struct xdlgroup *g)
{
if (g->end == xdf->nrec)
return -1;
@@ -752,7 +752,7 @@ static inline int group_next(xdfile_t *xdf, struct group *g)
* Move g to describe the previous (possibly empty) group in xdf and return 0.
* If g is already at the beginning of the file, do nothing and return -1.
*/
-static inline int group_previous(xdfile_t *xdf, struct group *g)
+static inline int group_previous(xdfile_t *xdf, struct xdlgroup *g)
{
if (g->start == 0)
return -1;
@@ -769,7 +769,7 @@ static inline int group_previous(xdfile_t *xdf, struct group *g)
* following group, expand this group to include it. Return 0 on success or -1
* if g cannot be slid down.
*/
-static int group_slide_down(xdfile_t *xdf, struct group *g, long flags)
+static int group_slide_down(xdfile_t *xdf, struct xdlgroup *g, long flags)
{
if (g->end < xdf->nrec &&
recs_match(xdf->recs[g->start], xdf->recs[g->end], flags)) {
@@ -790,7 +790,7 @@ static int group_slide_down(xdfile_t *xdf, struct group *g, long flags)
* into a previous group, expand this group to include it. Return 0 on success
* or -1 if g cannot be slid up.
*/
-static int group_slide_up(xdfile_t *xdf, struct group *g, long flags)
+static int group_slide_up(xdfile_t *xdf, struct xdlgroup *g, long flags)
{
if (g->start > 0 &&
recs_match(xdf->recs[g->start - 1], xdf->recs[g->end - 1], flags)) {
@@ -818,7 +818,7 @@ static void xdl_bug(const char *msg)
* size.
*/
int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {
- struct group g, go;
+ struct xdlgroup g, go;
long earliest_end, end_matching_other;
long groupsize;
unsigned int blank_lines;
--
2.10.0.492.g14f803f
^ permalink raw reply related
* Re: [PATCH 1/3] tree-walk: be more specific about corrupt tree errors
From: Jeff King @ 2016-09-27 4:53 UTC (permalink / raw)
To: Junio C Hamano; +Cc: David Turner, git
In-Reply-To: <xmqqtwd2ng8k.fsf@gitster.mtv.corp.google.com>
On Mon, Sep 26, 2016 at 09:01:15PM -0700, Junio C Hamano wrote:
> > 5 files changed, 21 insertions(+), 7 deletions(-)
> > create mode 100644 t/t1007/.gitattributes
> > create mode 100644 t/t1007/tree-with-empty-filename
> > create mode 100644 t/t1007/tree-with-malformed-mode
>
> I hate to report this, but this alone, or together with 2/2, when
> merged to 'pu', I cannot get them to pass the tests in my automated
> integration tests, even though they seem to pass when the problematic
> tests are run manually. I do not see offhand anything suspicious
> (like something that may be racy) in these two patches but I haven't
> figured out where it goes wrong.
>
> If somebody manages to find breakages in today's 'pu', please (1) do
> not be too alarmed, and (2) help figure out where things are broken.
I think the problem is just that they refer to t/t1450 (and t1007) from
the trash directory as "../t1450". That breaks when the test is run with
"--root" (and I imagine that like me, you have --root as part of your
automated tests but do not bother with it when doing a one-off run).
-Peff
^ permalink raw reply
* Re: [PATCH 1/2] tree-walk: be more specific about corrupt tree errors
From: Jeff King @ 2016-09-27 5:14 UTC (permalink / raw)
To: David Turner; +Cc: git, mhagger, David Turner
In-Reply-To: <1474918365-10937-2-git-send-email-novalis@novalis.org>
On Mon, Sep 26, 2016 at 03:32:44PM -0400, David Turner wrote:
> From: Jeff King <peff@peff.net>
>
> When the tree-walker runs into an error, it just calls
> die(), and the message is always "corrupt tree file".
> However, we are actually covering several cases here; let's
> give the user a hint about what happened.
>
> Let's also avoid using the word "corrupt", which makes it
> seem like the data bit-rotted on disk. Our sha1 check would
> already have found that. These errors are ones of data that
> is malformed in the first place.
>
> Signed-off-by: David Turner <dturner@twosigma.com>
> Signed-off-by: Jeff King <peff@peff.net>
Yay. This has been on my "to look at and repost" list for literally 2
years. Thanks for picking it up (see kids, procrastination _does_ pay
off).
> t/t1007-hash-object.sh | 15 +++++++++++++--
> t/t1007/tree-with-empty-filename | Bin 0 -> 28 bytes
> t/t1007/tree-with-malformed-mode | Bin 0 -> 39 bytes
Ooh, and tests. Exciting.
> -test_expect_success 'corrupt tree' '
> +test_expect_success 'too-short tree' '
> echo abc >malformed-tree &&
> - test_must_fail git hash-object -t tree malformed-tree
> + test_must_fail git hash-object -t tree malformed-tree 2>err &&
> + grep "too-short tree object" err
> +'
Should this be test_i18ngrep? Even if the message is not translated now,
it seems like a good proactive measure (and probably it _should_ be
translated).
> +test_expect_success 'malformed mode in tree' '
> + test_must_fail git hash-object -t tree ../t1007/tree-with-malformed-mode 2>err &&
> + grep "malformed mode in tree entry for tree" err
> +'
This ".." will break when the test is run with "--root". You should use
"$TEST_DIRECTORY"/t1007/...
instead. And ditto in the second test, of course.
> diff --git a/t/t1007/tree-with-empty-filename b/t/t1007/tree-with-empty-filename
> new file mode 100644
> index 0000000000000000000000000000000000000000..aeb1ceb20e485eebd0acbb81c974d1c6fedcc1fe
> GIT binary patch
> literal 28
> kcmXpsFfcPQQDAsB_tET47q2;ccWbUIkGgT_Nl)-Z0Hx{;SO5S3
>
> literal 0
> HcmV?d00001
This is rather opaque, of course. :)
I wonder if it would be possible to generate the test vector with
something like:
# any 20 bytes will do
bin_sha1='\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0'
printf "100644 \0$bin_sha1" >tree-with-empty-filename
I know that is longer and possibly more error-prone to run, but I think
it makes the test much easier to read and modify later.
I also wonder if $bin_sha1 should actually be more like:
hex_sha1=$(echo foo | git hash-object --stdin -w)
bin_sha1=$(echo $hex_sha1 | perl -ne 'printf "\\%3o", ord for /./g')
so that it's a real sha1 (or maybe it is in your original, from an
object that happens to be in the repo; it's hard to tell). I wouldn't
expect the code to actually get to the point of looking at the sha1, but
it's perhaps a more realistic test.
I also think it would be nice if hash-object had a "--binary-sha1"
option to avoid the perl grossness. :)
> diff --git a/tree-walk.c b/tree-walk.c
> index ce27842..ba544cf 100644
The code change itself looks brilliant, naturally. :)
-Peff
^ permalink raw reply
* Re: [PATCH 2/2] fsck: handle bad trees like other errors
From: Jeff King @ 2016-09-27 5:27 UTC (permalink / raw)
To: David Turner; +Cc: git, mhagger, David Turner
In-Reply-To: <1474918365-10937-3-git-send-email-novalis@novalis.org>
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=utf-8, Size: 4422 bytes --]
On Mon, Sep 26, 2016 at 03:32:45PM -0400, David Turner wrote:
> Instead of dying when fsck hits a malformed tree object, log the error
> like any other and continue. Now fsck can tell the user which tree is
> bad, too.
Cool. I think the lack of this is what made me drag my feet on the first
patch. Thanks for finishing it off.
> diff --git a/fsck.c b/fsck.c
> index c9cf3de..4a3069e 100644
> --- a/fsck.c
> +++ b/fsck.c
> @@ -347,8 +347,9 @@ static int fsck_walk_tree(struct tree *tree, void *data, struct fsck_options *op
> return -1;
>
> name = get_object_name(options, &tree->object);
> - init_tree_desc(&desc, tree->buffer, tree->size);
> - while (tree_entry(&desc, &entry)) {
> + if (init_tree_desc_gently(&desc, tree->buffer, tree->size))
> + return -1;
> + while (tree_entry_gently(&desc, &entry)) {
I wondered if other callers would be happy with init_tree_desc_gently().
Grepping for init_tree_desc(), it seems like it would be a fairly
trivial conversion for most of them, because they almost invariably run
unpack_trees() right afterwards, and so have to deal with errors from
it.
So perhaps in the long run we can convert them all. But certainly that
does not need to be part of this series.
> +test_expect_success 'unparseable tree object' '
> + test_when_finished "git update-ref -d refs/heads/wrong" &&
> + test_when_finished "remove_object 307e300745b82417cc1a903f875c7d22e45ef907" &&
> + test_when_finished "remove_object f506a346749bb96f52d8605ffba9fb93d46b5ffd" &&
> + mkdir -p .git/objects/30 mkdir -p .git/objects/f5 &&
> + cp ../t1450/bad-objects/307e300745b82417cc1a903f875c7d22e45ef907 .git/objects/30/7e300745b82417cc1a903f875c7d22e45ef907 &&
> + cp ../t1450/bad-objects/f506a346749bb96f52d8605ffba9fb93d46b5ffd .git/objects/f5/06a346749bb96f52d8605ffba9fb93d46b5ffd &&
This needs the same $TEST_DIRECTORY treatment as t1007.
> + git update-ref refs/heads/wrong 307e300745b82417cc1a903f875c7d22e45ef907 &&
> + test_must_fail git fsck 2>out &&
> + grep "warning: empty filename in tree entry" out &&
> + grep "f506a346749bb96f52d8605ffba9fb93d46b5ffd" out &&
> + ! grep "fatal: empty filename in tree entry" out
> +'
I'd also expect these to be test_i18ngrep, but I see that t1450 is quite
bad about this in general. I'm OK with adding them as greps and leaving
a conversion of the whole script until later.
> diff --git a/t/t1450/bad-objects/307e300745b82417cc1a903f875c7d22e45ef907 b/t/t1450/bad-objects/307e300745b82417cc1a903f875c7d22e45ef907
> new file mode 100644
> index 0000000..6e23d62
> --- /dev/null
> +++ b/t/t1450/bad-objects/307e300745b82417cc1a903f875c7d22e45ef907
> @@ -0,0 +1,4 @@
> +x\x01ŽA\x0e \x10E]sŠ¹€f°@Ä\x18\x17\x1eÁ\v0\x05Z\x12[\x12
> +õú¢é \ýüÅ{ÿ\x0fižc\x01IòP²÷\x104\x1aÛ)Ó+b&\x13ôÙ]\fê\x10ØR`êœ2Üš\x13¶–)exØ-:xÖ¼ø\f×%mö\x15×ûž§”Ç^[HÕd\x12{-áˆ
> +Q\f¿ÍÒ€\x7fè\x1d‡w,\x13p\x1aë
> +ßçâ\x03&ë?Þ
> \ No newline at end of file
Yikes. :)
I wonder if some printfs, similar to what I showed in the last patch,
combined with "hash-object --literally", could make these tests more
readable and avoid the binary goo.
> -static void decode_tree_entry(struct tree_desc *desc, const char *buf, unsigned long size)
> +static int decode_tree_entry(struct tree_desc *desc, const char *buf, unsigned long size, struct strbuf *err)
> {
I know we used the "err" strbuf pattern in the ref code, and it makes
sense there where we have a lot of different functions with public
interfaces. But here, we literally just feed the result to die() or
warning(). I wonder if a nicer interface would be:
typedef void (*err_fn)(const char *, ...);
static int decode_tree_entry(struct tree_desc *desc,
const char *buf, unsigned long size,
err_fn err)
{
...
if (size < 23 || buf[size - 21]) {
err("too-short tree object");
return -1;
}
}
I dunno. Maybe that is overengineering. I guess we only hit the strbufs
in the error path (which used to die!), so nobody really cares that much
about the extra allocation.
> +int init_tree_desc_gently(struct tree_desc *desc, const void *buffer, unsigned long size)
> +{
> + struct strbuf err = STRBUF_INIT;
> + int result = init_tree_desc_internal(desc, buffer, size, &err);
> + if (result)
> + warning("%s", err.buf);
> + strbuf_release(&err);
> + return result;
> }
I also wonder if this ought to be "error()" and not "warning()". I think
it's pretty common for fsck to spit out errors from sub-code but keep going.
-Peff
^ permalink raw reply
* Re: [PATCH 1/2] tree-walk: be more specific about corrupt tree errors
From: Junio C Hamano @ 2016-09-27 5:35 UTC (permalink / raw)
To: Jeff King; +Cc: David Turner, git, mhagger, David Turner
In-Reply-To: <20160927051453.yuvrnao5ldjpzhcj@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
>> +test_expect_success 'malformed mode in tree' '
>> + test_must_fail git hash-object -t tree ../t1007/tree-with-malformed-mode 2>err &&
>> + grep "malformed mode in tree entry for tree" err
>> +'
>
> This ".." will break when the test is run with "--root". You should use
>
> "$TEST_DIRECTORY"/t1007/...
>
> instead. And ditto in the second test, of course.
Ahh, that explains the breakage I saw.
Thanks.
^ permalink raw reply
* Re: [PATCH 10/10] get_short_sha1: list ambiguous objects on error
From: Jacob Keller @ 2016-09-27 5:42 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Jeff King, Junio C Hamano, Git Mailing List
In-Reply-To: <CA+55aFyfvvqq1c=hZcuL-yPavp2tjzx8r3bFJnMY7DAE7YcB=Q@mail.gmail.com>
On Mon, Sep 26, 2016 at 9:36 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> This looks very good to me, but I wonder if it couldn't be even more aggressive.
>
> In particular, the only hashes that most people ever use in short form
> are commit hashes. Those are the ones you'd use in normal human
> interactions to point to something happening.
>
> So when the disambiguation notices that there is ambiguity, but there
> is only _one_ commit, maybe it should just have an aggressive mode
> that says "use that as if it wasn't ambiguous".
>
> And then have an explicit command (or flag) to do disambiguation for
> when you explicitly want it.
>
> Rationale: you'd never care about short forms for tags. You'd just use
> the tag name. And while blob ID's certainly show up in short form in
> diff output (in the "index" line), very few people will use them. And
> tree hashes are basically never seen outside of any plumbing commands
> and then seldom in shortened form.
>
> So I think it would make sense to default to a mode that just picks
> the commit hash if there is only one such hash. Sure, some command
> might want a "treeish", but a commit is still more likely than a tree
> or a tag.
>
I'd think we would want to phase this in over a few releases if we do
this? Maybe at least sort commits first in the list so that they are
faster to spot.
I am trying to think of what problems we'd cause by having the
behavior be this aggressive...
Thanks,
Jake
> But regardless, this series looks like a good thing.
>
> Linus
^ permalink raw reply
* Re: [PATCH 1/2] utf8: refactor code to decide fallback encoding
From: Jeff King @ 2016-09-27 5:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <20160927012211.9378-2-gitster@pobox.com>
On Mon, Sep 26, 2016 at 06:22:10PM -0700, Junio C Hamano wrote:
> @@ -501,17 +516,9 @@ char *reencode_string_len(const char *in, int insz,
>
> conv = iconv_open(out_encoding, in_encoding);
> if (conv == (iconv_t) -1) {
> - /*
> - * Some platforms do not have the variously spelled variants of
> - * UTF-8, so let's fall back to trying the most official
> - * spelling. We do so only as a fallback in case the platform
> - * does understand the user's spelling, but not our official
> - * one.
> - */
> - if (is_encoding_utf8(in_encoding))
> - in_encoding = "UTF-8";
> - if (is_encoding_utf8(out_encoding))
> - out_encoding = "UTF-8";
> + in_encoding = fallback_encoding(in_encoding);
> + out_encoding = fallback_encoding(out_encoding);
> +
This comment is interesting. We're concerned about a platform knowing
"utf8" but not "UTF-8". When we fallback, we do it for both the input
and output encodings, because we don't know which may have caused the
problem. So is it possible that we improve one case but break the other?
With just UTF-8, I don't think so. That could only be the case with
something like "utf8 -> utf-8" because they both become "UTF-8". So
either it improves the situation or not (because we either understand
UTF-8 or not).
But once we introduce other fallbacks, then "utf8 -> latin1" may become
"UTF-8 -> iso8859-1". A system that knows only "utf8" and "iso8859-1"
_could_ work if we turned the knobs individually, but won't if we turn
them both at once. Worse, a system that knows only "UTF-8" and "latin1"
works now, but would break with your patches.
I'm not convinced it's worth worrying about, though. The existence of
such a system is theoretical at this point. I'm not even sure how common
the "know about utf8 but not UTF-8" thing is, or if we were merely being
overly cautious.
-Peff
^ permalink raw reply
* Re: [PATCH 2/2] utf8: accept "latin-1" as ISO-8859-1
From: Jeff King @ 2016-09-27 5:57 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <20160927012211.9378-3-gitster@pobox.com>
On Mon, Sep 26, 2016 at 06:22:11PM -0700, Junio C Hamano wrote:
> Even though latin-1 is still seen in e-mail headers, some platforms
> only install ISO-8859-1. "iconv -f ISO-8859-1" succeeds, while
> "iconv -f latin-1" fails on such a system.
>
> Using the same fallback_encoding() mechanism factored out in the
> previous step, teach ourselves that "ISO-8859-1" has a better chance
> of being accepted than "latin-1".
I was curious if this was the most official or accepted spelling.
Grepping a few hundred thousand messages from my mail archives, it does
seem to be the most common.
> diff --git a/utf8.c b/utf8.c
> index 550e785..0c8e011 100644
> --- a/utf8.c
> +++ b/utf8.c
> @@ -501,6 +501,13 @@ static const char *fallback_encoding(const char *name)
> if (is_encoding_utf8(name))
> return "UTF-8";
>
> + /*
> + * Even though latin-1 is still seen in e-mail
> + * headers, some platforms only install ISO-8859-1.
> + */
> + if (!strcasecmp(name, "latin-1"))
> + return "ISO-8859-1";
> +
For the UTF-8 fallbacks, we actually detect their equivalence via
same_encoding() before even hitting iconv. Is it worth doing the same
here?
I have to admit that I don't care too deeply about performance for
somebody who wants to convert "latin1" to "ISO-8859-1". If one of your
encodings is not UTF-8, you are probably Doing It Wrong. :)
-Peff
^ permalink raw reply
* Re: [PATCH 2/2] utf8: accept "latin-1" as ISO-8859-1
From: Junio C Hamano @ 2016-09-27 6:08 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20160927055744.el2jbxzdqfhjl6qt@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I have to admit that I don't care too deeply about performance for
> somebody who wants to convert "latin1" to "ISO-8859-1". If one of your
> encodings is not UTF-8, you are probably Doing It Wrong. :)
Exactly. Note that the "you" in the above are usually plural,
collectively referring to both the sender and the receiver. I
usually am on the poor receiving end ;-)
^ permalink raw reply
* Re: [RFC PATCH v4] revision: new rev^-n shorthand for rev^n..rev
From: Jeff King @ 2016-09-27 6:10 UTC (permalink / raw)
To: Junio C Hamano
Cc: Vegard Nossum, git, Santi Béjar, Kevin Bracey, Philip Oakley,
Matthieu Moy, Ramsay Jones, Jakub Narębski
In-Reply-To: <xmqqh992pbq6.fsf@gitster.mtv.corp.google.com>
On Mon, Sep 26, 2016 at 02:55:45PM -0700, Junio C Hamano wrote:
> Taking these two together, perhaps squashing this in may be
> sufficient.
> [...]
> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> index 2c3da19..9474c37 100644
> --- a/builtin/rev-parse.c
> +++ b/builtin/rev-parse.c
> @@ -333,8 +333,22 @@ static int try_parent_shorthands(const char *arg)
> if (include_rev)
> show_rev(NORMAL, sha1, arg);
> commit = lookup_commit_reference(sha1);
> +
> + if (exclude_parent) {
> + /* do we have enough parents? */
> + for (parent_number = 0, parents = commit->parents;
> + parents;
> + parents = parents->next)
> + parent_number++;
> + if (parent_number < exclude_parent) {
> + *dotdot = '^';
> + return 0;
> + }
> + }
I think you can use commit_list_count() to make this a bit shorter,
like:
if (exclude_parent &&
commit_list_count(commit->parents) < exclude_parent) {
*dotdot = '^';
return 0;
}
Technically you can drop the first half of the &&, but it is probably a
good idea to avoid the traversal when exclude_parent is not in use.
Also technically, you can stop counting when you hit exclude_parent
(which is only possible with a custom traversal), but it is unlikely
enough that it is probably not worth caring about.
-Peff
^ permalink raw reply
* [PATCH] worktree: honor configuration variables
From: Junio C Hamano @ 2016-09-27 6:49 UTC (permalink / raw)
To: git
The command accesses default_abbrev (defined in environment.c and is
updated via core.abbrev configuration), but never makes any call to
git_config(). The output from "worktree list" ignores the abbrev
setting for this reason.
Make a call to git_config() to read the default set of configuration
variables at the beginning of the command.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/worktree.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 6dcf7bd..5c4854d 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -528,6 +528,8 @@ int cmd_worktree(int ac, const char **av, const char *prefix)
OPT_END()
};
+ git_config(git_default_config, NULL);
+
if (ac < 2)
usage_with_options(worktree_usage, options);
if (!prefix)
--
2.10.0-561-g98a6b79
^ permalink raw reply related
* Re: Stack read out-of-bounds in parse_sha1_header_extended using git 2.10.0
From: Jeff King @ 2016-09-27 7:19 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Gustavo Grieco, git
In-Reply-To: <xmqqtwd2sf9t.fsf@gitster.mtv.corp.google.com>
On Mon, Sep 26, 2016 at 11:10:54AM -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > I am inclined to say that it has no security implications. You have
> > to be able to write a bogus loose object in an object store you
> > already have write access to in the first place, in order to cause
> > this ...
>
> Note that you could social-engineer others to fetch from you and
> feed a small enough update that results in loose objects created in
> their repositories, without you having a direct write access to the
> repository.
>
> The codepath under discussion in this thread however cannot be used
> as an attack vector via that route, because the "fetch from
> elsewhere" codepath runs verification of the incoming data stream
> before storing the results (either in loose object files, or in a
> packfile) on disk.
I don't think it could be used at all for anything that speaks the git
protocol, because the object header is not present at all in a packfile.
So even if you hit unpack-objects, it would be writing the (correct)
loose object header itself.
But when we grab loose objects _directly_ from a remote, as in dumb-http
fetch, I'd suspect that the code doing the verification calls
unpack_sha1_header() as part of it. So I didn't test, but I'd strongly
suspect that's a viable attack vector.
I'm not sure what the actual attack would look like, though, aside from
locally accessing memory in a read-only way.
-Peff
^ permalink raw reply
* Re: [PATCH] git-gui: Do not reset author details on amend
From: Orgad Shaneh @ 2016-09-27 7:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Pat Thoyts, git
In-Reply-To: <xmqqmviupcpx.fsf@gitster.mtv.corp.google.com>
On Tue, Sep 27, 2016 at 12:34 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Orgad Shaneh <orgads@gmail.com> writes:
>
>> On Sun, Jul 10, 2016 at 7:36 AM, Orgad Shaneh <orgads@gmail.com> wrote:
>>
>>> On Wed, May 18, 2016 at 9:12 AM, Orgad Shaneh <orgads@gmail.com> wrote:
>>>> ping?
>>>>
>>> It's been over 2 months. Can anyone please review and merge it?
>>>
>> 4.5 months and counting... :(
>>>
>>>> On Thu, May 5, 2016 at 8:22 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>>>> Pat, we haven't heard from you for a long time. Are you still
>>>>> around and interested in helping us by maintaining git-gui?
>>>>>
>>>>> Otherwise we may have to start recruiting a volunteer or two to take
>>>>> this over.
>
> Sorry about that. No volunteers materialized yet X-<, and I really
> really do not want to apply anything other than trivial patches to
> it myself, as I am not a git-gui user.
>
This patch has been in use in Git for Windows for a decent period of time.
I actually see that there is a problem with it:
https://github.com/git-for-windows/git/issues/761
I'll try to revise it and resubmit.
- Orgad
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox