threads / patch / 34445

patchFix some sparse warnings

Subject: [PATCH] Fix some sparse warnings

## tl;dr

12 messages between Jul 15, 2013 and Jul 18, 2013. Diffs are folded; open one to read it.

replies: 11people: 6as markdown or json

Ramsay Jones· Jul 15, 2013, 17:31 UTC · lore

Sparse issues three "Using plain integer as NULL pointer" warnings. Each warning relates to the use of an '{0}' initialiser expression in the declaration of an 'struct object_info'. The first field of this structure has pointer type. Thus, in order to suppress these warnings, we replace the initialiser expression with '{NULL}'.

Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
---
Hi Jeff,

If you need to re-roll the patches in your 'jk/in-pack-size-measurement' branch, could you please squash this (or something like it) into the patches equivalent to commit 7c07385d ("zero-initialize object_info structs", 07-07-2013) [sha1_file.c and streaming.c] and commit 778d263a ("cat-file: add --batch-disk-sizes option", 07-07-2013) [builtin/cat-file.c].

Thanks!

ATB, Ramsay Jones

 builtin/cat-file.c | 2 +-
 sha1_file.c        | 2 +-
 streaming.c        | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)
Show changes to 3 files +3 −3

builtin/cat-file.c, sha1_file.c, streaming.c

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index bf12883..860576e 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -135,7 +135,7 @@ static int batch_one_object(const char *obj_name, int print_contents)
 	if (print_contents == BATCH)
 		contents = read_sha1_file(sha1, &type, &size);
 	else if (print_contents == BATCH_DISK_SIZES) {
-		struct object_info oi = {0};
+		struct object_info oi = {NULL};
 		oi.disk_sizep = &size;
 		type = sha1_object_info_extended(sha1, &oi);
 	}
