threads / patch / 35831

patchbuiltin/blame.c::prepare_lines: fix allocation size of sb->lineno

Subject: [PATCH] builtin/blame.c::prepare_lines: fix allocation size of sb->lineno

## tl;dr

4 messages between Feb 8, 2014 and Feb 8, 2014. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

David Kastrup· Feb 8, 2014, 09:19 UTC · lore

If we are calling xrealloc on every single line, the least we can do is get the right allocation size.

Signed-off-by: David Kastrup <dak@gnu.org>
---
This should be less contentious than the patch in
<URL:http://permalink.gmane.org/gmane.comp.version-control.git/241561>,
Message-ID: <1391550392-17118-1-git-send-email-dak@gnu.org> as it
makes no stylistic decisions whatsoever and only fixes a clear bug.

builtin/blame.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Show changes to builtin/blame.c +2 −2

diff --git a/builtin/blame.c b/builtin/blame.c
index e44a6bb..29eb31c 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1779,7 +1779,7 @@ static int prepare_lines(struct scoreboard *sb)
 	while (len--) {
 		if (bol) {
 			sb->lineno = xrealloc(sb->lineno,
-					      sizeof(int *) * (num + 1));
+					      sizeof(int) * (num + 1));
 			sb->lineno[num] = buf - sb->final_buf;
 			bol = 0;
 		}
@@ -1789,7 +1789,7 @@ static int prepare_lines(struct scoreboard *sb)
 		}
 	}
 	sb->lineno = xrealloc(sb->lineno,
-			      sizeof(int *) * (num + incomplete + 1));
+			      sizeof(int) * (num + incomplete + 1));
 	sb->lineno[num + incomplete] = buf - sb->final_buf;
 	sb->num_lines = num + incomplete;
 	return sb->num_lines;

-- 
1.8.3.2

David Kastrup· Feb 8, 2014, 09:49 UTC · re: David Kastrup · lore

Re: [PATCH] builtin/blame.c::prepare_lines: fix allocation size of sb->lineno

David Kastrup <dak@gnu.org> writes:

Show 23 quoted lines

> If we are calling xrealloc on every single line, the least we can do
> is get the right allocation size.
>
> Signed-off-by: David Kastrup <dak@gnu.org>
> ---
> This should be less contentious than the patch in
> <URL:http://permalink.gmane.org/gmane.comp.version-control.git/241561>,
> Message-ID: <1391550392-17118-1-git-send-email-dak@gnu.org> as it
> makes no stylistic decisions whatsoever and only fixes a clear bug.
>
> builtin/blame.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/blame.c b/builtin/blame.c
> index e44a6bb..29eb31c 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -1779,7 +1779,7 @@ static int prepare_lines(struct scoreboard *sb)
>  	while (len--) {
>  		if (bol) {
>  			sb->lineno = xrealloc(sb->lineno,
> -					      sizeof(int *) * (num + 1));
> +					      sizeof(int) * (num + 1));

But please note that since sb->lineno originally comes from a zeroed memory area and is passed to xrealloc, this requires that after

int *p; memset(&p, 0, sizeof(p));

the equivalence

((void *)p == NULL)

will hold. While this is true on most platforms, and while the C standard guarantees the slightly different ((void *)0 == NULL) is true, it makes no statement concerning the memory representation of the NULL pointer.

I have not bothered addressing this non-compliance with the C standard as it would be polishing a turd. A wholesale replacement has already been proposed, and it's likely that this assumption is prevalent in the Git codebase elsewhere anyway.

-- 
David Kastrup

Jeff King· Feb 8, 2014, 21:21 UTC · re: David Kastrup · lore

Re: [PATCH] builtin/blame.c::prepare_lines: fix allocation size of sb->lineno

On Sat, Feb 08, 2014 at 10:49:40AM +0100, David Kastrup wrote:

Show 20 quoted lines

> But please note that since sb->lineno originally comes from a zeroed
> memory area and is passed to xrealloc, this requires that after
> 
> int *p;
> memset(&p, 0, sizeof(p));
> 
> the equivalence
> 
> ((void *)p == NULL)
> 
> will hold.  While this is true on most platforms, and while the C
> standard guarantees the slightly different
> ((void *)0 == NULL)
> is true, it makes no statement concerning the memory representation of
> the NULL pointer.
> 
> I have not bothered addressing this non-compliance with the C standard
> as it would be polishing a turd.  A wholesale replacement has already
> been proposed, and it's likely that this assumption is prevalent in the
> Git codebase elsewhere anyway.

Yes, we explicitly break this part of the standard in the name of practicality (it simplifies frequently-used code, and machines on which it matters are rare enough that nobody has ever complained about it).

So I do not think this is a problem.

However, is there a reason not to use:

  sizeof(*sb->lineno)

rather than

  sizeof(int)

to avoid type-mismatch errors entirely (this applies both to this patch, and to any proposed rewrites using malloc).

-Peff

David Kastrup· Feb 8, 2014, 21:34 UTC · re: Jeff King · lore

Re: [PATCH] builtin/blame.c::prepare_lines: fix allocation size of sb->lineno

Jeff King <peff@peff.net> writes:

Show 10 quoted lines

> However, is there a reason not to use:
>
>   sizeof(*sb->lineno)
>
> rather than
>
>   sizeof(int)
>
> to avoid type-mismatch errors entirely (this applies both to this patch,
> and to any proposed rewrites using malloc).

It deviates from the style of the original code by tried and true Git developers. So feel free to roll your own patch here: it's not like this one has any copyrightable content in it.

-- 
David Kastrup