threads / patch / 14131

patchfactorize pack structure allocation

Subject: [PATCH] factorize pack structure allocation

## tl;dr

5 messages between Jun 24, 2008 and Jun 26, 2008. Diffs are folded; open one to read it.

replies: 4people: 5as markdown or json

Nicolas Pitre· Jun 24, 2008, 22:58 UTC · lore

New pack structures are currently allocated in 2 different places and all members have to be initialized explicitly. This is prone to errors leading to segmentation faults as found by Teemu Likonen.

Let's have a common place where this structure is allocated, and have all members implicitly initialized to zero.

Signed-off-by: Nicolas Pitre <nico@cam.org>
---
Show changes to sha1_file.c +15 −18
diff --git a/sha1_file.c b/sha1_file.c
index a92f023..c56f674 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -792,18 +792,28 @@ unsigned char* use_pack(struct packed_git *p,
 	return win->base + offset;
 }
 
+static struct packed_git *alloc_packed_git(int extra)
+{
+	struct packed_git *p = xmalloc(sizeof(*p) + extra);
+	memset(p, 0, sizeof(*p));
+	p->pack_fd = -1;
+	return p;
+}
+
 struct packed_git *add_packed_git(const char *path, int path_len, int local)
 {
 	struct stat st;
-	struct packed_git *p = xmalloc(sizeof(*p) + path_len + 2);
+	struct packed_git *p = alloc_packed_git(path_len + 2);
 
 	/*
 	 * Make sure a corresponding .pack file exists and that
 	 * the index looks sane.
 	 */
 	path_len -= strlen(".idx");
-	if (path_len < 1)
+	if (path_len < 1) {
+		free(p);
 		return NULL;
+	}
 	memcpy(p->pack_name, path, path_len);
 	strcpy(p->pack_name + path_len, ".pack");
 	if (stat(p->pack_name, &st) || !S_ISREG(st.st_mode)) {
@@ -814,16 +824,7 @@ struct packed_git *add_packed_git(const char *path, int path_len, int local)
 	/* ok, it looks sane as far as we can check without
 	 * actually mapping the pack file.
 	 */
-	p->index_version = 0;
-	p->index_data = NULL;
-	p->index_size = 0;
-	p->num_objects = 0;
-	p->num_bad_objects = 0;
-	p->bad_object_sha1 = NULL;
 	p->pack_size = st.st_size;
-	p->next = NULL;
-	p->windows = NULL;
-	p->pack_fd = -1;
 	p->pack_local = local;
 	p->mtime = st.st_mtime;
 	if (path_len < 40 || get_sha1_hex(path + path_len - 40, p->sha1))
@@ -835,19 +836,15 @@ struct packed_git *parse_pack_index(unsigned char *sha1)
 {
 	const char *idx_path = sha1_pack_index_name(sha1);
 	const char *path = sha1_pack_name(sha1);
-	struct packed_git *p = xmalloc(sizeof(*p) + strlen(path) + 2);
+	struct packed_git *p = alloc_packed_git(strlen(path) + 1);
 
+	strcpy(p->pack_name, path);
+	hashcpy(p->sha1, sha1);
 	if (check_packed_git_idx(idx_path, p)) {
 		free(p);
 		return NULL;
 	}
 
-	strcpy(p->pack_name, path);
-	p->pack_size = 0;
-	p->next = NULL;
-	p->windows = NULL;
-	p->pack_fd = -1;
-	hashcpy(p->sha1, sha1);
 	return p;
 }
Jon Loeliger· Jun 24, 2008, 23:13 UTC · re: Nicolas Pitre · lore

Re: [PATCH] factorize pack structure allocation

Nicolas Pitre wrote:
Show 24 quoted lines
> New pack structures are currently allocated in 2 different places
> and all members have to be initialized explicitly.  This is prone
> to errors leading to segmentation faults as found by Teemu Likonen.
> 
> Let's have a common place where this structure is allocated, and have 
> all members implicitly initialized to zero.
> 
> Signed-off-by: Nicolas Pitre <nico@cam.org>
> ---
> diff --git a/sha1_file.c b/sha1_file.c
> index a92f023..c56f674 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -792,18 +792,28 @@ unsigned char* use_pack(struct packed_git *p,
>  	return win->base + offset;
>  }
>  
> +static struct packed_git *alloc_packed_git(int extra)
> +{
> +	struct packed_git *p = xmalloc(sizeof(*p) + extra);
> +	memset(p, 0, sizeof(*p));
> +	p->pack_fd = -1;
> +	return p;
> +}
Nit:  That's an explicit 0 initialization!
jdl
Junio C Hamano· Jun 25, 2008, 03:22 UTC · re: Nicolas Pitre · lore

Re: [PATCH] factorize pack structure allocation

Nicolas Pitre <nico@cam.org> writes:
> New pack structures are currently allocated in 2 different places
> and all members have to be initialized explicitly.  This is prone
> to errors leading to segmentation faults as found by Teemu Likonen.

Thanks. This is a much better equivalent to the "probably fixed with this" patch you sent earlier ;-)

Teemu Likonen· Jun 25, 2008, 07:19 UTC · re: Nicolas Pitre · lore

Re: [PATCH] factorize pack structure allocation

Nicolas Pitre wrote (2008-06-24 18:58 -0400):
Show 8 quoted lines
> New pack structures are currently allocated in 2 different places
> and all members have to be initialized explicitly.  This is prone
> to errors leading to segmentation faults as found by Teemu Likonen.
> 
> Let's have a common place where this structure is allocated, and have 
> all members implicitly initialized to zero.
> 
> Signed-off-by: Nicolas Pitre <nico@cam.org>

Because of time zone issues I didn't get a chance to check this until now. This fixes the segfault issue for me. Thanks!

Andreas Ericsson· Jun 26, 2008, 06:40 UTC · re: Nicolas Pitre · lore

Re: [PATCH] factorize pack structure allocation

Nicolas Pitre wrote:
Show 25 quoted lines
> New pack structures are currently allocated in 2 different places
> and all members have to be initialized explicitly.  This is prone
> to errors leading to segmentation faults as found by Teemu Likonen.
> 
> Let's have a common place where this structure is allocated, and have 
> all members implicitly initialized to zero.
> 
> Signed-off-by: Nicolas Pitre <nico@cam.org>
> ---
> diff --git a/sha1_file.c b/sha1_file.c
> index a92f023..c56f674 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -792,18 +792,28 @@ unsigned char* use_pack(struct packed_git *p,
>  	return win->base + offset;
>  }
>  
> +static struct packed_git *alloc_packed_git(int extra)
> +{
> +	struct packed_git *p = xmalloc(sizeof(*p) + extra);
> +	memset(p, 0, sizeof(*p));
> +	p->pack_fd = -1;
> +	return p;
> +}
> +

Minor nit; Use xcalloc() instead. It initializes the allocated area to zero by default, either by the glibc allocator when it re-uses old memory, or by the kernel when it's handed to userspace. It's a micro-optimization, but a worthwhile one imo, especially for repos with lots and lots of packs (git gc --auto runs galore).

The "calloc() returns nulified memory" dogma conforms to C89 and is thus about as portable as it gets.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

← back to recent threads