diff --git a/sha1_file.c b/sha1_file.c
index 4c2365f..e4ab0a4 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2440,7 +2440,7 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi)
 
 int sha1_object_info(const unsigned char *sha1, unsigned long *sizep)
 {
-	struct object_info oi = {0};
+	struct object_info oi = {NULL};
 
 	oi.sizep = sizep;
 	return sha1_object_info_extended(sha1, &oi);
diff --git a/streaming.c b/streaming.c
index cac282f..5710065 100644
--- a/streaming.c
+++ b/streaming.c
@@ -135,7 +135,7 @@ struct git_istream *open_istream(const unsigned char *sha1,
 				 struct stream_filter *filter)
 {
 	struct git_istream *st;
-	struct object_info oi = {0};
+	struct object_info oi = {NULL};
 	const unsigned char *real = lookup_replace_object(sha1);
 	enum input_source src = istream_source(real, type, &oi);
 
-- 
1.8.3
Johannes Sixt· Jul 16, 2013, 05:57 UTC · re: Ramsay Jones · lore

Re: [PATCH] Fix some sparse warnings

Am 7/15/2013 19:31, schrieb Ramsay Jones:
> Sparse issues three "Using plain integer as NULL pointer" warnings.
> Each warning relates to the use of an '{0}' initialiser expression
> in the declaration of an 'struct object_info'.

I question the value of this warning. Initialization with '= {0}' is a well-established idiom, and sparse should know about it. Also, plain 0 *is* a null pointer constant.

-- Hannes
Jeff King· Jul 16, 2013, 06:21 UTC · re: Johannes Sixt · lore

Re: [PATCH] Fix some sparse warnings

On Tue, Jul 16, 2013 at 07:57:20AM +0200, Johannes Sixt wrote:
Show 8 quoted lines
> Am 7/15/2013 19:31, schrieb Ramsay Jones:
> > Sparse issues three "Using plain integer as NULL pointer" warnings.
> > Each warning relates to the use of an '{0}' initialiser expression
> > in the declaration of an 'struct object_info'.
> 
> I question the value of this warning. Initialization with '= {0}' is a
> well-established idiom, and sparse should know about it. Also, plain 0
> *is* a null pointer constant.

I agree with you. It's not a bug, and I think sparse is being overly picky here; it is missing the forest for the trees in interpreting the idiom.

Still, it may be worth tweaking in the name of eliminating compiler noise, since it does not cost us very much to do so (and I believe we have done so in the past, too).

We could also ask people with sparse to turn off the "use NULL instead of 0" warning, but I think it _is_ a useful warning elsewhere (even though it is never a bug, it violates our style guidelines and may be an indication of a bug). It would be nice if sparse learned to ignore the warning in this particular idiom, but I am not going to hold my breath for that.

-Peff
Philip Oakley· Jul 16, 2013, 20:53 UTC · re: Jeff King · lore

Re: [PATCH] Fix some sparse warnings

From: "Jeff King" <peff@peff.net>
Sent: Tuesday, July 16, 2013 7:21 AM
Show 31 quoted lines
> On Tue, Jul 16, 2013 at 07:57:20AM +0200, Johannes Sixt wrote:
>
>> Am 7/15/2013 19:31, schrieb Ramsay Jones:
>> > Sparse issues three "Using plain integer as NULL pointer" warnings.
>> > Each warning relates to the use of an '{0}' initialiser expression
>> > in the declaration of an 'struct object_info'.
>>
>> I question the value of this warning. Initialization with '= {0}' is
>> a
>> well-established idiom, and sparse should know about it. Also, plain
>> 0
>> *is* a null pointer constant.
>
> I agree with you. It's not a bug, and I think sparse is being overly
> picky here; it is missing the forest for the trees in interpreting the
> idiom.
>
> Still, it may be worth tweaking in the name of eliminating compiler
> noise, since it does not cost us very much to do so (and I believe we
> have done so in the past, too).
>
> We could also ask people with sparse to turn off the "use NULL instead
> of 0" warning, but I think it _is_ a useful warning elsewhere (even
> though it is never a bug, it violates our style guidelines and may be
> an
> indication of a bug). It would be nice if sparse learned to ignore the
> warning in this particular idiom, but I am not going to hold my breath
> for that.
>
> -Peff
> --

On the subject of warnings and null pointers, yesterday's Code Project news linked to a blog on the problems of unexpected optimization bugs, such as dereferencing a null pointer. "Finding Undefined Behavior Bugs by Finding Dead Code" http://blog.regehr.org/archives/970 which links to the draft of an interesting paper [http://pdos.csail.mit.edu/~xi/papers/stack-sosp13.pdf]

Does anyone run the "new static checker called 'Stack' that precisely identifies unstable code"? [though the paper's conclusion says 'All Stack source code will be publicly available.' which suggests it's not yet available]

Or use the ‘-fno-delete-null-pointer-checks’ referred to in the blog comments (see also index : kernel/git/torvalds/linux.git "Add '-fno-delete-null-pointer-checks' to gcc CFLAGS" https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=a3ca86aea507904148870946d599e07a340b39bf

You're probably already aware of these aspects but I thought it worth mentioning for the wider readership.

regards
Philip
Stefan Beller· Jul 16, 2013, 21:18 UTC · re: Philip Oakley · lore

Re: [PATCH] Fix some sparse warnings

On 07/16/2013 10:53 PM, Philip Oakley wrote:
Show 60 quoted lines
> From: "Jeff King" <peff@peff.net>
> Sent: Tuesday, July 16, 2013 7:21 AM
>> On Tue, Jul 16, 2013 at 07:57:20AM +0200, Johannes Sixt wrote:
>>
>>> Am 7/15/2013 19:31, schrieb Ramsay Jones:
>>> > Sparse issues three "Using plain integer as NULL pointer" warnings.
>>> > Each warning relates to the use of an '{0}' initialiser expression
>>> > in the declaration of an 'struct object_info'.
>>>
>>> I question the value of this warning. Initialization with '= {0}' is
>>> a
>>> well-established idiom, and sparse should know about it. Also, plain
>>> 0
>>> *is* a null pointer constant.
>>
>> I agree with you. It's not a bug, and I think sparse is being overly
>> picky here; it is missing the forest for the trees in interpreting the
>> idiom.
>>
>> Still, it may be worth tweaking in the name of eliminating compiler
>> noise, since it does not cost us very much to do so (and I believe we
>> have done so in the past, too).
>>
>> We could also ask people with sparse to turn off the "use NULL instead
>> of 0" warning, but I think it _is_ a useful warning elsewhere (even
>> though it is never a bug, it violates our style guidelines and may be
>> an
>> indication of a bug). It would be nice if sparse learned to ignore the
>> warning in this particular idiom, but I am not going to hold my breath
>> for that.
>>
>> -Peff
>> -- 
> 
> On the subject of warnings and null pointers, yesterday's Code Project
> news linked to a blog on the problems of unexpected optimization bugs,
> such as dereferencing a null pointer. "Finding Undefined Behavior Bugs
> by Finding Dead Code" http://blog.regehr.org/archives/970 which links to
> the draft of an interesting paper
> [http://pdos.csail.mit.edu/~xi/papers/stack-sosp13.pdf]
> 
> Does anyone run the "new static checker called 'Stack' that precisely
> identifies unstable code"? [though the paper's conclusion says 'All
> Stack source code will be publicly available.' which suggests it's not
> yet available]
> 
> Or use the ‘-fno-delete-null-pointer-checks’ referred to in the blog
> comments (see also index : kernel/git/torvalds/linux.git "Add
> '-fno-delete-null-pointer-checks' to gcc CFLAGS"
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=a3ca86aea507904148870946d599e07a340b39bf
> 
> 
> You're probably already aware of these aspects but I thought it worth
> mentioning for the wider readership.
> 
> regards
> 
> Philip
> 
> 

I recently started contributing and I used cppcheck found at https://github.com/danmar/cppcheck to submit the patches at origin/sb/misc-fixes As it is originally for C++, that tool throws lots of false-positives (i.e. warns about null pointer dereferencing when it's not possible to be a null pointer) unfortunately.

Also I hear llvm/clang comes with a good static code analyzer, which I tried today on a different project. Though I could not figure out how to use that on a pure C project such as git, as that tool seems to require a C++ compilation for doing its static code analysis.

The blog post you linked to seems very interesting and promising. :)

Regards, Stefan

Philip Oakley· Jul 16, 2013, 22:18 UTC · re: Stefan Beller · lore

Re: [PATCH] Fix some sparse warnings

On 16/07/13 22:18, Stefan Beller wrote:
Show 80 quoted lines
> On 07/16/2013 10:53 PM, Philip Oakley wrote:
>> From: "Jeff King" <peff@peff.net>
>> Sent: Tuesday, July 16, 2013 7:21 AM
>>> On Tue, Jul 16, 2013 at 07:57:20AM +0200, Johannes Sixt wrote:
>>>
>>>> Am 7/15/2013 19:31, schrieb Ramsay Jones:
>>>>> Sparse issues three "Using plain integer as NULL pointer" warnings.
>>>>> Each warning relates to the use of an '{0}' initialiser expression
>>>>> in the declaration of an 'struct object_info'.
>>>>
>>>> I question the value of this warning. Initialization with '= {0}' is
>>>> a
>>>> well-established idiom, and sparse should know about it. Also, plain
>>>> 0
>>>> *is* a null pointer constant.
>>>
>>> I agree with you. It's not a bug, and I think sparse is being overly
>>> picky here; it is missing the forest for the trees in interpreting the
>>> idiom.
>>>
>>> Still, it may be worth tweaking in the name of eliminating compiler
>>> noise, since it does not cost us very much to do so (and I believe we
>>> have done so in the past, too).
>>>
>>> We could also ask people with sparse to turn off the "use NULL instead
>>> of 0" warning, but I think it _is_ a useful warning elsewhere (even
>>> though it is never a bug, it violates our style guidelines and may be
>>> an
>>> indication of a bug). It would be nice if sparse learned to ignore the
>>> warning in this particular idiom, but I am not going to hold my breath
>>> for that.
>>>
>>> -Peff
>>> --
>>
>> On the subject of warnings and null pointers, yesterday's Code Project
>> news linked to a blog on the problems of unexpected optimization bugs,
>> such as dereferencing a null pointer. "Finding Undefined Behavior Bugs
>> by Finding Dead Code" http://blog.regehr.org/archives/970 which links to
>> the draft of an interesting paper
>> [http://pdos.csail.mit.edu/~xi/papers/stack-sosp13.pdf]
>>
>> Does anyone run the "new static checker called 'Stack' that precisely
>> identifies unstable code"? [though the paper's conclusion says 'All
>> Stack source code will be publicly available.' which suggests it's not
>> yet available]
>>
>> Or use the ‘-fno-delete-null-pointer-checks’ referred to in the blog
>> comments (see also index : kernel/git/torvalds/linux.git "Add
>> '-fno-delete-null-pointer-checks' to gcc CFLAGS"
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=a3ca86aea507904148870946d599e07a340b39bf
>>
>>
>> You're probably already aware of these aspects but I thought it worth
>> mentioning for the wider readership.
>>
>> regards
>>
>> Philip
>>
>>
>
> I recently started contributing and I used cppcheck found at
> https://github.com/danmar/cppcheck to submit the patches
> at origin/sb/misc-fixes
> As it is originally for C++, that tool throws lots of
> false-positives (i.e. warns about null pointer dereferencing
> when it's not possible to be a null pointer) unfortunately.
>
> Also I hear llvm/clang comes with a good static code analyzer,
> which I tried today on a different project.
> Though I could not figure out how to use that on a pure C project
> such as git, as that tool seems to require a C++ compilation for
> doing its static code analysis.
>
> The blog post you linked to seems very interesting and promising. :)
>
> Regards,
> Stefan
>

A bit more searching also finds http://pdos.csail.mit.edu/~xi/papers/ub-apsys12.pdf and http://pdos.csail.mit.edu/~xi/papers/ub-apsys12-slides.pptx by the same author.

Johannes Sixt· Jul 17, 2013, 05:47 UTC · re: Stefan Beller · lore

Re: [PATCH] Fix some sparse warnings

>>>> I question the value of this warning. Initialization with '=
>>>> {0}' is a well-established idiom, and sparse should know about
>>>> it.

Thanks everyone for your feedback. But I really wanted to call only the warning in the case of the '= {0}' idiom into question, not about 0 vs. NULL in general.

-- Hannes
Stefan Beller· Jul 17, 2013, 22:08 UTC · re: Philip Oakley · lore

Re: [PATCH] Fix some sparse warnings

On 07/16/2013 10:53 PM, Philip Oakley wrote:
Show 6 quoted lines
> 
> Does anyone run the "new static checker called 'Stack' that precisely
> identifies unstable code"? [though the paper's conclusion says 'All
> Stack source code will be publicly available.' which suggests it's not
> yet available]
> 

