index-pack --verify (or verify-pack) is about verifying the pack
itself. SHA-1 collision test is about outside (probably malicious)
objects with the same SHA-1 entering current repo.
SHA-1 collision test is currently done unconditionally. Which means if
you verify an in-repo pack, all objects from the pack will be checked
against objects in repo, which are themselves.
Skip this test for --verify, unless --strict is also specified.
linux-2.6 $ ls -sh .git/objects/pack/pack-e7732c98a8d54840add294c3c562840f78764196.pack
401M .git/objects/pack/pack-e7732c98a8d54840add294c3c562840f78764196.pack
Without the patch (and with another patch to cut out second pass in
index-pack):
linux-2.6 $ time ~/w/git/old index-pack -v --verify .git/objects/pack/pack-e7732c98a8d54840add294c3c562840f78764196.pack
Indexing objects: 100% (1944656/1944656), done.
fatal: pack has 1617280 unresolved deltas
real 1m1.223s
user 0m55.028s
sys 0m0.828s
With the patch:
linux-2.6 $ time ~/w/git/git index-pack -v --verify .git/objects/pack/pack-e7732c98a8d54840add294c3c562840f78764196.pack
Indexing objects: 100% (1944656/1944656), done.
fatal: pack has 1617280 unresolved deltas
real 0m41.714s
user 0m40.994s
sys 0m0.550s
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
builtin/index-pack.c | 5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)
This command unpacks every non-delta objects in order to:
1. calculate sha-1
2. do byte-to-byte sha-1 collision test if we happen to have objects
with the same sha-1
3. validate object content in strict mode
All this requires the entire object to stay in memory, a bad news for
giant blobs. This patch lowers memory consumption by not saving the
object in memory whenever possible, calculating SHA-1 while unpacking
the object.
This patch assumes that the collision test is rarely needed. The
collision test will be done later in second pass if necessary, which
puts the entire object back to memory again (We could even do the
collision test without putting the entire object back in memory, by
comparing as we unpack it).
In strict mode, it always keeps non-blob objects in memory for
validation (blobs do not need data validation). "--strict --verify"
also keeps blobs in memory.
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
Does anybody do "git index-pack --stdin < .git/objects/pack/something"?
builtin/index-pack.c | 74 +++++++++++++++++++++++++++++++++++++++++---------
1 files changed, 61 insertions(+), 13 deletions(-)
From: Ian Kumlien <hidden> Date: 2016-06-15 22:53:08
On Fri, Feb 24, 2012 at 07:23:21PM +0700, Nguyễn Thái Ngọc Duy wrote:
This command unpacks every non-delta objects in order to:
1. calculate sha-1
2. do byte-to-byte sha-1 collision test if we happen to have objects
with the same sha-1
3. validate object content in strict mode
All this requires the entire object to stay in memory, a bad news for
giant blobs. This patch lowers memory consumption by not saving the
object in memory whenever possible, calculating SHA-1 while unpacking
the object.
This patch assumes that the collision test is rarely needed. The
collision test will be done later in second pass if necessary, which
puts the entire object back to memory again (We could even do the
collision test without putting the entire object back in memory, by
comparing as we unpack it).
In strict mode, it always keeps non-blob objects in memory for
validation (blobs do not need data validation). "--strict --verify"
also keeps blobs in memory.
I applied both patches to git master, with some manual tinkering so i
might have missed some change that caused this to break.
But i get a segmentation fault and i just thought that i'd send you a
small trace before i even start trying to look in to this:
0xb7eb5b43 in SHA1_Update () from /lib/i686/cmov/libcrypto.so.0.9.8
(gdb) bt
#0 0xb7eb5b43 in SHA1_Update () from /lib/i686/cmov/libcrypto.so.0.9.8
#1 0x08116a2d in write_sha1_file_prepare
#2 0x08116a83 in hash_sha1_file
#3 0x0807c2a6 in sha1_object
#4 0x0807d74a in parse_pack_objects
#5 0x0807de6f in cmd_index_pack
#6 0x0804be97 in run_builtin
#7 handle_internal_command
#8 0x0804c0ad in run_argv
#9 main
Sorry about the censorship but i don't know how sensetive this data
is...
sha1_file.c:2343
---
static void write_sha1_file_prepare(const void *buf, unsigned long len,
const char *type, unsigned char *sha1,
char *hdr, int *hdrlen)
{
git_SHA_CTX c;
/* Generate the header */
*hdrlen = sprintf(hdr, "%s %lu", type, len)+1;
/* Sha1.. */
git_SHA1_Init(&c);
git_SHA1_Update(&c, hdr, *hdrlen);
git_SHA1_Update(&c, buf, len); <== this line fails.
git_HA1_Final(sha1, &c);
}
---
Just keep sending patches, i have atleast one git to test it on. ;)
From: Ian Kumlien <hidden> Date: 2016-06-15 22:53:08
On Fri, Feb 24, 2012 at 07:23:21PM +0700, Nguyễn Thái Ngọc Duy wrote:
This command unpacks every non-delta objects in order to:
1. calculate sha-1
2. do byte-to-byte sha-1 collision test if we happen to have objects
with the same sha-1
3. validate object content in strict mode
All this requires the entire object to stay in memory, a bad news for
giant blobs. This patch lowers memory consumption by not saving the
object in memory whenever possible, calculating SHA-1 while unpacking
the object.
This patch assumes that the collision test is rarely needed. The
collision test will be done later in second pass if necessary, which
puts the entire object back to memory again (We could even do the
collision test without putting the entire object back in memory, by
comparing as we unpack it).
In strict mode, it always keeps non-blob objects in memory for
validation (blobs do not need data validation). "--strict --verify"
also keeps blobs in memory.
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
Actually, nevermind my last report - i had missed a merge :(
And now that i merged that part it seems like it doesn't do much..
(No real output for 2+ minutes)
I think i should reapply the patches again and verify that everything is
correct before reporting any additional progress.
But, this might not be before monday, unfortunately... But *thanks* for
posting the patches!
From: Ian Kumlien <hidden> Date: 2016-06-15 22:53:08
On Fri, Feb 24, 2012 at 07:23:21PM +0700, Nguyễn Thái Ngọc Duy wrote:
This command unpacks every non-delta objects in order to:
1. calculate sha-1
2. do byte-to-byte sha-1 collision test if we happen to have objects
with the same sha-1
3. validate object content in strict mode
All this requires the entire object to stay in memory, a bad news for
giant blobs. This patch lowers memory consumption by not saving the
object in memory whenever possible, calculating SHA-1 while unpacking
the object.
This patch assumes that the collision test is rarely needed. The
collision test will be done later in second pass if necessary, which
puts the entire object back to memory again (We could even do the
collision test without putting the entire object back in memory, by
comparing as we unpack it).
In strict mode, it always keeps non-blob objects in memory for
validation (blobs do not need data validation). "--strict --verify"
also keeps blobs in memory.
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
Finally, reapplied the patches and so on:
remote: Counting objects: 1425, done.
remote: Compressing objects: 100% (617/617), done.
remote: Total 1425 (delta 790), reused 1425 (delta 790)
Receiving objects: 100% (1425/1425), 56.06 MiB | 3.97 MiB/s, done.
Resolving deltas: 100% (790/790), done.
real 1m57.742s
user 0m29.950s
sys 0m6.308s
*YAY*
I wonder how the hell i could have missed several parts of the patch =(
But there seems to be some issue in gerrit 2.1.8, will have to check
against a newer gerrit to verify if it's still a problem.
FYI - it seems to hang doing nothing.
As for your patches:
Tested-by: Ian Kumlien <redacted>
;)
From: Ian Kumlien <hidden> Date: 2016-06-15 22:53:08
On Fri, Feb 24, 2012 at 07:23:21PM +0700, Nguyễn Thái Ngọc Duy wrote:
This command unpacks every non-delta objects in order to:
1. calculate sha-1
2. do byte-to-byte sha-1 collision test if we happen to have objects
with the same sha-1
3. validate object content in strict mode
All this requires the entire object to stay in memory, a bad news for
giant blobs. This patch lowers memory consumption by not saving the
object in memory whenever possible, calculating SHA-1 while unpacking
the object.
This patch assumes that the collision test is rarely needed. The
collision test will be done later in second pass if necessary, which
puts the entire object back to memory again (We could even do the
collision test without putting the entire object back in memory, by
comparing as we unpack it).
In strict mode, it always keeps non-blob objects in memory for
validation (blobs do not need data validation). "--strict --verify"
also keeps blobs in memory.
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
Writing objects: 100% (1425/1425), 56.06 MiB | 4.62 MiB/s, done.
Total 1425 (delta 790), reused 1425 (delta 790)
fatal: Out of memory, malloc failed (tried to allocate 3310214315 bytes)
fatal: Out of memory, malloc failed (tried to allocate 3310214315 bytes)
fatal: Out of memory, malloc failed (tried to allocate 3310214315 bytes)
fatal: Out of memory, malloc failed (tried to allocate 3310214315 bytes)
To ../test_data/
! [remote rejected] master -> master (missing necessary objects)
! [remote rejected] origin/HEAD -> origin/HEAD (missing necessary objects)
! [remote rejected] origin/master -> origin/master (missing necessary objects)
error: failed to push some refs to '../test_data/'
So there are additional code paths to look at... =(
OBJ_COMMIT)
+ if (info->revs->verify_objects && !obj->parsed &&
+ obj->type != OBJ_COMMIT && obj->type != OBJ_BLOB)
parse_object(obj->sha1);
show_object_with_name(stdout, obj, path, component);
}
-- 8< --
If not, you might need to apply this to generate coredump, then look
and see where that failed malloc comes from
-- 8< --
I get:
../git/git push --mirror ../test_data/
Counting objects: 1425, done.
Delta compression using up to 2 threads.
Compressing objects: 100% (617/617), done.
Writing objects: 100% (1425/1425), 56.06 MiB | 4.22 MiB/s, done.
Total 1425 (delta 790), reused 1425 (delta 790)
error: index-pack died of signal 11
error: unpack failed: index-pack abnormal exit
To ../test_data/
! [remote rejected] master -> master (n/a (unpacker error))
! [remote rejected] origin/HEAD -> origin/HEAD (n/a (unpacker error))
! [remote rejected] origin/master -> origin/master (n/a (unpacker error))
error: failed to push some refs to '../test_data/'
Which, to me, means that the installed git is now the problem - it can't verify
the pack and say that it's all ok ;)
I'll have to look some more at this on monday, or during the weekend if i get too curious =)
For now, thank $deity that $company i work for allows VPN from Linux machines! It looks
really good, i wonder if there is further tests i should do - any clues?
From: Ian Kumlien <hidden> Date: 2016-06-15 22:53:09
On Sat, Feb 25, 2012 at 08:49:55AM +0700, Nguyen Thai Ngoc Duy wrote:
2012/2/24 Ian Kumlien [off-list ref]:
quoted
Writing objects: 100% (1425/1425), 56.06 MiB | 4.62 MiB/s, done.
Total 1425 (delta 790), reused 1425 (delta 790)
fatal: Out of memory, malloc failed (tried to allocate 3310214315 bytes)
fatal: Out of memory, malloc failed (tried to allocate 3310214315 bytes)
fatal: Out of memory, malloc failed (tried to allocate 3310214315 bytes)
fatal: Out of memory, malloc failed (tried to allocate 3310214315 bytes)
To ../test_data/
! [remote rejected] master -> master (missing necessary objects)
! [remote rejected] origin/HEAD -> origin/HEAD (missing necessary objects)
! [remote rejected] origin/master -> origin/master (missing necessary objects)
error: failed to push some refs to '../test_data/'
So there are additional code paths to look at... =(
I can't say where that came from. Does this help? (Space damaged, may
need manual application)
If not, you might need to apply this to generate coredump, then look
and see where that failed malloc comes from
Actually, i added a backtrace and used addr2line to confirm my
suspicion... which is:
builtin/index-pack.c:414
ie get_data_from_pack...
It looks to me like, if we are to support this kind of things, we need a
slightly different approach - instead of passing the data around, it
feels like passing a function pointer around would be beneficial.
Looking at the code i see alot of places where this would be a issue,
just the fact that get_data_from_pack is used in several functions that
might do some small operation and then just free it.
I understand and recognize that my "problem" is not what git was
designed for; it was designed for small files, which is very evident in
how it approaches the data... And I'd most definetly have to look alot
closer to this code... =)
On Sun, Feb 26, 2012 at 5:45 AM, Ian Kumlien [off-list ref] wrote:
Actually, i added a backtrace and used addr2line to confirm my
suspicion... which is:
builtin/index-pack.c:414
ie get_data_from_pack...
That function should only be called when objects are deltified, which
should _not_ happen for large blobs. What is its caller?
It looks to me like, if we are to support this kind of things, we need a
slightly different approach - instead of passing the data around, it
feels like passing a function pointer around would be beneficial.
Looking at the code i see alot of places where this would be a issue,
just the fact that get_data_from_pack is used in several functions that
might do some small operation and then just free it.
I understand and recognize that my "problem" is not what git was
designed for; it was designed for small files, which is very evident in
how it approaches the data... And I'd most definetly have to look alot
closer to this code... =)
From: Ian Kumlien <hidden> Date: 2016-06-15 22:53:09
On Sun, Feb 26, 2012 at 11:10:14AM +0700, Nguyen Thai Ngoc Duy wrote:
On Sun, Feb 26, 2012 at 5:45 AM, Ian Kumlien [off-list ref] wrote:
quoted
Actually, i added a backtrace and used addr2line to confirm my
suspicion... which is:
builtin/index-pack.c:414
ie get_data_from_pack...
That function should only be called when objects are deltified, which
should _not_ happen for large blobs. What is its caller?
Full backtrace:
for x in 0x536031 0x451b0e 0x452212 0x4523f5 0x452711 0x452799 0x452bbb
0x454344 0x4170d1 0x41726c ; do addr2line $x -e ../git/git ; done
git/wrapper.c:41
git/builtin/index-pack.c:414
git/builtin/index-pack.c:588
git/builtin/index-pack.c:625
git/builtin/index-pack.c:679
git/builtin/index-pack.c:694
git/builtin/index-pack.c:805
git/builtin/index-pack.c:1246
git/git.c:308
git/git.c:467
Which means:
xmalloc
get_data_from_pack
get_base_data -- line just after: if (!delta_nr) {
resolve_delta
find_unresolved_deltas_1
find_unresolved_deltas
parse_pack_objects
cmd_index_pack
[skipping the git.c part]
Btw, i'm running these tests on a 64 bit laptop - since i'm not at work
;) (had to manually limit xmalloc but it triggers at the same point)