{"thread":{"id":"1896","subject":"[PATCH] Make object creation in http fetch a bit safer.","startedAt":"2005-09-21T09:55:49Z","lastAt":"2005-09-21T15:55:09Z","messageCount":2,"participants":["Junio C Hamano","Daniel Barkalow"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"9028","messageId":"7vd5n2ycbe.fsf@assigned-by-dhcp.cox.net","threadId":"1896","inReplyTo":null,"subject":"[PATCH] Make object creation in http fetch a bit safer.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-09-21T09:55:49Z","receivedAt":"2005-09-21T09:55:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Unlike write_sha1_file() that tries to create the object file in a\ntemporary location and then move it to the final location, fetch_object\ncould have been interrupted in the middle, leaving a corrupt file.\n\nSigned-off-by: Junio C Hamano <junkio@cox.net>\n\n---\n\n I think the packfile side is safer -- the index is verified\n with the call to parse_pack_index() which eventually calls\n check_packed_git_idx() to validate it, and the pack file is\n checked when it is first used, just like any pack file we\n originally had.  Ideally, we should validate the pair after\n downloading with git-verify-pack, though.\n\n http-fetch.c |   29 ++++++++++++++++++++++++-----\n 1 files changed, 24 insertions(+), 5 deletions(-)\n\n09d920831edba17fe983269a9d0d85e592dd4bb3\ndiff --git a/http-fetch.c b/http-fetch.c\n--- a/http-fetch.c\n+++ b/http-fetch.c\n@@ -350,13 +350,18 @@ int fetch_object(struct alt_base *repo, \n \tchar *hex = sha1_to_hex(sha1);\n \tchar *filename = sha1_file_name(sha1);\n \tunsigned char real_sha1[20];\n+\tchar tmpfile[PATH_MAX];\n+\tint ret;\n \tchar *url;\n \tchar *posn;\n \n-\tlocal = open(filename, O_WRONLY | O_CREAT | O_EXCL, 0666);\n+\tsnprintf(tmpfile, sizeof(tmpfile), \"%s/obj_XXXXXX\",\n+\t\t get_object_directory());\n \n+\tlocal = mkstemp(tmpfile);\n \tif (local < 0)\n-\t\treturn error(\"Couldn't open local object %s\\n\", filename);\n+\t\treturn error(\"Couldn't create temporary file %s for %s: %s\\n\",\n+\t\t\t     tmpfile, filename, strerror(errno));\n \n \tmemset(&stream, 0, sizeof(stream));\n \n@@ -386,18 +391,32 @@ int fetch_object(struct alt_base *repo, \n \t\treturn -1;\n \t}\n \n+\tfchmod(local, 0444);\n \tclose(local);\n \tinflateEnd(&stream);\n \tSHA1_Final(real_sha1, &c);\n \tif (zret != Z_STREAM_END) {\n-\t\tunlink(filename);\n+\t\tunlink(tmpfile);\n \t\treturn error(\"File %s (%s) corrupt\\n\", hex, url);\n \t}\n \tif (memcmp(sha1, real_sha1, 20)) {\n-\t\tunlink(filename);\n+\t\tunlink(tmpfile);\n \t\treturn error(\"File %s has bad hash\\n\", hex);\n \t}\n-\t\n+\tret = link(tmpfile, filename);\n+\tif (ret < 0) {\n+\t\t/* Same Coda hack as in write_sha1_file(sha1_file.c) */\n+\t\tret = errno;\n+\t\tif (ret == EXDEV && !rename(tmpfile, filename))\n+\t\t\tgoto out;\n+\t}\n+\tunlink(tmpfile);\n+\tif (ret) {\n+\t\tif (ret != EEXIST)\n+\t\t\treturn error(\"unable to write sha1 filename %s: %s\",\n+\t\t\t\t     filename, strerror(ret));\n+\t}\n+ out:\n \tpull_say(\"got %s\\n\", hex);\n \treturn 0;\n }\n"},{"id":"9045","messageId":"Pine.LNX.4.63.0509211140230.23242@iabervon.org","threadId":"1896","inReplyTo":"7vd5n2ycbe.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Make object creation in http fetch a bit safer.","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2005-09-21T15:55:09Z","receivedAt":"2005-09-21T15:55:09Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Wed, 21 Sep 2005, Junio C Hamano wrote:\n\n> Unlike write_sha1_file() that tries to create the object file in a\n> temporary location and then move it to the final location, fetch_object\n> could have been interrupted in the middle, leaving a corrupt file.\n\nLooks fine to me, although it should possibly share the code somehow with \nwrite_sha1_file().\n\n>  I think the packfile side is safer -- the index is verified\n>  with the call to parse_pack_index() which eventually calls\n>  check_packed_git_idx() to validate it, and the pack file is\n>  checked when it is first used, just like any pack file we\n>  originally had.  Ideally, we should validate the pair after\n>  downloading with git-verify-pack, though.\n\nWell, with verify_pack(), anyway. No need to prepare a command line, fork, \nexec, and parse it, just to call a library function.\n\nI think we want to have the partial file in the destination location, and \nmaybe teach http-fetch about resuming the download, since it takes \nsignificant time to download a single pack, and it would be nice to be \nable to use the part we've got if we get interrupted.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"}]}