So I started using the clang code analyzer on git. One of the first warnings actually is this:

object.c:241:7: warning: Branch condition evaluates to a garbage value
                if (!eaten)
So that part of object.c lookx like this:
	struct object *parse_object(const unsigned char *sha1) 
	{
		int eaten;
		...
		obj = parse_object_buffer(sha1, type, size, buffer, &eaten);
		if (!eaten)
			free(buffer);
	}
And the parse_object_buffer looks like this with respect to the eaten 
variable:
	struct object *parse_object_buffer(...)
	{
		int eaten = 0;
		if (something)
			return NULL;
		...
		if (something_different)
			eaten=1;
		*eaten_p = eaten;
	}
So what might happen is, that parse_object_buffer exits early, without
executing 
	
	*eaten_p = eaten;

Then in the parse_object function eaten was never initialized nor set inside the call to parse_object_buffer. Then it is obvious that the free(buffer) is executed depending on garbage left on the stack. Definitely something what we want to change.

The obvious way to repair this would be to just initialize the eaten variable
inside parse_object.
	struct object *parse_object(const unsigned char *sha1) 
	{
		int eaten=0;
		...

However I'd like to propose another solution: In parse_object_buffer we do not have a local eaten variable, but directly write to *eaten_p. That would be the following patch.

Was there a particular idea or goal behind first having a local eaten variable, which later near the correct return of the function was used to set the eaten_p?

Thanks, Stefan

Stefan Beller· Jul 17, 2013, 22:09 UTC · re: Stefan Beller · lore

[PATCH] parse_object_buffer: Correct freeing the buffer.

If we exit early in the function parse_object_buffer, we did not write to *eaten_p. Then the calling function parse_object, which looks like the following with respect to the eaten variable, cannot rely on a proper value set in eaten, hence the freeing of the buffer depends on random values in memory.

	struct object *parse_object(const unsigned char *sha1)
	{
		int eaten;
		...
		obj = parse_object_buffer(sha1, type, size, buffer, &eaten);
		if (!eaten)
			free(buffer);
	}
This change makes sure, the buffer freeing condition is deterministic.
Signed-off-by: Stefan Beller <stefanbeller@googlemail.com>
---
 object.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)
