{"thread":{"id":"32702","subject":"[PATCH/RFC] Revoke write access to refs and odb after importing another repo's odb","startedAt":"2013-01-23T13:34:27Z","lastAt":"2013-01-24T05:58:16Z","messageCount":7,"participants":["Nguyễn Thái Ngọc Duy","Duy Nguyen","Junio C Hamano","Jens Lehmann"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"207590","messageId":"1358948067-2792-1-git-send-email-pclouds@gmail.com","threadId":"32702","inReplyTo":null,"subject":"[PATCH/RFC] Revoke write access to refs and odb after importing another repo's odb","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-01-23T13:34:27Z","receivedAt":"2013-01-23T13:34:27Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"add_submodule_odb() can be used to import objects from another\nrepository temporarily. After this point we don't know which objects\nare ours, which are external. If we create an object that refers to an\nexternal object, next time git runs, it may find a hole in the object\ngraph because the external repository may not be imported. The same\ngoes for pointing a ref to an external SHA-1.\n\nTo protect ourselves, once add_submodule_odb() is used:\n\n - trees, tags and commits cannot be created\n - refs cannot be updated\n\nIn certain cases that submodule code knows that it's safe to write, it\ncan turn the readonly flag off.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n I think this is a good safety check. It catches at least a case in\n t7405.3. I did not investigate further though.\n\n cache.h | 1 +\n refs.c | 2 ++\n sha1_file.c | 2 ++\n submodule.c | 7 +++++++\n 5 files changed, 16 insertions(+)\n\ndiff --git a/cache.h b/cache.h\nindex c257953..772d229 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -753,6 +753,7 @@ extern int force_object_loose(const unsigned char *sha1, time_t mtime);\n extern void *map_sha1_file(const unsigned char *sha1, unsigned long *size);\n extern int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long mapsize, void *buffer, unsigned long bufsiz);\n extern int parse_sha1_header(const char *hdr, unsigned long *sizep);\n+extern int git_repo_readonly();\n \n /* global flag to enable extra checks when accessing packed objects */\n extern int do_check_packed_object_crc;\ndiff --git a/refs.c b/refs.c\nindex 541fec2..22b13f4 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1711,6 +1711,8 @@ struct ref_lock *lock_ref_sha1(const char *refname, const unsigned char *old_sha\n struct ref_lock *lock_any_ref_for_update(const char *refname,\n \t\t\t\t\t const unsigned char *old_sha1, int flags)\n {\n+\tif (git_repo_readonly())\n+\t\tdie(\"repository in read-only mode, cannot update refs\");\n \tif (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))\n \t\treturn NULL;\n \treturn lock_ref_sha1_basic(refname, old_sha1, flags, NULL);\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 40b2329..b9e8b59 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2575,6 +2575,8 @@ int write_sha1_file(const void *buf, unsigned long len, const char *type, unsign\n \tchar hdr[32];\n \tint hdrlen;\n \n+\tif (git_repo_readonly() && strcmp(type, \"blob\"))\n+\t\tdie(\"repository in read-only mode, cannot update object database\");\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 */\ndiff --git a/submodule.c b/submodule.c\nindex 2f55436..5eba597 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -19,6 +19,7 @@ static struct string_list changed_submodule_paths;\n static int initialized_fetch_ref_tips;\n static struct sha1_array ref_tips_before_fetch;\n static struct sha1_array ref_tips_after_fetch;\n+static int readonly;\n \n /*\n * The following flag is set if the .gitmodules file is unmerged. We then\n@@ -30,6 +31,11 @@ static struct sha1_array ref_tips_after_fetch;\n */\n static int gitmodules_is_unmerged;\n \n+int git_repo_readonly()\n+{\n+\treturn readonly;\n+}\n+\n static int add_submodule_odb(const char *path)\n {\n \tstruct strbuf objects_directory = STRBUF_INIT;\n@@ -67,6 +73,7 @@ static int add_submodule_odb(const char *path)\n \t/* add possible alternates from the submodule */\n \tread_info_alternates(objects_directory.buf, 0);\n \tprepare_alt_odb();\n+\treadonly = 1;\n done:\n \tstrbuf_release(&objects_directory);\n \treturn ret;\n-- \n1.8.0.rc2.23.g1fb49df\n"},{"id":"207591","messageId":"CACsJy8DSQMQ_mLxTXDL3oHuV9DdRv9MFH0-h=KzaBEhXtkLpZQ@mail.gmail.com","threadId":"32702","inReplyTo":"1358948067-2792-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH/RFC] Revoke write access to refs and odb after importing another repo's odb","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-01-23T13:38:08Z","receivedAt":"2013-01-23T13:38:08Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Jan 23, 2013 at 8:34 PM, Nguyễn Thái Ngọc Duy wrote:\n> add_submodule_odb() can be used to import objects from another\n> repository temporarily. After this point we don't know which objects\n> are ours, which are external. If we create an object that refers to an\n> external object, next time git runs, it may find a hole in the object\n> graph because the external repository may not be imported. The same\n> goes for pointing a ref to an external SHA-1.\n>\n> To protect ourselves, once add_submodule_odb() is used:\n>\n> - trees, tags and commits cannot be created\n> - refs cannot be updated\n\n.. and soon after I pressed send, I realized I missed at least two\nother places write should not be allowed:\n\n - index\n - reflog\n\nBut the general idea is probably more important than implementation\ndetails at this stage.\n-- \nDuy\n"},{"id":"207608","messageId":"7v1udbj0kt.fsf@alter.siamese.dyndns.org","threadId":"32702","inReplyTo":"1358948067-2792-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH/RFC] Revoke write access to refs and odb after importing another repo's odb","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-23T17:01:06Z","receivedAt":"2013-01-23T17:01:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> add_submodule_odb() can be used to import objects from another\n> repository temporarily. After this point we don't know which objects\n> are ours, which are external. If we create an object that refers to an\n> external object, next time git runs, it may find a hole in the object\n> graph because the external repository may not be imported. The same\n> goes for pointing a ref to an external SHA-1.\n>\n> To protect ourselves, once add_submodule_odb() is used:\n>\n> - trees, tags and commits cannot be created\n> - refs cannot be updated\n>\n> In certain cases that submodule code knows that it's safe to write, it\n> can turn the readonly flag off.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> I think this is a good safety check.\n\nTwo step implementation to bring \"read-only\" support into a testable\nshape and then flip that bit in add_submdule_odb() would be a\nsensible approach.\n\nI however have this suspicion that this will become a losing battle\nand we would be better off getting rid of add_submodule_odb();\ninstead operations that work across repositories will be done as a\nsubprocess, which will get us back closer to one of the original\ndesign goals of submodule support to have a clear separation between\nthe superproject and its submodules.\n"},{"id":"207633","messageId":"51004A37.6040301@web.de","threadId":"32702","inReplyTo":"7v1udbj0kt.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH/RFC] Revoke write access to refs and odb after importing another repo's odb","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2013-01-23T20:38:15Z","receivedAt":"2013-01-23T20:38:15Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 23.01.2013 18:01, schrieb Junio C Hamano:\n> Nguyễn Thái Ngọc Duy writes:\n> \n>> add_submodule_odb() can be used to import objects from another\n>> repository temporarily. After this point we don't know which objects\n>> are ours, which are external. If we create an object that refers to an\n>> external object, next time git runs, it may find a hole in the object\n>> graph because the external repository may not be imported. The same\n>> goes for pointing a ref to an external SHA-1.\n>>\n>> To protect ourselves, once add_submodule_odb() is used:\n>>\n>> - trees, tags and commits cannot be created\n>> - refs cannot be updated\n>>\n>> In certain cases that submodule code knows that it's safe to write, it\n>> can turn the readonly flag off.\n>>\n>> Signed-off-by: Nguyễn Thái Ngọc Duy \n>> ---\n>> I think this is a good safety check.\n> \n> Two step implementation to bring \"read-only\" support into a testable\n> shape and then flip that bit in add_submdule_odb() would be a\n> sensible approach.\n\nI agree this is a worthwhile change so nobody accidentally screws\nthings up.\n\n>> It catches at least a case in\n>> t7405.3. I did not investigate further though.\n\nThis is a false positive. The merge algorithm picked a fast-forward\nin a submodule as a proper merge result and records that in a\ngitlink. But as Duy pointed out this could be easily fixed by\nturning the readonly flag off in that case.\n\n> I however have this suspicion that this will become a losing battle\n> and we would be better off getting rid of add_submodule_odb();\n> instead operations that work across repositories will be done as a\n> subprocess, which will get us back closer to one of the original\n> design goals of submodule support to have a clear separation between\n> the superproject and its submodules.\n\nPlease don't. While I agree with your goal, I'd be unhappy to do\nthat because of the performance drop (especially on fork-challenged\noperating systems).\n"},{"id":"207637","messageId":"7vwqv3fw2b.fsf@alter.siamese.dyndns.org","threadId":"32702","inReplyTo":"51004A37.6040301@web.de","subject":"Re: [PATCH/RFC] Revoke write access to refs and odb after importing another repo's odb","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-23T21:06:52Z","receivedAt":"2013-01-23T21:06:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jens Lehmann writes:\n\n> This is a false positive. The merge algorithm picked a fast-forward\n> in a submodule as a proper merge result and records that in a\n> gitlink. But as Duy pointed out this could be easily fixed by\n> turning the readonly flag off in that case.\n\nI see that as \"easily circumvented and not an effective protection\",\nthough.\n\nIn theory, adding a gitlink to the index, removing a gitlink to the\nindex and modifying an existing gitlink in the index to another\ngitlink in the index and writing the resulting in-core index out to\nthe on-disk index should be allowed, even after objects from the\nsubmodule object database have contaminated our in-core object pool,\nas long as you do not run cache_tree_update(). I am not sure if that\nsingle loophole would be sufficient, though.\n"},{"id":"207655","messageId":"CACsJy8A1EVJbUsqyqNueEvhvd61VHfuiPL3VXBuAPs2FUw=_dg@mail.gmail.com","threadId":"32702","inReplyTo":"7v1udbj0kt.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH/RFC] Revoke write access to refs and odb after importing another repo's odb","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-01-24T01:30:29Z","receivedAt":"2013-01-24T01:30:29Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Jan 24, 2013 at 12:01 AM, Junio C Hamano wrote:\n> I however have this suspicion that this will become a losing battle\n> and we would be better off getting rid of add_submodule_odb();\n> instead operations that work across repositories will be done as a\n> subprocess, which will get us back closer to one of the original\n> design goals of submodule support to have a clear separation between\n> the superproject and its submodules.\n\nIt does not have to be subprocess. Thomas Rast did some work on\nsupport multithread access to object db by basically replicating all\ndatastructure per thread. If that work is complete, we have something\nlike \"odb container\" that could be used to access objects from another\nrepository and it won't contaminate the original odb. The same thing\ncan be done for ref and index access.\n-- \nDuy\n"},{"id":"207665","messageId":"CACsJy8B-mCBFF0YUCU5BRCRhHJjZDfQY9+9ETBrJzLOe=19NXA@mail.gmail.com","threadId":"32702","inReplyTo":"7vwqv3fw2b.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH/RFC] Revoke write access to refs and odb after importing another repo's odb","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-01-24T05:58:16Z","receivedAt":"2013-01-24T05:58:16Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Jan 24, 2013 at 4:06 AM, Junio C Hamano wrote:\n> Jens Lehmann writes:\n>\n>> This is a false positive. The merge algorithm picked a fast-forward\n>> in a submodule as a proper merge result and records that in a\n>> gitlink. But as Duy pointed out this could be easily fixed by\n>> turning the readonly flag off in that case.\n>\n> I see that as \"easily circumvented and not an effective protection\",\n> though.\n>\n> In theory, adding a gitlink to the index, removing a gitlink to the\n> index and modifying an existing gitlink in the index to another\n> gitlink in the index and writing the resulting in-core index out to\n> the on-disk index should be allowed, even after objects from the\n> submodule object database have contaminated our in-core object pool,\n> as long as you do not run cache_tree_update(). I am not sure if that\n> single loophole would be sufficient, though.\n\nThe problem is we don't know which entries are updated in index. We\ndon't keep track of them. And I think in the unpack-trees case, we\nscape the whole index then copy over, making it look like the whole\nindex is updated (even with the same content). One way to check this\nis verify the source of all non-gitlink entries in index before\nwriting to disk (only when readonly flag is on, of course).\nsha1_object_info_extended() should help (or be extended to do the\njob). Hmm.. if we do this, we could also verify if new sha-1 objects\ndo not refer to an external source, if so allow them to be created.\n-- \nDuy\n"}]}