{"thread":{"id":"14131","subject":"[PATCH] factorize pack structure allocation","startedAt":"2008-06-24T22:58:06Z","lastAt":"2008-06-26T06:40:17Z","messageCount":5,"participants":["Nicolas Pitre","Jon Loeliger","Junio C Hamano","Teemu Likonen","Andreas Ericsson"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"81013","messageId":"alpine.LFD.1.10.0806241851420.2979@xanadu.home","threadId":"14131","inReplyTo":null,"subject":"[PATCH] factorize pack structure allocation","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-06-24T22:58:06Z","receivedAt":"2008-06-24T22:58:06Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"New pack structures are currently allocated in 2 different places\nand all members have to be initialized explicitly. This is prone\nto errors leading to segmentation faults as found by Teemu Likonen.\n\nLet's have a common place where this structure is allocated, and have \nall members implicitly initialized to zero.\n\nSigned-off-by: Nicolas Pitre \n---\ndiff --git a/sha1_file.c b/sha1_file.c\nindex a92f023..c56f674 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -792,18 +792,28 @@ unsigned char* use_pack(struct packed_git *p,\n \treturn win->base + offset;\n }\n \n+static struct packed_git *alloc_packed_git(int extra)\n+{\n+\tstruct packed_git *p = xmalloc(sizeof(*p) + extra);\n+\tmemset(p, 0, sizeof(*p));\n+\tp->pack_fd = -1;\n+\treturn p;\n+}\n+\n struct packed_git *add_packed_git(const char *path, int path_len, int local)\n {\n \tstruct stat st;\n-\tstruct packed_git *p = xmalloc(sizeof(*p) + path_len + 2);\n+\tstruct packed_git *p = alloc_packed_git(path_len + 2);\n \n \t/*\n \t * Make sure a corresponding .pack file exists and that\n \t * the index looks sane.\n \t */\n \tpath_len -= strlen(\".idx\");\n-\tif (path_len < 1)\n+\tif (path_len < 1) {\n+\t\tfree(p);\n \t\treturn NULL;\n+\t}\n \tmemcpy(p->pack_name, path, path_len);\n \tstrcpy(p->pack_name + path_len, \".pack\");\n \tif (stat(p->pack_name, &st) || !S_ISREG(st.st_mode)) {\n@@ -814,16 +824,7 @@ struct packed_git *add_packed_git(const char *path, int path_len, int local)\n \t/* ok, it looks sane as far as we can check without\n \t * actually mapping the pack file.\n \t */\n-\tp->index_version = 0;\n-\tp->index_data = NULL;\n-\tp->index_size = 0;\n-\tp->num_objects = 0;\n-\tp->num_bad_objects = 0;\n-\tp->bad_object_sha1 = NULL;\n \tp->pack_size = st.st_size;\n-\tp->next = NULL;\n-\tp->windows = NULL;\n-\tp->pack_fd = -1;\n \tp->pack_local = local;\n \tp->mtime = st.st_mtime;\n \tif (path_len < 40 || get_sha1_hex(path + path_len - 40, p->sha1))\n@@ -835,19 +836,15 @@ struct packed_git *parse_pack_index(unsigned char *sha1)\n {\n \tconst char *idx_path = sha1_pack_index_name(sha1);\n \tconst char *path = sha1_pack_name(sha1);\n-\tstruct packed_git *p = xmalloc(sizeof(*p) + strlen(path) + 2);\n+\tstruct packed_git *p = alloc_packed_git(strlen(path) + 1);\n \n+\tstrcpy(p->pack_name, path);\n+\thashcpy(p->sha1, sha1);\n \tif (check_packed_git_idx(idx_path, p)) {\n \t\tfree(p);\n \t\treturn NULL;\n \t}\n \n-\tstrcpy(p->pack_name, path);\n-\tp->pack_size = 0;\n-\tp->next = NULL;\n-\tp->windows = NULL;\n-\tp->pack_fd = -1;\n-\thashcpy(p->sha1, sha1);\n \treturn p;\n }\n \n"},{"id":"81015","messageId":"48617F7E.4020706@freescale.com","threadId":"14131","inReplyTo":"alpine.LFD.1.10.0806241851420.2979@xanadu.home","subject":"Re: [PATCH] factorize pack structure allocation","fromName":"Jon Loeliger","fromEmail":"jdl@freescale.com","sentAt":"2008-06-24T23:13:02Z","receivedAt":"2008-06-24T23:13:02Z","isPatch":true,"sender":{"key":"jdl@jdl.com","avatar":"https://gravatar.com/avatar/75ce9a10b151acd2c28ec4ab2136dba7b2ff1634530bd04b155981a749d08a64?d=mp&s=160"},"body":"Nicolas Pitre wrote:\n> New pack structures are currently allocated in 2 different places\n> and all members have to be initialized explicitly. This is prone\n> to errors leading to segmentation faults as found by Teemu Likonen.\n> \n> Let's have a common place where this structure is allocated, and have \n> all members implicitly initialized to zero.\n> \n> Signed-off-by: Nicolas Pitre \n> ---\n> diff --git a/sha1_file.c b/sha1_file.c\n> index a92f023..c56f674 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -792,18 +792,28 @@ unsigned char* use_pack(struct packed_git *p,\n> \treturn win->base + offset;\n> }\n> \n> +static struct packed_git *alloc_packed_git(int extra)\n> +{\n> +\tstruct packed_git *p = xmalloc(sizeof(*p) + extra);\n> +\tmemset(p, 0, sizeof(*p));\n> +\tp->pack_fd = -1;\n> +\treturn p;\n> +}\n\nNit: That's an explicit 0 initialization!\n\njdl\n"},{"id":"81045","messageId":"7vtzfi6vwj.fsf@gitster.siamese.dyndns.org","threadId":"14131","inReplyTo":"alpine.LFD.1.10.0806241851420.2979@xanadu.home","subject":"Re: [PATCH] factorize pack structure allocation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-06-25T03:22:36Z","receivedAt":"2008-06-25T03:22:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Pitre writes:\n\n> New pack structures are currently allocated in 2 different places\n> and all members have to be initialized explicitly. This is prone\n> to errors leading to segmentation faults as found by Teemu Likonen.\n\nThanks. This is a much better equivalent to the \"probably fixed with\nthis\" patch you sent earlier ;-)\n"},{"id":"81087","messageId":"20080625071920.GA3307@mithlond.arda.local","threadId":"14131","inReplyTo":"alpine.LFD.1.10.0806241851420.2979@xanadu.home","subject":"Re: [PATCH] factorize pack structure allocation","fromName":"Teemu Likonen","fromEmail":"tlikonen@iki.fi","sentAt":"2008-06-25T07:19:20Z","receivedAt":"2008-06-25T07:19:20Z","isPatch":true,"sender":{"key":"tlikonen@iki.fi","avatar":null},"body":"Nicolas Pitre wrote (2008-06-24 18:58 -0400):\n\n> New pack structures are currently allocated in 2 different places\n> and all members have to be initialized explicitly. This is prone\n> to errors leading to segmentation faults as found by Teemu Likonen.\n> \n> Let's have a common place where this structure is allocated, and have \n> all members implicitly initialized to zero.\n> \n> Signed-off-by: Nicolas Pitre \n\nBecause of time zone issues I didn't get a chance to check this until\nnow. This fixes the segfault issue for me. Thanks!\n"},{"id":"81244","messageId":"486339D1.7040706@op5.se","threadId":"14131","inReplyTo":"alpine.LFD.1.10.0806241851420.2979@xanadu.home","subject":"Re: [PATCH] factorize pack structure allocation","fromName":"Andreas Ericsson","fromEmail":"ae@op5.se","sentAt":"2008-06-26T06:40:17Z","receivedAt":"2008-06-26T06:40:17Z","isPatch":true,"sender":{"key":"ae@op5.se","avatar":"https://gravatar.com/avatar/426e89595c75a8f5252dd0c989e5fabe5bcac616e68557427ad9aef6b0ca342a?d=mp&s=160"},"body":"Nicolas Pitre wrote:\n> New pack structures are currently allocated in 2 different places\n> and all members have to be initialized explicitly. This is prone\n> to errors leading to segmentation faults as found by Teemu Likonen.\n> \n> Let's have a common place where this structure is allocated, and have \n> all members implicitly initialized to zero.\n> \n> Signed-off-by: Nicolas Pitre \n> ---\n> diff --git a/sha1_file.c b/sha1_file.c\n> index a92f023..c56f674 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -792,18 +792,28 @@ unsigned char* use_pack(struct packed_git *p,\n> \treturn win->base + offset;\n> }\n> \n> +static struct packed_git *alloc_packed_git(int extra)\n> +{\n> +\tstruct packed_git *p = xmalloc(sizeof(*p) + extra);\n> +\tmemset(p, 0, sizeof(*p));\n> +\tp->pack_fd = -1;\n> +\treturn p;\n> +}\n> +\n\nMinor nit; Use xcalloc() instead. It initializes the allocated area\nto zero by default, either by the glibc allocator when it re-uses old\nmemory, or by the kernel when it's handed to userspace. It's a\nmicro-optimization, but a worthwhile one imo, especially for repos\nwith lots and lots of packs (git gc --auto runs galore).\n\nThe \"calloc() returns nulified memory\" dogma conforms to C89 and is\nthus about as portable as it gets.\n\n-- \nAndreas Ericsson andreas.ericsson@op5.se\nOP5 AB www.op5.se\nTel: +46 8-230225 Fax: +46 8-230231\n"}]}