Show changes to object.c +3 −4
diff --git a/object.c b/object.c
index cbc7333..d8a4b1f 100644
--- a/object.c
+++ b/object.c
@@ -145,7 +145,7 @@ struct object *lookup_unknown_object(const unsigned char *sha1)
 struct object *parse_object_buffer(const unsigned char *sha1, enum object_type type, unsigned long size, void *buffer, int *eaten_p)
 {
 	struct object *obj;
-	int eaten = 0;
+	*eaten_p = 0;
 
 	obj = NULL;
 	if (type == OBJ_BLOB) {
@@ -164,7 +164,7 @@ struct object *parse_object_buffer(const unsigned char *sha1, enum object_type t
 			if (!tree->object.parsed) {
 				if (parse_tree_buffer(tree, buffer, size))
 					return NULL;
-				eaten = 1;
+				*eaten_p = 1;
 			}
 		}
 	} else if (type == OBJ_COMMIT) {
@@ -174,7 +174,7 @@ struct object *parse_object_buffer(const unsigned char *sha1, enum object_type t
 				return NULL;
 			if (!commit->buffer) {
 				commit->buffer = buffer;
-				eaten = 1;
+				*eaten_p = 1;
 			}
 			obj = &commit->object;
 		}
@@ -191,7 +191,6 @@ struct object *parse_object_buffer(const unsigned char *sha1, enum object_type t
 	}
 	if (obj && obj->type == OBJ_NONE)
 		obj->type = type;
-	*eaten_p = eaten;
 	return obj;
 }
 
-- 
1.8.3.3.754.g9c3c367.dirty
Stefan Beller· Jul 17, 2013, 22:16 UTC · re: Stefan Beller · lore

Re: [PATCH] Fix some sparse warnings

On 07/18/2013 12:08 AM, Stefan Beller wrote:
> 
> So I started using the clang code analyzer on git. One of the 
> first warnings actually is this:
> 

So in case somebody else would also like to play around with the clang static code analyzer:

# get clang: cd <good location> git clone http://llvm.org/git/llvm.git cd llvm/tools git clone http://llvm.org/git/clang cd <good location> mkdir build && cd build cmake .. # if desired make install

# in the Makefile of git change CC = <good location>/llvm/tools/clang/tools/scan-build/ccc-analyzer # then obviously: make clean make

Junio C Hamano· Jul 17, 2013, 23:22 UTC · re: Stefan Beller · lore

Re: [PATCH] Fix some sparse warnings

Stefan Beller <stefanbeller@googlemail.com> writes:
Show 16 quoted lines
> And the parse_object_buffer looks like this with respect to the eaten 
> variable:
> 	struct object *parse_object_buffer(...)
> 	{
> 		int eaten = 0;
> 		if (something)
> 			return NULL;
> 		...
> 		if (something_different)
> 			eaten=1;
> 		*eaten_p = eaten;
> 	}
> ...
> Was there a particular idea or goal behind first having a local eaten
> variable, which later near the correct return of the function was used to set the 
> eaten_p?

I didn't run "blame" to see the evolution of this function, but I suspect that the original code, when the "eaten" local variable was introduced, very much tried to do exactly what you suspect. The early return codepaths you see in today's code may be much newer, added without much thinking about the exact issue you are bringing up.

Ramsay Jones· Jul 18, 2013, 17:58 UTC · re: Jeff King · lore

Re: [PATCH] Fix some sparse warnings

Jeff King wrote:
Show 14 quoted lines
> On Tue, Jul 16, 2013 at 07:57:20AM +0200, Johannes Sixt wrote:
> 
>> Am 7/15/2013 19:31, schrieb Ramsay Jones:
>>> Sparse issues three "Using plain integer as NULL pointer" warnings.
>>> Each warning relates to the use of an '{0}' initialiser expression
>>> in the declaration of an 'struct object_info'.
>>
>> I question the value of this warning. Initialization with '= {0}' is a
>> well-established idiom, and sparse should know about it. Also, plain 0
>> *is* a null pointer constant.
> 
> I agree with you. It's not a bug, and I think sparse is being overly
> picky here; it is missing the forest for the trees in interpreting the
> idiom.

Yes, last time this came up, I looked at writing a patch to sparse to allow this without complaint. It's still on my sparse TODO list, but even if I finished it tonight, it would take a while to get into a released version of sparse. ;-)

Show 8 quoted lines
> Still, it may be worth tweaking in the name of eliminating compiler
> noise, since it does not cost us very much to do so (and I believe we
> have done so in the past, too).
> 
> We could also ask people with sparse to turn off the "use NULL instead
> of 0" warning, but I think it _is_ a useful warning elsewhere (even
> though it is never a bug, it violates our style guidelines and may be an
> indication of a bug).
Indeed, if it wasn't for this, I would be happy to turn this warning off.

ATB, Ramsay Jones

← back to recent threads