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

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

[Brandon Casey]

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(-)

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

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

[Miklos Vajna]

[-- Attachment #1: Type: text/plain, Size: 373 bytes --]

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.

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

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

[Brandon Casey]

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.

  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

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

[Junio C Hamano]

Brandon Casey <casey@nrlssc.navy.mil> writes:

> 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).

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

[Brandon Casey]

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:

--->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>

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

[Miklos Vajna]

[-- Attachment #1: Type: text/plain, Size: 337 bytes --]

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>

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

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

[Shawn O. Pearce]

Brandon Casey <casey@nrlssc.navy.mil> wrote:
> 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.
 
> --->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.