{"thread":{"id":"32714","subject":"[PATCH 1/7] sha1_file: keep track of where an SHA-1 object comes from","startedAt":"2013-01-24T08:42:14Z","lastAt":"2013-01-28T06:57:57Z","messageCount":21,"participants":["Nguyễn Thái Ngọc Duy","Junio C Hamano","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"207674","messageId":"1359016940-18849-1-git-send-email-pclouds@gmail.com","threadId":"32714","inReplyTo":null,"subject":"[PATCH 1/7] sha1_file: keep track of where an SHA-1 object comes from","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-01-24T08:42:14Z","receivedAt":"2013-01-24T08:42:14Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"We currently know if an object is loose or packed. We do not know if\nit's from the repo's object database, or via alternates\nmechanism. With this patch, sha1_object_info_extended() can tell if an\nobject comes from alternates source (and which one).\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n How about this way instead: we keep track of where objects come from\n so we can verify object source when we create or update something\n that contains SHA-1.\n\n 1/7 and 2/7 prepare for tracking object source. The rest verifies\n that new commits, trees, tags, indexes, refs or reflogs do not refer\n to an external source.\n\n This adds some cost when add_submodule_odb() is used. I did not\n measure, but I guess the added cost is much smaller compared to\n forking, especially on Windows. No breakages detected by the test\n suite, which is really good (or my code is really broken).\n\n builtin/index-pack.c |  2 +-\n cache.h              |  4 +++-\n fast-import.c        |  2 +-\n sha1_file.c          | 66 ++++++++++++++++++++++++++++++++++++----------------\n 4 files changed, 51 insertions(+), 23 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 43d364b..a7de3f8 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -1393,7 +1393,7 @@ static void read_v2_anomalous_offsets(struct packed_git *p,\n \n static void read_idx_option(struct pack_idx_option *opts, const char *pack_name)\n {\n-\tstruct packed_git *p = add_packed_git(pack_name, strlen(pack_name), 1);\n+\tstruct packed_git *p = add_packed_git(pack_name, strlen(pack_name), 1, NULL);\n \n \tif (!p)\n \t\tdie(_(\"Cannot open existing pack file '%s'\"), pack_name);\ndiff --git a/cache.h b/cache.h\nindex c257953..92854ab 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -978,6 +978,7 @@ struct pack_window {\n extern struct packed_git {\n \tstruct packed_git *next;\n \tstruct pack_window *windows;\n+\tstruct alternate_object_database *alt;\n \toff_t pack_size;\n \tconst void *index_data;\n \tsize_t index_size;\n@@ -1066,7 +1067,7 @@ extern void close_pack_windows(struct packed_git *);\n extern void unuse_pack(struct pack_window **);\n extern void free_pack_by_name(const char *);\n extern void clear_delta_base_cache(void);\n-extern struct packed_git *add_packed_git(const char *, int, int);\n+extern struct packed_git *add_packed_git(const char *, int, int, struct alternate_object_database *);\n extern const unsigned char *nth_packed_object_sha1(struct packed_git *, uint32_t);\n extern off_t nth_packed_object_offset(const struct packed_git *, uint32_t);\n extern off_t find_pack_entry_one(const unsigned char *, struct packed_git *);\n@@ -1102,6 +1103,7 @@ struct object_info {\n \t\t\tunsigned int is_delta;\n \t\t} packed;\n \t} u;\n+\tstruct alternate_object_database *alt;\n };\n extern int sha1_object_info_extended(const unsigned char *, struct object_info *);\n \ndiff --git a/fast-import.c b/fast-import.c\nindex c2a814e..4bf732e 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -964,7 +964,7 @@ static void end_packfile(void)\n \t\tidx_name = keep_pack(create_index());\n \n \t\t/* Register the packfile with core git's machinery. */\n-\t\tnew_p = add_packed_git(idx_name, strlen(idx_name), 1);\n+\t\tnew_p = add_packed_git(idx_name, strlen(idx_name), 1, NULL);\n \t\tif (!new_p)\n \t\t\tdie(\"core git rejected index %s\", idx_name);\n \t\tall_packs[pack_id] = new_p;\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 40b2329..afc7355 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -933,7 +933,8 @@ static void try_to_free_pack_memory(size_t size)\n \trelease_pack_memory(size, -1);\n }\n \n-struct packed_git *add_packed_git(const char *path, int path_len, int local)\n+struct packed_git *add_packed_git(const char *path, int path_len, int local,\n+\t\t\t\t  struct alternate_object_database *alt)\n {\n \tstatic int have_set_try_to_free_routine;\n \tstruct stat st;\n@@ -973,6 +974,7 @@ struct packed_git *add_packed_git(const char *path, int path_len, int local)\n \tp->mtime = st.st_mtime;\n \tif (path_len < 40 || get_sha1_hex(path + path_len - 40, p->sha1))\n \t\thashclr(p->sha1);\n+\tp->alt = alt;\n \treturn p;\n }\n \n@@ -1000,7 +1002,8 @@ void install_packed_git(struct packed_git *pack)\n \tpacked_git = pack;\n }\n \n-static void prepare_packed_git_one(char *objdir, int local)\n+static void prepare_packed_git_one(char *objdir, int local,\n+\t\t\t\t   struct alternate_object_database *alt)\n {\n \t/* Ensure that this buffer is large enough so that we can\n \t   append \"/pack/\" without clobbering the stack even if\n@@ -1041,7 +1044,7 @@ static void prepare_packed_git_one(char *objdir, int local)\n \t\t/* See if it really is a valid .idx file with corresponding\n \t\t * .pack file that we can map.\n \t\t */\n-\t\tp = add_packed_git(path, len + namelen, local);\n+\t\tp = add_packed_git(path, len + namelen, local, alt);\n \t\tif (!p)\n \t\t\tcontinue;\n \t\tinstall_packed_git(p);\n@@ -1110,11 +1113,11 @@ void prepare_packed_git(void)\n \n \tif (prepare_packed_git_run_once)\n \t\treturn;\n-\tprepare_packed_git_one(get_object_directory(), 1);\n+\tprepare_packed_git_one(get_object_directory(), 1, NULL);\n \tprepare_alt_odb();\n \tfor (alt = alt_odb_list; alt; alt = alt->next) {\n \t\talt->name[-1] = 0;\n-\t\tprepare_packed_git_one(alt->base, 0);\n+\t\tprepare_packed_git_one(alt->base, 0, alt);\n \t\talt->name[-1] = '/';\n \t}\n \trearrange_packed_git();\n@@ -1215,15 +1218,19 @@ static int git_open_noatime(const char *name)\n \t}\n }\n \n-static int open_sha1_file(const unsigned char *sha1)\n+static int open_sha1_file(const unsigned char *sha1,\n+\t\t\t  struct alternate_object_database **p_alt)\n {\n \tint fd;\n \tchar *name = sha1_file_name(sha1);\n \tstruct alternate_object_database *alt;\n \n \tfd = git_open_noatime(name);\n-\tif (fd >= 0)\n+\tif (fd >= 0) {\n+\t\tif (p_alt)\n+\t\t\t*p_alt = NULL;\n \t\treturn fd;\n+\t}\n \n \tprepare_alt_odb();\n \terrno = ENOENT;\n@@ -1231,18 +1238,23 @@ static int open_sha1_file(const unsigned char *sha1)\n \t\tname = alt->name;\n \t\tfill_sha1_path(name, sha1);\n \t\tfd = git_open_noatime(alt->base);\n-\t\tif (fd >= 0)\n+\t\tif (fd >= 0) {\n+\t\t\tif (p_alt)\n+\t\t\t\t*p_alt = alt;\n \t\t\treturn fd;\n+\t\t}\n \t}\n \treturn -1;\n }\n \n-void *map_sha1_file(const unsigned char *sha1, unsigned long *size)\n+static void *map_sha1_file_extended(const unsigned char *sha1,\n+\t\t\t\t    unsigned long *size,\n+\t\t\t\t    struct alternate_object_database **p_alt)\n {\n \tvoid *map;\n \tint fd;\n \n-\tfd = open_sha1_file(sha1);\n+\tfd = open_sha1_file(sha1, p_alt);\n \tmap = NULL;\n \tif (fd >= 0) {\n \t\tstruct stat st;\n@@ -1261,6 +1273,11 @@ void *map_sha1_file(const unsigned char *sha1, unsigned long *size)\n \treturn map;\n }\n \n+void *map_sha1_file(const unsigned char *sha1, unsigned long *size)\n+{\n+\treturn map_sha1_file_extended(sha1, size, NULL);\n+}\n+\n /*\n  * There used to be a second loose object header format which\n  * was meant to mimic the in-pack format, allowing for direct\n@@ -2096,7 +2113,9 @@ static int fill_pack_entry(const unsigned char *sha1,\n \treturn 1;\n }\n \n-static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)\n+static int find_pack_entry(const unsigned char *sha1,\n+\t\t\t   struct pack_entry *e,\n+\t\t\t   struct alternate_object_database **p_alt)\n {\n \tstruct packed_git *p;\n \n@@ -2104,14 +2123,19 @@ static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)\n \tif (!packed_git)\n \t\treturn 0;\n \n-\tif (last_found_pack && fill_pack_entry(sha1, e, last_found_pack))\n+\tif (last_found_pack && fill_pack_entry(sha1, e, last_found_pack)) {\n+\t\tif (p_alt)\n+\t\t\t*p_alt = last_found_pack->alt;\n \t\treturn 1;\n+\t}\n \n \tfor (p = packed_git; p; p = p->next) {\n \t\tif (p == last_found_pack || !fill_pack_entry(sha1, e, p))\n \t\t\tcontinue;\n \n \t\tlast_found_pack = p;\n+\t\tif (p_alt)\n+\t\t\t*p_alt = p->alt;\n \t\treturn 1;\n \t}\n \treturn 0;\n@@ -2130,7 +2154,9 @@ struct packed_git *find_sha1_pack(const unsigned char *sha1,\n \n }\n \n-static int sha1_loose_object_info(const unsigned char *sha1, unsigned long *sizep)\n+static int sha1_loose_object_info(const unsigned char *sha1,\n+\t\t\t\t  unsigned long *sizep,\n+\t\t\t\t  struct alternate_object_database **p_alt)\n {\n \tint status;\n \tunsigned long mapsize, size;\n@@ -2138,7 +2164,7 @@ static int sha1_loose_object_info(const unsigned char *sha1, unsigned long *size\n \tgit_zstream stream;\n \tchar hdr[32];\n \n-\tmap = map_sha1_file(sha1, &mapsize);\n+\tmap = map_sha1_file_extended(sha1, &mapsize, p_alt);\n \tif (!map)\n \t\treturn error(\"unable to find %s\", sha1_to_hex(sha1));\n \tif (unpack_sha1_header(&stream, map, mapsize, hdr, sizeof(hdr)) < 0)\n@@ -2168,9 +2194,9 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi)\n \t\treturn co->type;\n \t}\n \n-\tif (!find_pack_entry(sha1, &e)) {\n+\tif (!find_pack_entry(sha1, &e, &oi->alt)) {\n \t\t/* Most likely it's a loose object. */\n-\t\tstatus = sha1_loose_object_info(sha1, oi->sizep);\n+\t\tstatus = sha1_loose_object_info(sha1, oi->sizep, &oi->alt);\n \t\tif (status >= 0) {\n \t\t\toi->whence = OI_LOOSE;\n \t\t\treturn status;\n@@ -2178,7 +2204,7 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi)\n \n \t\t/* Not a loose object; someone else may have just packed it. */\n \t\treprepare_packed_git();\n-\t\tif (!find_pack_entry(sha1, &e))\n+\t\tif (!find_pack_entry(sha1, &e, &oi->alt))\n \t\t\treturn status;\n \t}\n \n@@ -2213,7 +2239,7 @@ static void *read_packed_sha1(const unsigned char *sha1,\n \tstruct pack_entry e;\n \tvoid *data;\n \n-\tif (!find_pack_entry(sha1, &e))\n+\tif (!find_pack_entry(sha1, &e, NULL))\n \t\treturn NULL;\n \tdata = cache_or_unpack_entry(e.p, e.offset, size, type, 1);\n \tif (!data) {\n@@ -2618,14 +2644,14 @@ int has_pack_index(const unsigned char *sha1)\n int has_sha1_pack(const unsigned char *sha1)\n {\n \tstruct pack_entry e;\n-\treturn find_pack_entry(sha1, &e);\n+\treturn find_pack_entry(sha1, &e, NULL);\n }\n \n int has_sha1_file(const unsigned char *sha1)\n {\n \tstruct pack_entry e;\n \n-\tif (find_pack_entry(sha1, &e))\n+\tif (find_pack_entry(sha1, &e, NULL))\n \t\treturn 1;\n \treturn has_loose_object(sha1);\n }\n-- \n1.8.0.rc3.18.g0d9b108\n"},{"id":"207675","messageId":"1359016940-18849-2-git-send-email-pclouds@gmail.com","threadId":"32714","inReplyTo":"1359016940-18849-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 2/7] sha1_file: separate alt object db from own repos and submodules's","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-01-24T08:42:15Z","receivedAt":"2013-01-24T08:42:15Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"A submodule's object database may be imported to in-core object pool\nfor a quick peek without paying the price of running a separate git\ncommand. These databases are marked in for stricter checks later to\navoid accidentially refering to a submodule's SHA-1 from main repo\n(except in gitlinks).\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n cache.h       |  4 +++-\n environment.c |  2 ++\n sha1_file.c   | 38 +++++++++++++++++++++++++++++---------\n submodule.c   |  2 +-\n 4 files changed, 35 insertions(+), 11 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 92854ab..b8d5826 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -561,6 +561,7 @@ extern int fsync_object_files;\n extern int core_preload_index;\n extern int core_apply_sparse_checkout;\n extern int precomposed_unicode;\n+extern int object_database_contaminated;\n \n enum branch_track {\n \tBRANCH_TRACK_UNSPECIFIED = -1,\n@@ -957,11 +958,12 @@ extern void remove_scheduled_dirs(void);\n \n extern struct alternate_object_database {\n \tstruct alternate_object_database *next;\n+\tint external;\n \tchar *name;\n \tchar base[FLEX_ARRAY]; /* more */\n } *alt_odb_list;\n extern void prepare_alt_odb(void);\n-extern void read_info_alternates(const char * relative_base, int depth);\n+extern void read_external_info_alternates(const char * relative_base, int depth);\n extern void add_to_alternates_file(const char *reference);\n typedef int alt_odb_fn(struct alternate_object_database *, void *);\n extern void foreach_alt_odb(alt_odb_fn, void*);\ndiff --git a/environment.c b/environment.c\nindex 85edd7f..3c90d95 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -65,6 +65,8 @@ unsigned long pack_size_limit_cfg;\n /* Parallel index stat data preload? */\n int core_preload_index = 0;\n \n+int object_database_contaminated;\n+\n /* This is set by setup_git_dir_gently() and/or git_default_config() */\n char *git_work_tree_cfg;\n static char *work_tree;\ndiff --git a/sha1_file.c b/sha1_file.c\nindex afc7355..af71122 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -58,6 +58,9 @@ static struct cached_object empty_tree = {\n \n static struct packed_git *last_found_pack;\n \n+static void read_info_alternates(const char * relative_base,\n+\t\t\t\t int depth, int external);\n+\n static struct cached_object *find_cached_object(const unsigned char *sha1)\n {\n \tint i;\n@@ -247,7 +250,10 @@ static int git_open_noatime(const char *name);\n  * SHA1, an extra slash for the first level indirection, and the\n  * terminating NUL.\n  */\n-static int link_alt_odb_entry(const char *entry, const char *relative_base, int depth)\n+static int link_alt_odb_entry(const char *entry,\n+\t\t\t      const char *relative_base,\n+\t\t\t      int depth,\n+\t\t\t      int external)\n {\n \tconst char *objdir = get_object_directory();\n \tstruct alternate_object_database *ent;\n@@ -277,6 +283,7 @@ static int link_alt_odb_entry(const char *entry, const char *relative_base, int\n \tmemcpy(ent->base, pathbuf.buf, pfxlen);\n \tstrbuf_release(&pathbuf);\n \n+\tent->external = external;\n \tent->name = ent->base + pfxlen + 1;\n \tent->base[pfxlen + 3] = '/';\n \tent->base[pfxlen] = ent->base[entlen-1] = 0;\n@@ -310,15 +317,19 @@ static int link_alt_odb_entry(const char *entry, const char *relative_base, int\n \tent->next = NULL;\n \n \t/* recursively add alternates */\n-\tread_info_alternates(ent->base, depth + 1);\n+\tread_info_alternates(ent->base, depth + 1, 0);\n \n \tent->base[pfxlen] = '/';\n \n+\tif (external)\n+\t\tobject_database_contaminated = 1;\n+\n \treturn 0;\n }\n \n static void link_alt_odb_entries(const char *alt, int len, int sep,\n-\t\t\t\t const char *relative_base, int depth)\n+\t\t\t\t const char *relative_base,\n+\t\t\t\t int depth, int external)\n {\n \tstruct string_list entries = STRING_LIST_INIT_NODUP;\n \tchar *alt_copy;\n@@ -340,14 +351,16 @@ static void link_alt_odb_entries(const char *alt, int len, int sep,\n \t\t\terror(\"%s: ignoring relative alternate object store %s\",\n \t\t\t\t\trelative_base, entry);\n \t\t} else {\n-\t\t\tlink_alt_odb_entry(entry, relative_base, depth);\n+\t\t\tlink_alt_odb_entry(entry, relative_base,\n+\t\t\t\t\t   depth, external);\n \t\t}\n \t}\n \tstring_list_clear(&entries, 0);\n \tfree(alt_copy);\n }\n \n-void read_info_alternates(const char * relative_base, int depth)\n+static void read_info_alternates(const char * relative_base,\n+\t\t\t\t int depth, int external)\n {\n \tchar *map;\n \tsize_t mapsz;\n@@ -371,11 +384,18 @@ void read_info_alternates(const char * relative_base, int depth)\n \tmap = xmmap(NULL, mapsz, PROT_READ, MAP_PRIVATE, fd, 0);\n \tclose(fd);\n \n-\tlink_alt_odb_entries(map, mapsz, '\\n', relative_base, depth);\n+\tlink_alt_odb_entries(map, mapsz, '\\n', relative_base,\n+\t\t\t     depth, external);\n \n \tmunmap(map, mapsz);\n }\n \n+void read_external_info_alternates(const char *relative_base,\n+\t\t\t\t   int depth)\n+{\n+\tread_info_alternates(relative_base, depth, 1);\n+}\n+\n void add_to_alternates_file(const char *reference)\n {\n \tstruct lock_file *lock = xcalloc(1, sizeof(struct lock_file));\n@@ -385,7 +405,7 @@ void add_to_alternates_file(const char *reference)\n \tif (commit_lock_file(lock))\n \t\tdie(\"could not close alternates file\");\n \tif (alt_odb_tail)\n-\t\tlink_alt_odb_entries(alt, strlen(alt), '\\n', NULL, 0);\n+\t\tlink_alt_odb_entries(alt, strlen(alt), '\\n', NULL, 0, 0);\n }\n \n void foreach_alt_odb(alt_odb_fn fn, void *cb)\n@@ -409,9 +429,9 @@ void prepare_alt_odb(void)\n \tif (!alt) alt = \"\";\n \n \talt_odb_tail = &alt_odb_list;\n-\tlink_alt_odb_entries(alt, strlen(alt), PATH_SEP, NULL, 0);\n+\tlink_alt_odb_entries(alt, strlen(alt), PATH_SEP, NULL, 0, 0);\n \n-\tread_info_alternates(get_object_directory(), 0);\n+\tread_info_alternates(get_object_directory(), 0, 0);\n }\n \n static int has_loose_object_local(const unsigned char *sha1)\ndiff --git a/submodule.c b/submodule.c\nindex 2f55436..8e4e2ec 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -65,7 +65,7 @@ static int add_submodule_odb(const char *path)\n \talt_odb_list = alt_odb;\n \n \t/* add possible alternates from the submodule */\n-\tread_info_alternates(objects_directory.buf, 0);\n+\tread_external_info_alternates(objects_directory.buf, 0);\n \tprepare_alt_odb();\n done:\n \tstrbuf_release(&objects_directory);\n-- \n1.8.0.rc3.18.g0d9b108\n"},{"id":"207676","messageId":"1359016940-18849-3-git-send-email-pclouds@gmail.com","threadId":"32714","inReplyTo":"1359016940-18849-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 3/7] sha1_file: refuse to write commits referring to external objects","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-01-24T08:42:16Z","receivedAt":"2013-01-24T08:42:16Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n sha1_file.c | 34 ++++++++++++++++++++++++++++++++++\n 1 file changed, 34 insertions(+)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex af71122..5948dcb 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2615,12 +2615,46 @@ static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,\n \treturn move_temp_to_file(tmp_file, filename);\n }\n \n+static void check_sha1_file_for_external_source(const char *buf,\n+\t\t\t\t\t\tunsigned long len,\n+\t\t\t\t\t\tconst char *type)\n+{\n+\tstruct object_info oi;\n+\tunsigned char sha1[20];\n+\n+\t/*\n+\t * It is assumed that the object is well formatted. Otherwise\n+\t * die() is waiting..\n+\t */\n+\tif (!strcmp(type, \"commit\")) {\n+\t\tconst char *tail = buf + len;\n+\t\tif (get_sha1_hex(buf + 5, sha1) < 0 ||\n+\t\t    sha1_object_info_extended(sha1, &oi) != OBJ_TREE ||\n+\t\t    (oi.alt && oi.alt->external))\n+\t\t\tdie(\"cannot create a commit with external tree %s\",\n+\t\t\t    sha1_to_hex(sha1));\n+\t\tbuf += 46; /* \"tree \" + \"hex sha1\" + \"\\n\" */\n+\n+\t\twhile (buf + 48 < tail && !memcmp(buf, \"parent \", 7)) {\n+\t\t\tif (get_sha1_hex(buf + 7, sha1) < 0 ||\n+\t\t\t    sha1_object_info_extended(sha1, &oi) != OBJ_COMMIT ||\n+\t\t\t    (oi.alt && oi.alt->external))\n+\t\t\t\tdie(\"cannot create a commit with external parent %s\",\n+\t\t\t\t    sha1_to_hex(sha1));\n+\t\t\tbuf += 48; /* \"parent \" + hex sha1 + \"\\n\" */\n+\t\t}\n+\t}\n+}\n+\n int write_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *returnsha1)\n {\n \tunsigned char sha1[20];\n \tchar hdr[32];\n \tint hdrlen;\n \n+\tif (object_database_contaminated)\n+\t\tcheck_sha1_file_for_external_source(buf, len, type);\n+\n \t/* Normally if we have it in the pack then we do not bother writing\n \t * it out into .git/objects/??/?{38} file.\n \t */\n-- \n1.8.0.rc3.18.g0d9b108\n"},{"id":"207677","messageId":"1359016940-18849-4-git-send-email-pclouds@gmail.com","threadId":"32714","inReplyTo":"1359016940-18849-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 4/7] sha1_file: refuse to write trees referring to external objects","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-01-24T08:42:17Z","receivedAt":"2013-01-24T08:42:17Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n sha1_file.c | 23 +++++++++++++++++++++++\n 1 file changed, 23 insertions(+)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 5948dcb..ec3a040 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2643,6 +2643,29 @@ static void check_sha1_file_for_external_source(const char *buf,\n \t\t\t\t    sha1_to_hex(sha1));\n \t\t\tbuf += 48; /* \"parent \" + hex sha1 + \"\\n\" */\n \t\t}\n+\t} else if (!strcmp(type, \"tree\")) {\n+\t\tstruct tree_desc desc;\n+\t\tstruct name_entry entry;\n+\n+\t\tinit_tree_desc(&desc, buf, len);\n+\t\twhile (tree_entry(&desc, &entry))\n+\t\t\tswitch (entry.mode) {\n+\t\t\tcase S_IFGITLINK:\n+\t\t\t\t/* we already know we probably don't have this SHA-1 */\n+\t\t\t\tbreak;\n+\t\t\tcase S_IFDIR:\n+\t\t\t\tif (sha1_object_info_extended(entry.sha1, &oi) != OBJ_TREE ||\n+\t\t\t\t    (oi.alt && oi.alt->external))\n+\t\t\t\t\tdie(\"cannot create a tree with external tree %s\",\n+\t\t\t\t\t    sha1_to_hex(entry.sha1));\n+\t\t\t\tbreak;\n+\t\t\tdefault:\n+\t\t\t\tif (sha1_object_info_extended(entry.sha1, &oi) != OBJ_BLOB ||\n+\t\t\t\t    (oi.alt && oi.alt->external))\n+\t\t\t\t\tdie(\"cannot create a tree with external blob %s\",\n+\t\t\t\t\t    sha1_to_hex(entry.sha1));\n+\t\t\t\tbreak;\n+\t\t\t}\n \t}\n }\n \n-- \n1.8.0.rc3.18.g0d9b108\n"},{"id":"207678","messageId":"1359016940-18849-5-git-send-email-pclouds@gmail.com","threadId":"32714","inReplyTo":"1359016940-18849-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 5/7] sha1_file: refuse to write tags referring to external objects","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-01-24T08:42:18Z","receivedAt":"2013-01-24T08:42:18Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n sha1_file.c | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex ec3a040..01681e5 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2666,6 +2666,12 @@ static void check_sha1_file_for_external_source(const char *buf,\n \t\t\t\t\t    sha1_to_hex(entry.sha1));\n \t\t\t\tbreak;\n \t\t\t}\n+\t} else if (!strcmp(type, \"tag\")) {\n+\t\tif (get_sha1_hex(buf + 7, sha1) < 0 ||\n+\t\t    sha1_object_info_extended(sha1, &oi) != OBJ_TREE ||\n+\t\t    (oi.alt && oi.alt->external))\n+\t\t\tdie(\"cannot create a tag with external tree %s\",\n+\t\t\t    sha1_to_hex(sha1));\n \t}\n }\n \n-- \n1.8.0.rc3.18.g0d9b108\n"},{"id":"207679","messageId":"1359016940-18849-6-git-send-email-pclouds@gmail.com","threadId":"32714","inReplyTo":"1359016940-18849-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 6/7] read-cache: refuse to create index referring to external objects","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-01-24T08:42:19Z","receivedAt":"2013-01-24T08:42:19Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n read-cache.c | 20 ++++++++++++++++++++\n 1 file changed, 20 insertions(+)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex fda78bc..4579215 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1720,6 +1720,26 @@ static int ce_write_entry(git_SHA_CTX *c, int fd, struct cache_entry *ce,\n \t\t\t      ce->name + common, ce_namelen(ce) - common);\n \t}\n \n+\tif (object_database_contaminated) {\n+\t\tstruct object_info oi;\n+\t\tswitch (ce->ce_mode) {\n+\t\tcase S_IFGITLINK:\n+\t\t\tbreak;\n+\t\tcase S_IFDIR:\n+\t\t\tif (sha1_object_info_extended(ce->sha1, &oi) != OBJ_TREE ||\n+\t\t\t    (oi.alt && oi.alt->external))\n+\t\t\t\tdie(\"cannot create index referring to an external tree %s\",\n+\t\t\t\t    sha1_to_hex(ce->sha1));\n+\t\t\tbreak;\n+\t\tdefault:\n+\t\t\tif (sha1_object_info_extended(ce->sha1, &oi) != OBJ_BLOB ||\n+\t\t\t\t    (oi.alt && oi.alt->external))\n+\t\t\t\tdie(\"cannot create index referring to an external blob %s\",\n+\t\t\t\t    sha1_to_hex(ce->sha1));\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\n \tresult = ce_write(c, fd, ondisk, size);\n \tfree(ondisk);\n \treturn result;\n-- \n1.8.0.rc3.18.g0d9b108\n"},{"id":"207680","messageId":"1359016940-18849-7-git-send-email-pclouds@gmail.com","threadId":"32714","inReplyTo":"1359016940-18849-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 7/7] refs: do not update ref(log) referring to external objects","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-01-24T08:42:20Z","receivedAt":"2013-01-24T08:42:20Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n refs.c | 24 ++++++++++++++++++++++++\n 1 file changed, 24 insertions(+)\n\ndiff --git a/refs.c b/refs.c\nindex 541fec2..86003af 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2014,6 +2014,18 @@ static int log_ref_write(const char *refname, const unsigned char *old_sha1,\n \tchar *logrec;\n \tconst char *committer;\n \n+\tif (object_database_contaminated) {\n+\t\tstruct object_info oi;\n+\t\tif (sha1_object_info_extended(old_sha1, &oi) == -1 ||\n+\t\t    (oi.alt && oi.alt->external))\n+\t\t\treturn error(\"cannot update reflog pointing to an external SHA-1 %s\",\n+\t\t\t\t     sha1_to_hex(old_sha1));\n+\t\tif (sha1_object_info_extended(new_sha1, &oi) == -1 ||\n+\t\t    (oi.alt && oi.alt->external))\n+\t\t\treturn error(\"cannot update reflog pointing to an external SHA-1 %s\",\n+\t\t\t\t     sha1_to_hex(new_sha1));\n+\t}\n+\n \tif (log_all_ref_updates < 0)\n \t\tlog_all_ref_updates = !is_bare_repository();\n \n@@ -2054,6 +2066,18 @@ int write_ref_sha1(struct ref_lock *lock,\n \n \tif (!lock)\n \t\treturn -1;\n+\n+\tif (object_database_contaminated) {\n+\t\tstruct object_info oi;\n+\t\tif (sha1_object_info_extended(sha1, &oi) == -1 ||\n+\t\t    (oi.alt && oi.alt->external)) {\n+\t\t\terror(\"cannot update ref pointing to an external SHA-1 %s\",\n+\t\t\t      sha1_to_hex(sha1));\n+\t\t\tunlock_ref(lock);\n+\t\t\treturn -1;\n+\t\t}\n+\t}\n+\n \tif (!lock->force_write && !hashcmp(lock->old_sha1, sha1)) {\n \t\tunlock_ref(lock);\n \t\treturn 0;\n-- \n1.8.0.rc3.18.g0d9b108\n"},{"id":"207697","messageId":"7va9rycw4t.fsf@alter.siamese.dyndns.org","threadId":"32714","inReplyTo":"1359016940-18849-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 1/7] sha1_file: keep track of where an SHA-1 object comes from","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-24T17:45:54Z","receivedAt":"2013-01-24T17:45:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:\n\n> We currently know if an object is loose or packed. We do not know if\n> it's from the repo's object database, or via alternates\n> mechanism. With this patch, sha1_object_info_extended() can tell if an\n> object comes from alternates source (and which one).\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n>  How about this way instead: we keep track of where objects come from\n>  so we can verify object source when we create or update something\n>  that contains SHA-1.\n\nThe overall approach taken by this series may be worth considering, but\nI do not think the check implemented here is correct.\n\nAn object may be found in an alternate odb but we may also have our\nown copy of the same object.  You need to prove that a suspicious\nobject is visible to us *ONLY* through add_submodule_odb().\n\nOnce you do add_submodule_odb() to contaminate our object pool, you\nmake everything a suspicious object that needs to be checked; that\nis the worst part of the story.\n\nIf I understand the test case that triggers the issue correctly, it\nmakes a merge in the top-level superproject, and while doing so,\ntries to three-way merge gitlink changes, so that it can detect\nfast-forward situation inside the submodule.  It should just be done\nwith running \"git merge-base A B\" there; after all, we are in the\ntree-wide merge of the top-level superproject, which is already a\nheavy-weight operation. I do not see a reason to make it more brittle\nthan necessary.\n\nIf we rip out add_submodule_odb() we do not have to ever worry about\nthis kind of object database breakages that are hard to track down.\nIt is an optimization to allow the code become more brittle, violate\nthe project boundary, and break the object database faster as a\nresult.  That is not a good optimization at all.\n"},{"id":"207703","messageId":"7vpq0ubdec.fsf@alter.siamese.dyndns.org","threadId":"32714","inReplyTo":"1359016940-18849-6-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 6/7] read-cache: refuse to create index referring to external objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-24T19:15:55Z","receivedAt":"2013-01-24T19:15:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n>  read-cache.c | 20 ++++++++++++++++++++\n>  1 file changed, 20 insertions(+)\n>\n> diff --git a/read-cache.c b/read-cache.c\n> index fda78bc..4579215 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -1720,6 +1720,26 @@ static int ce_write_entry(git_SHA_CTX *c, int fd, struct cache_entry *ce,\n>  \t\t\t      ce->name + common, ce_namelen(ce) - common);\n>  \t}\n>  \n> +\tif (object_database_contaminated) {\n> +\t\tstruct object_info oi;\n> +\t\tswitch (ce->ce_mode) {\n> +\t\tcase S_IFGITLINK:\n> +\t\t\tbreak;\n> +\t\tcase S_IFDIR:\n> +\t\t\tif (sha1_object_info_extended(ce->sha1, &oi) != OBJ_TREE ||\n\nThis case should never happen.  Do we store any tree object in the\nindex in the first place?\n\n> +\t\t\t    (oi.alt && oi.alt->external))\n> +\t\t\t\tdie(\"cannot create index referring to an external tree %s\",\n> +\t\t\t\t    sha1_to_hex(ce->sha1));\n> +\t\t\tbreak;\n> +\t\tdefault:\n> +\t\t\tif (sha1_object_info_extended(ce->sha1, &oi) != OBJ_BLOB ||\n> +\t\t\t\t    (oi.alt && oi.alt->external))\n> +\t\t\t\tdie(\"cannot create index referring to an external blob %s\",\n> +\t\t\t\t    sha1_to_hex(ce->sha1));\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\n>  \tresult = ce_write(c, fd, ondisk, size);\n>  \tfree(ondisk);\n>  \treturn result;\n\nI think the check you want to add is to the cache-tree code; before\nwriting the new index out, if you suspect you might have called\ncache_tree_update() before, invalidate any hierarchy that is\nrecorded to produce a tree object that refers to an object that is\nonly available through external object store.\n\nAs to the logic to check if a object is something that we should\nrefuse to create a new dependent, I think you should not butcher\nsha1_object_info_extended(), and instead add a new call that checks\nif a given SHA-1 yields an object if you ignore alternate object\ndatabases that are marked as \"external\", whose signature may\nresemble:\n\n\tint has_sha1_file_proper(const unsigned char *sha1);\n\nor something.\n\nThis is a tangent, but in addition, you may want to add an even\nnarrower variant that checks the same but ignoring _all_ alternate\nobject databases, \"external\" or not:\n\n        int has_sha1_file_local(const unsigned char *sha1);\n\nThat may be useful if we want to later make the alternate safer to\nuse; instead of letting the codepath to create an object ask\nhas_sha1_file() to see if it already exists and refrain from writing\na new copy, we switch to ask has_sha1_file_locally() and even if an\nalternate object database we borrow from has it, we keep our own\ncopy in our repository.\n\nNot tested beyond syntax, but rebasing something like this patch on\ntop of your \"mark external sources\" change may take us closer to\nthat.\n\n cache.h     |  2 ++\n sha1_file.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-----\n 2 files changed, 57 insertions(+), 5 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 1f96f65..8d651b6 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -766,6 +766,8 @@ extern int move_temp_to_file(const char *tmpfile, const char *filename);\n \n extern int has_sha1_pack(const unsigned char *sha1);\n extern int has_sha1_file(const unsigned char *sha1);\n+extern int has_sha1_file_proper(const unsigned char *sha1);\n+extern int has_sha1_file_local(const unsigned char *sha1);\n extern int has_loose_object_nonlocal(const unsigned char *sha1);\n \n extern int has_pack_index(const unsigned char *sha1);\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 40b2329..1a3192a 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -420,11 +420,18 @@ static int has_loose_object_local(const unsigned char *sha1)\n \treturn !access(name, F_OK);\n }\n \n-int has_loose_object_nonlocal(const unsigned char *sha1)\n+enum odb_origin {\n+\todb_default = 0, odb_proper, odb_local\n+};\n+\n+static int has_loose_object_nonlocal_limited(const unsigned char *sha1,\n+\t\t\t\t\t     enum odb_origin origin)\n {\n \tstruct alternate_object_database *alt;\n \tprepare_alt_odb();\n \tfor (alt = alt_odb_list; alt; alt = alt->next) {\n+\t\tif (origin == odb_proper && 0 /* alt->external */)\n+\t\t\tcontinue;\n \t\tfill_sha1_path(alt->name, sha1);\n \t\tif (!access(alt->base, F_OK))\n \t\t\treturn 1;\n@@ -432,6 +439,11 @@ int has_loose_object_nonlocal(const unsigned char *sha1)\n \treturn 0;\n }\n \n+int has_loose_object_nonlocal(const unsigned char *sha1)\n+{\n+\treturn has_loose_object_nonlocal_limited(sha1, odb_default);\n+}\n+\n static int has_loose_object(const unsigned char *sha1)\n {\n \treturn has_loose_object_local(sha1) ||\n@@ -2062,12 +2074,28 @@ int is_pack_valid(struct packed_git *p)\n \treturn !open_packed_git(p);\n }\n \n+static int limit_pack_odb_origin(enum odb_origin origin, struct packed_git *p)\n+{\n+\tswitch (origin) {\n+\tdefault:\n+\t\treturn 0;\n+\tcase odb_proper:\n+\t\treturn 0; /* p->external */\n+\tcase odb_local:\n+\t\treturn !p->pack_local;\n+\t}\n+}\n+\n static int fill_pack_entry(const unsigned char *sha1,\n \t\t\t   struct pack_entry *e,\n-\t\t\t   struct packed_git *p)\n+\t\t\t   struct packed_git *p,\n+\t\t\t   enum odb_origin origin)\n {\n \toff_t offset;\n \n+\tif (limit_pack_odb_origin(origin, p))\n+\t\treturn 0;\n+\n \tif (p->num_bad_objects) {\n \t\tunsigned i;\n \t\tfor (i = 0; i < p->num_bad_objects; i++)\n@@ -2096,7 +2124,8 @@ static int fill_pack_entry(const unsigned char *sha1,\n \treturn 1;\n }\n \n-static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)\n+static int find_pack_entry_limited(const unsigned char *sha1, struct pack_entry *e,\n+\t\t\t\t   enum odb_origin origin)\n {\n \tstruct packed_git *p;\n \n@@ -2104,11 +2133,11 @@ static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)\n \tif (!packed_git)\n \t\treturn 0;\n \n-\tif (last_found_pack && fill_pack_entry(sha1, e, last_found_pack))\n+\tif (last_found_pack && fill_pack_entry(sha1, e, last_found_pack, origin))\n \t\treturn 1;\n \n \tfor (p = packed_git; p; p = p->next) {\n-\t\tif (p == last_found_pack || !fill_pack_entry(sha1, e, p))\n+\t\tif (p == last_found_pack || !fill_pack_entry(sha1, e, p, origin))\n \t\t\tcontinue;\n \n \t\tlast_found_pack = p;\n@@ -2117,6 +2146,11 @@ static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)\n \treturn 0;\n }\n \n+static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)\n+{\n+\treturn find_pack_entry_limited(sha1, e, odb_default);\n+}\n+\n struct packed_git *find_sha1_pack(const unsigned char *sha1,\n \t\t\t\t  struct packed_git *packs)\n {\n@@ -2630,6 +2664,22 @@ int has_sha1_file(const unsigned char *sha1)\n \treturn has_loose_object(sha1);\n }\n \n+int has_sha1_file_local(const unsigned char *sha1)\n+{\n+\tstruct pack_entry e;\n+\tif (find_pack_entry_limited(sha1, &e, odb_local))\n+\t\treturn 1;\n+\treturn has_loose_object_local(sha1);\n+}\n+\n+int has_sha1_file_proper(const unsigned char *sha1)\n+{\n+\tstruct pack_entry e;\n+\treturn (!!find_pack_entry_limited(sha1, &e, odb_proper) ||\n+\t\thas_loose_object_local(sha1) ||\n+\t\thas_loose_object_nonlocal_limited(sha1, odb_proper));\n+}\n+\n static void check_tree(const void *buf, size_t size)\n {\n \tstruct tree_desc desc;\n"},{"id":"207739","messageId":"CACsJy8Ag6v7wUnupRwGid26AUzgZ=WbdA5F-MpjUv5ktaj5Asg@mail.gmail.com","threadId":"32714","inReplyTo":"7va9rycw4t.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/7] sha1_file: keep track of where an SHA-1 object comes from","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-01-25T01:38:38Z","receivedAt":"2013-01-25T01:38:38Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Jan 25, 2013 at 12:45 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>>  How about this way instead: we keep track of where objects come from\n>>  so we can verify object source when we create or update something\n>>  that contains SHA-1.\n>\n> The overall approach taken by this series may be worth considering, but\n> I do not think the check implemented here is correct.\n>\n> An object may be found in an alternate odb but we may also have our\n> own copy of the same object.  You need to prove that a suspicious\n> object is visible to us *ONLY* through add_submodule_odb().\n\nThe way alt odbs are linked (new odbs area always at the end), if we\nhave the same copy, their copy will never be read (we check out alt\nodbs from head to tail). So those duplicate suspicious objects are\nactually invisible to us.\n\n> Once you do add_submodule_odb() to contaminate our object pool, you\n> make everything a suspicious object that needs to be checked; that\n> is the worst part of the story.\n\nAnd because we never really touch their alt copy, the returned alt\nsource is \"ours\", trusted and not checked. The check should only occur\non objects that we do not have.\n-- \nDuy\n"},{"id":"207741","messageId":"CACsJy8C3tLOHCK4Qc--W630do0M=xLKSMoYUxxv2_GDaUXaRww@mail.gmail.com","threadId":"32714","inReplyTo":"7vpq0ubdec.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 6/7] read-cache: refuse to create index referring to external objects","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-01-25T02:00:16Z","receivedAt":"2013-01-25T02:00:16Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Jan 25, 2013 at 2:15 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n>\n>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n>> ---\n>>  read-cache.c | 20 ++++++++++++++++++++\n>>  1 file changed, 20 insertions(+)\n>>\n>> diff --git a/read-cache.c b/read-cache.c\n>> index fda78bc..4579215 100644\n>> --- a/read-cache.c\n>> +++ b/read-cache.c\n>> @@ -1720,6 +1720,26 @@ static int ce_write_entry(git_SHA_CTX *c, int fd, struct cache_entry *ce,\n>>                             ce->name + common, ce_namelen(ce) - common);\n>>       }\n>>\n>> +     if (object_database_contaminated) {\n>> +             struct object_info oi;\n>> +             switch (ce->ce_mode) {\n>> +             case S_IFGITLINK:\n>> +                     break;\n>> +             case S_IFDIR:\n>> +                     if (sha1_object_info_extended(ce->sha1, &oi) != OBJ_TREE ||\n>\n> This case should never happen.  Do we store any tree object in the\n> index in the first place?\n\nNo it was copy/paste mistake (from cache-tree.c, before I deleted it\nand added check_sha1_file_for_external_source in 3/7)\n\n>> +                         (oi.alt && oi.alt->external))\n>> +                             die(\"cannot create index referring to an external tree %s\",\n>> +                                 sha1_to_hex(ce->sha1));\n>> +                     break;\n>> +             default:\n>> +                     if (sha1_object_info_extended(ce->sha1, &oi) != OBJ_BLOB ||\n>> +                                 (oi.alt && oi.alt->external))\n>> +                             die(\"cannot create index referring to an external blob %s\",\n>> +                                 sha1_to_hex(ce->sha1));\n>> +                     break;\n>> +             }\n>> +     }\n>> +\n>>       result = ce_write(c, fd, ondisk, size);\n>>       free(ondisk);\n>>       return result;\n>\n> I think the check you want to add is to the cache-tree code; before\n> writing the new index out, if you suspect you might have called\n> cache_tree_update() before, invalidate any hierarchy that is\n> recorded to produce a tree object that refers to an object that is\n> only available through external object store.\n\ncache-tree is covered by check_sha1_file_for_external_source() when it\nactually writes a tree (dry-run mode goes through unchecked). Even\nwhen cache-tree is not involved, I do not want the index to point to\nan non-existing SHA-1 (\"git diff --cached\" may fail next time, for\nexample).\n\n> As to the logic to check if a object is something that we should\n> refuse to create a new dependent, I think you should not butcher\n> sha1_object_info_extended(), and instead add a new call that checks\n> if a given SHA-1 yields an object if you ignore alternate object\n> databases that are marked as \"external\", whose signature may\n> resemble:\n>\n>         int has_sha1_file_proper(const unsigned char *sha1);\n>\n> or something.\n\nAgreed.\n\n> This is a tangent, but in addition, you may want to add an even\n> narrower variant that checks the same but ignoring _all_ alternate\n> object databases, \"external\" or not:\n>\n>         int has_sha1_file_local(const unsigned char *sha1);\n>\n> That may be useful if we want to later make the alternate safer to\n> use; instead of letting the codepath to create an object ask\n> has_sha1_file() to see if it already exists and refrain from writing\n> a new copy, we switch to ask has_sha1_file_locally() and even if an\n> alternate object database we borrow from has it, we keep our own\n> copy in our repository.\n>\n> Not tested beyond syntax, but rebasing something like this patch on\n> top of your \"mark external sources\" change may take us closer to\n> that.\n\nThanks, will incorporate in the reroll.\n-- \nDuy\n"},{"id":"207745","messageId":"7v4ni59ano.fsf@alter.siamese.dyndns.org","threadId":"32714","inReplyTo":"CACsJy8Ag6v7wUnupRwGid26AUzgZ=WbdA5F-MpjUv5ktaj5Asg@mail.gmail.com","subject":"Re: [PATCH 1/7] sha1_file: keep track of where an SHA-1 object comes from","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-25T03:58:03Z","receivedAt":"2013-01-25T03:58:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen <pclouds@gmail.com> writes:\n\n> On Fri, Jan 25, 2013 at 12:45 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>>>  How about this way instead: we keep track of where objects come from\n>>>  so we can verify object source when we create or update something\n>>>  that contains SHA-1.\n>>\n>> The overall approach taken by this series may be worth considering, but\n>> I do not think the check implemented here is correct.\n>>\n>> An object may be found in an alternate odb but we may also have our\n>> own copy of the same object.  You need to prove that a suspicious\n>> object is visible to us *ONLY* through add_submodule_odb().\n>\n> The way alt odbs are linked (new odbs area always at the end), if we\n> have the same copy, their copy will never be read (we check out alt\n> odbs from head to tail). So those duplicate suspicious objects are\n> actually invisible to us.\n\nThe way I read find_pack_entry() is that our heuristics to start\nour search by looking at the pack in which we found an object the\nlast time will invalidate your assumption above.  Am I mistaken?\n"},{"id":"207747","messageId":"CACsJy8Dv6jVO2BJpQ3H_f7NEkzuZSYOipsbPatAjciufd3X4qQ@mail.gmail.com","threadId":"32714","inReplyTo":"7v4ni59ano.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/7] sha1_file: keep track of where an SHA-1 object comes from","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-01-25T04:02:12Z","receivedAt":"2013-01-25T04:02:12Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Jan 25, 2013 at 10:58 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Duy Nguyen <pclouds@gmail.com> writes:\n>\n>> On Fri, Jan 25, 2013 at 12:45 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>>>>  How about this way instead: we keep track of where objects come from\n>>>>  so we can verify object source when we create or update something\n>>>>  that contains SHA-1.\n>>>\n>>> The overall approach taken by this series may be worth considering, but\n>>> I do not think the check implemented here is correct.\n>>>\n>>> An object may be found in an alternate odb but we may also have our\n>>> own copy of the same object.  You need to prove that a suspicious\n>>> object is visible to us *ONLY* through add_submodule_odb().\n>>\n>> The way alt odbs are linked (new odbs area always at the end), if we\n>> have the same copy, their copy will never be read (we check out alt\n>> odbs from head to tail). So those duplicate suspicious objects are\n>> actually invisible to us.\n>\n> The way I read find_pack_entry() is that our heuristics to start\n> our search by looking at the pack in which we found an object the\n> last time will invalidate your assumption above.  Am I mistaken?\n\nNo, you are right. Loose objects are always searched from the start of\nalt odb list. Packs are searched till the end then back to head again.\n-- \nDuy\n"},{"id":"207824","messageId":"7vpq0t3x60.fsf@alter.siamese.dyndns.org","threadId":"32714","inReplyTo":"CACsJy8C3tLOHCK4Qc--W630do0M=xLKSMoYUxxv2_GDaUXaRww@mail.gmail.com","subject":"Re: [PATCH 6/7] read-cache: refuse to create index referring to external objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-25T19:00:39Z","receivedAt":"2013-01-25T19:00:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen <pclouds@gmail.com> writes:\n\n> .... Even\n> when cache-tree is not involved, I do not want the index to point to\n> an non-existing SHA-1 (\"git diff --cached\" may fail next time, for\n> example).\n\nI think we have tests that explicitly add SHA-1 that names an object\nthat does not exist to the index and check failures; you may have to\nthink what to do with them.\n\n>> This is a tangent, but in addition, you may want to add an even\n>> narrower variant that checks the same but ignoring _all_ alternate\n>> object databases, \"external\" or not:\n>>\n>>         int has_sha1_file_local(const unsigned char *sha1);\n>>\n>> That may be useful if we want to later make the alternate safer to\n>> use; instead of letting the codepath to create an object ask\n>> has_sha1_file() to see if it already exists and refrain from writing\n>> a new copy, we switch to ask has_sha1_file_locally() and even if an\n>> alternate object database we borrow from has it, we keep our own\n>> copy in our repository.\n\nThis is not a tangent, but if you want to go this \"forbid making our\nrepository depend on objects we do not have but we know about after\nwe peek submodule odb\" route [*1*], write_sha1_file() needs to be\ntold about has_sha1_file_proper().  We may \"git add\" a new blob in\nthe superproject, the blob may not yet exist in *our* repository,\nbut may happen to already exist in the submodue odb.  In such a\ncase, write_sha1_file() has to write that blob in our repository,\nwithout the existing has_sha1_file() check bypassing it.  Otherwise\nour attempt to create a tree that contains that blob will fail,\nsaying that the blob only seems to exist to us via submodule odb but\nnot in our repository.\n\n\n[Footnote]\n\n*1* which I do not necessarily agree with---I am in favor of getting\nrid of add_submodule_odb() to fix these issues at the root cause of\nthem.\n"},{"id":"208056","messageId":"CACsJy8BJZgyEn1n2GWgAVSGhSkVUO-P=GXwR02OcDf0ziTTRaA@mail.gmail.com","threadId":"32714","inReplyTo":"7vpq0t3x60.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 6/7] read-cache: refuse to create index referring to external objects","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-01-28T05:48:26Z","receivedAt":"2013-01-28T05:48:26Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Jan 26, 2013 at 2:00 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> This is not a tangent, but if you want to go this \"forbid making our\n> repository depend on objects we do not have but we know about after\n> we peek submodule odb\" route [*1*], write_sha1_file() needs to be\n> told about has_sha1_file_proper().  We may \"git add\" a new blob in\n> the superproject, the blob may not yet exist in *our* repository,\n> but may happen to already exist in the submodue odb.  In such a\n> case, write_sha1_file() has to write that blob in our repository,\n> without the existing has_sha1_file() check bypassing it.  Otherwise\n> our attempt to create a tree that contains that blob will fail,\n> saying that the blob only seems to exist to us via submodule odb but\n> not in our repository.\n\nAnother thing needs to be done for this to work. The current reading\norder is packs first, loose objects next. If we create a local loose\nduplicate of an alternate packed object, our local version will never\nbe read. Regardless the submodule odb issue, I think we should prefer\nreading local loose objects over alternate packed ones.\n-- \nDuy\n"},{"id":"208059","messageId":"7vy5fdyhs0.fsf@alter.siamese.dyndns.org","threadId":"32714","inReplyTo":"CACsJy8BJZgyEn1n2GWgAVSGhSkVUO-P=GXwR02OcDf0ziTTRaA@mail.gmail.com","subject":"Re: [PATCH 6/7] read-cache: refuse to create index referring to external objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-28T05:54:07Z","receivedAt":"2013-01-28T05:54:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen <pclouds@gmail.com> writes:\n\n> On Sat, Jan 26, 2013 at 2:00 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>> This is not a tangent, but if you want to go this \"forbid making our\n>> repository depend on objects we do not have but we know about after\n>> we peek submodule odb\" route [*1*], write_sha1_file() needs to be\n>> told about has_sha1_file_proper().  We may \"git add\" a new blob in\n>> the superproject, the blob may not yet exist in *our* repository,\n>> but may happen to already exist in the submodue odb.  In such a\n>> case, write_sha1_file() has to write that blob in our repository,\n>> without the existing has_sha1_file() check bypassing it.  Otherwise\n>> our attempt to create a tree that contains that blob will fail,\n>> saying that the blob only seems to exist to us via submodule odb but\n>> not in our repository.\n>\n> Another thing needs to be done for this to work. The current reading\n\nFor *what* to work???  I think the performance consideration is the\nonly thing that should drive the read-order; correctness should not\nbe affected.\n\n> order is packs first, loose objects next. If we create a local loose\n> duplicate of an alternate packed object, our local version will never\n> be read. Regardless the submodule odb issue, I think we should prefer\n> reading local loose objects over alternate packed ones.\n"},{"id":"208060","messageId":"CACsJy8CXC=poDBenBwo6t8=Qv-_zvGbvHYo-cDdGA8_fpw4Cyg@mail.gmail.com","threadId":"32714","inReplyTo":"7vy5fdyhs0.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 6/7] read-cache: refuse to create index referring to external objects","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-01-28T05:57:15Z","receivedAt":"2013-01-28T05:57:15Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Jan 28, 2013 at 12:54 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Duy Nguyen <pclouds@gmail.com> writes:\n>\n>> On Sat, Jan 26, 2013 at 2:00 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>>> This is not a tangent, but if you want to go this \"forbid making our\n>>> repository depend on objects we do not have but we know about after\n>>> we peek submodule odb\" route [*1*], write_sha1_file() needs to be\n>>> told about has_sha1_file_proper().  We may \"git add\" a new blob in\n>>> the superproject, the blob may not yet exist in *our* repository,\n>>> but may happen to already exist in the submodue odb.  In such a\n>>> case, write_sha1_file() has to write that blob in our repository,\n>>> without the existing has_sha1_file() check bypassing it.  Otherwise\n>>> our attempt to create a tree that contains that blob will fail,\n>>> saying that the blob only seems to exist to us via submodule odb but\n>>> not in our repository.\n>>\n>> Another thing needs to be done for this to work. The current reading\n>\n> For *what* to work???\n\nThe \"forbid making our repository depend on objects we do not have but\nwe know about afterwe peek submodule odb\"\n\n> I think the performance consideration is the\n> only thing that should drive the read-order; correctness should not\n> be affected.\n>\n>> order is packs first, loose objects next. If we create a local loose\n>> duplicate of an alternate packed object, our local version will never\n>> be read. Regardless the submodule odb issue, I think we should prefer\n>> reading local loose objects over alternate packed ones.\n-- \nDuy\n"},{"id":"208061","messageId":"7vtxq1yh6t.fsf@alter.siamese.dyndns.org","threadId":"32714","inReplyTo":"CACsJy8CXC=poDBenBwo6t8=Qv-_zvGbvHYo-cDdGA8_fpw4Cyg@mail.gmail.com","subject":"Re: [PATCH 6/7] read-cache: refuse to create index referring to external objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-28T06:06:50Z","receivedAt":"2013-01-28T06:06:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen <pclouds@gmail.com> writes:\n\n>>> Another thing needs to be done for this to work. The current reading\n>>\n>> For *what* to work???\n>\n> The \"forbid making our repository depend on objects we do not have but\n> we know about afterwe peek submodule odb\"\n\nWith your \"when our object database is contaminated, check objects\nwe base our new object on are available in local or our alternates\"\ntogether with the \"when we try write_object(), do not bypass it with\nhas_sha1_file() check because that may find the object in submodule odb\nwe should *not* have access to; instead check with the same 'local\nor our alternates' test\" I brought up in the message you were\nresponding to, I do not think object read order does not make a\ndifference to our effort to prevent the object database breakage due\nto temporary contamination by submodule objects.\n\n>>> Regardless the submodule odb issue, I think we should prefer\n>>> reading local loose objects over alternate packed ones.\n\nI suspect you are alluding to make write_object() check with\nhas_sha1_file_locally() so that we can wean our repository from\nexisting alternates, but I do not think it is a right approach\n(instead, you just fully repack locally if you want to dissociate\nyourself from your alternates).  What I was suggesting was to change\nit to check with has_sha1_file_proper(), to make sure we do not omit\nwriting an object we need to access to, when we know it will vanish\nonce we stop temporarily borrowing from the submodule object store.\n"},{"id":"208063","messageId":"CACsJy8BwCCAZyMZ2w9fyMaNJsHRNp2V3Aen8g3drAkZ4y9mfBg@mail.gmail.com","threadId":"32714","inReplyTo":"CACsJy8BJZgyEn1n2GWgAVSGhSkVUO-P=GXwR02OcDf0ziTTRaA@mail.gmail.com","subject":"Re: [PATCH 6/7] read-cache: refuse to create index referring to external objects","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-01-28T06:18:12Z","receivedAt":"2013-01-28T06:18:12Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Jan 28, 2013 at 12:48 PM, Duy Nguyen <pclouds@gmail.com> wrote:\n> Regardless the submodule odb issue, I think we should prefer\n> reading local loose objects over alternate packed ones.\n\nI think I went from one problem to another and did not make it clear.\nThe reason behind this preference is security. With \"all packs first\"\nreading order, someone can create a pack in the alternate source and\nthat pack will override the same local loose objects (local packed\nones are safe). If someone can create a malicious version with the\nsame SHA-1, we (well, the user) are in trouble. If the user uses this\nrepository directly then the malicious object can be used without\ndetected, even if it does not match the original SHA-1, as we do not\nalways verify content against its SHA-1 for common commands.\n-- \nDuy\n"},{"id":"208064","messageId":"7vpq0pyfsq.fsf@alter.siamese.dyndns.org","threadId":"32714","inReplyTo":"CACsJy8BwCCAZyMZ2w9fyMaNJsHRNp2V3Aen8g3drAkZ4y9mfBg@mail.gmail.com","subject":"Re: [PATCH 6/7] read-cache: refuse to create index referring to external objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-28T06:36:53Z","receivedAt":"2013-01-28T06:36:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen <pclouds@gmail.com> writes:\n\n> On Mon, Jan 28, 2013 at 12:48 PM, Duy Nguyen <pclouds@gmail.com> wrote:\n>> Regardless the submodule odb issue, I think we should prefer\n>> reading local loose objects over alternate packed ones.\n>\n> I think I went from one problem to another and did not make it clear.\n> The reason behind this preference is security.\n\nReading from ours first would not help you at all if you are lacking\nsome that you do need from your local repository and the only\nsolution is not to borrow from untrustworthy sources, I think.\n\nYou borrow only from a trusted source in the first place, no?\n"},{"id":"208066","messageId":"CACsJy8BUhSDEdV3MtoBG1=Xx1OmiYvkvZQffV6cz1_HSAkcf1A@mail.gmail.com","threadId":"32714","inReplyTo":"7vpq0pyfsq.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 6/7] read-cache: refuse to create index referring to external objects","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-01-28T06:57:57Z","receivedAt":"2013-01-28T06:57:57Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Jan 28, 2013 at 1:36 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Duy Nguyen <pclouds@gmail.com> writes:\n>\n>> On Mon, Jan 28, 2013 at 12:48 PM, Duy Nguyen <pclouds@gmail.com> wrote:\n>>> Regardless the submodule odb issue, I think we should prefer\n>>> reading local loose objects over alternate packed ones.\n>>\n>> I think I went from one problem to another and did not make it clear.\n>> The reason behind this preference is security.\n>\n> Reading from ours first would not help you at all if you are lacking\n> some that you do need from your local repository and the only\n> solution is not to borrow from untrustworthy sources, I think.\n>\n> You borrow only from a trusted source in the first place, no?\n\nHmm.. yeah.\n-- \nDuy\n"}]}