threads / patch / 15828

patchbuiltin-merge.c: fix memory under-allocation

Subject: [PATCH maint] builtin-merge.c: fix memory under-allocation

## tl;dr

7 messages between Oct 9, 2008 and Oct 9, 2008. Diffs are folded; open one to read it.

replies: 6people: 4as markdown or json

Brandon Casey· Oct 9, 2008, 00:07 UTC · lore

While we're at it, change the allocation to reference the variable it is allocating memory for to try to prevent a similar mistake if the type is changed in the future.

Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---

This causes segfault on IRIX64. I guess the irix compiler doesn't over-allocate memory as aggressively as gcc.

-brandon
 builtin-merge.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
Show changes to builtin-merge.c +2 −2
diff --git a/builtin-merge.c b/builtin-merge.c
index dcaf368..d0bf1fc 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -651,12 +651,12 @@ static void add_strategies(const char *string, unsigned attr)
 static int merge_trivial(void)
 {
 	unsigned char result_tree[20], result_commit[20];
-	struct commit_list *parent = xmalloc(sizeof(struct commit_list *));
+	struct commit_list *parent = xmalloc(sizeof(*parent));
 
 	write_tree_trivial(result_tree);
 	printf("Wonderful.\n");
 	parent->item = lookup_commit(head);
-	parent->next = xmalloc(sizeof(struct commit_list *));
+	parent->next = xmalloc(sizeof(*parent->next));
 	parent->next->item = remoteheads->item;
 	parent->next->next = NULL;
 	commit_tree(merge_msg.buf, result_tree, parent, result_commit);

-- 
1.6.0.2.446.ge243d
Miklos Vajna· Oct 9, 2008, 00:17 UTC · re: Brandon Casey · lore

Re: [PATCH maint] builtin-merge.c: fix memory under-allocation

On Wed, Oct 08, 2008 at 07:07:54PM -0500, Brandon Casey <casey@nrlssc.navy.mil> wrote:
> While we're at it, change the allocation to reference the variable it is
> allocating memory for to try to prevent a similar mistake if the type is
> changed in the future.

If this is really a problem, then I think it would be good to mention this in Documentation/CodingGuidelines.

Brandon Casey· Oct 9, 2008, 00:27 UTC · re: Miklos Vajna · lore

Re: [PATCH maint] builtin-merge.c: fix memory under-allocation

Miklos Vajna wrote:
Show 7 quoted lines
> On Wed, Oct 08, 2008 at 07:07:54PM -0500, Brandon Casey <casey@nrlssc.navy.mil> wrote:
>> While we're at it, change the allocation to reference the variable it is
>> allocating memory for to try to prevent a similar mistake if the type is
>> changed in the future.
> 
> If this is really a problem, then I think it would be good to mention
> this in Documentation/CodingGuidelines.

That's fine. Though I didn't mean to imply that the memory under-allocation was caused by a change in variable type in this case. Re-reading my commit message, maybe it sounds like that.

  Something like this sometimes happens:
-    struct a_struct *foo = xmalloc(sizeof(struct a_struct));
+    struct a_bigr_struct *foo = xmalloc(sizeof(struct a_struct));
  which would be avoided if we had started with:
     struct a_struct *foo = xmalloc(sizeof(*foo));
-brandon
Junio C Hamano· Oct 9, 2008, 11:12 UTC · re: Brandon Casey · lore

Re: [PATCH maint] builtin-merge.c: fix memory under-allocation

Brandon Casey <casey@nrlssc.navy.mil> writes:
Show 12 quoted lines
> Miklos Vajna wrote:
>> On Wed, Oct 08, 2008 at 07:07:54PM -0500, Brandon Casey <casey@nrlssc.navy.mil> wrote:
>>> While we're at it, change the allocation to reference the variable it is
>>> allocating memory for to try to prevent a similar mistake if the type is
>>> changed in the future.
>> 
>> If this is really a problem, then I think it would be good to mention
>> this in Documentation/CodingGuidelines.
>
> That's fine. Though I didn't mean to imply that the memory under-allocation
> was caused by a change in variable type in this case. Re-reading my commit
> message, maybe it sounds like that.

Yeah, it does. I was scratching my head and had to read the patch three times until I got it (yes, I am especially slower than usual today, as the reason I am reading mails right now is because I am jetlagged and cannot sleep).

Brandon Casey· Oct 9, 2008, 14:15 UTC · re: Junio C Hamano · lore

Re: [PATCH maint] builtin-merge.c: fix memory under-allocation

Junio C Hamano wrote:
Show 9 quoted lines
> Brandon Casey <casey@nrlssc.navy.mil> writes:
>> I didn't mean to imply that the memory under-allocation
>> was caused by a change in variable type in this case. Re-reading my commit
>> message, maybe it sounds like that.
> 
> Yeah, it does.  I was scratching my head and had to read the patch three
> times until I got it (yes, I am especially slower than usual today, as the
> reason I am reading mails right now is because I am jetlagged and cannot
> sleep).
If it's not too late, maybe this would make a better commit message:

--->8--- builtin-merge.c: allocate correct amount of memory

Fix two memory allocation errors which allocate space for a pointer rather than enough space for the structure itself.

This:
    struct commit_list *parent = xmalloc(sizeof(struct commit_list *));
should have been this:
    struct commit_list *parent = xmalloc(sizeof(struct commit_list));
    
But while we're at it, change the allocation to reference the variable it is
allocating memory for to try to prevent a similar mistake, for example if the
type is changed, in the future.
Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
Miklos Vajna· Oct 9, 2008, 14:49 UTC · re: Brandon Casey · lore

Re: [PATCH maint] builtin-merge.c: fix memory under-allocation

On Thu, Oct 09, 2008 at 09:15:02AM -0500, Brandon Casey <casey@nrlssc.navy.mil> wrote:
> Fix two memory allocation errors which allocate space for a pointer rather
> than enough space for the structure itself.

Aah, I see it now. I thought first that it was just a variable->type change.

Acked-by: Miklos Vajna <vmiklos@frugalware.org>
Shawn O. Pearce· Oct 9, 2008, 15:13 UTC · re: Brandon Casey · lore

Re: [PATCH maint] builtin-merge.c: fix memory under-allocation

Brandon Casey <casey@nrlssc.navy.mil> wrote:
Show 12 quoted lines
> Junio C Hamano wrote:
> > Brandon Casey <casey@nrlssc.navy.mil> writes:
> >> I didn't mean to imply that the memory under-allocation
> >> was caused by a change in variable type in this case. Re-reading my commit
> >> message, maybe it sounds like that.
> > 
> > Yeah, it does.  I was scratching my head and had to read the patch three
> > times until I got it (yes, I am especially slower than usual today, as the
> > reason I am reading mails right now is because I am jetlagged and cannot
> > sleep).
> 
> If it's not too late, maybe this would make a better commit message:

Nope, I didn't get around to the patch until now. Better message is being used... ;-)

Thanks everyone.
Show 19 quoted lines
> --->8---
> builtin-merge.c: allocate correct amount of memory
> 
> Fix two memory allocation errors which allocate space for a pointer rather
> than enough space for the structure itself.
> 
> This:
> 
>     struct commit_list *parent = xmalloc(sizeof(struct commit_list *));
> 
> should have been this:
> 
>     struct commit_list *parent = xmalloc(sizeof(struct commit_list));
>     
> But while we're at it, change the allocation to reference the variable it is
> allocating memory for to try to prevent a similar mistake, for example if the
> type is changed, in the future.
> 
> Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
-- 
Shawn.

← back to recent threads