[PATCH 0/7] Coverity

[PATCH 0/7] Coverity

[David Gibson]

Since Emilio has set up ccan on Coverity Scan, here are some patches
making use of it.  The first updates our Travis configuration to push
builds to Coverity for analysis.  The rest fix some of the easier to
address bugs that Coverity located.

David Gibson (7):
  .travis.yml: Add support for Coverity Scan
  failtest: Remove memory leak
  tools: Remove fd leak
  crypto/hmac_sha256: Remove undefined memset()
  crypto/ripemd160: Correct badly sized union member
  ccanlint: Fix leak in do_reduce_features()
  net: Add check for failure of setsockopt()

 .travis.yml                            | 17 +++++++++++++++++
 ccan/crypto/hmac_sha256/hmac_sha256.c  |  4 +++-
 ccan/crypto/ripemd160/ripemd160.h      |  2 +-
 ccan/failtest/failtest.c               |  4 +++-
 ccan/net/net.c                         |  4 +++-
 tools/ccanlint/tests/reduce_features.c |  1 +
 tools/depends.c                        |  4 +++-
 7 files changed, 31 insertions(+), 5 deletions(-)

-- 
2.9.3

_______________________________________________
ccan mailing list
ccan@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/ccan

[PATCH 1/7] .travis.yml: Add support for Coverity Scan

[David Gibson]

Add support for Travis to upload builds to Coverity Scan.

Travis encrypted keys are there which should make it work for builds
at either:
    https://travis-ci.org/rustyrussell/ccan
or
    https://travis-ci.org/dgibson/ccan

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 .travis.yml | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/.travis.yml b/.travis.yml
index 9f9dbcb..fe3dffc 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -1,6 +1,23 @@
 language: c
 sudo: false
 
+# Coverity Scan uploads
+env:
+  global:
+  # COVERITY_SCAN_TOKEN (rustyrussell/ccan)
+  - secure: "RU+r62MYrhaUcFOiXykFt0lpFzQVL6f5gyAKOv39PpYK10rsI/cM6N8wOReH1EmI7ktn7M+g3Fs2jA+i+bZW4rZSfm379hzduWisEo0vEWNwyWiIrl1WjgGptFDwOCzMQHwo+u4zH6E+CAOWnje5Zw+elokVTfC1orLbuQHABG4="
+  # COVERITY_SCAN_TOKEN (dgibson/ccan)
+  - secure: "OL7OqYjQPE6rpHwujZbZt1gR5TNYn6Z0b1JxBaL1AG+iHlTbRrFGqec/VpYi2SAlqcDZK3x2lUzsF+hzx4pNBLFu8wvMtDKD4grlME5M+Bu3LT+YnmWSkaCw1/01n/SFyeRAkNDUJhnp0wS5oJ9sWx6trKWf9Xw9HpoKlwbFEGM="
+
+addons:
+  coverity_scan:
+    project:
+      name: CCAN
+      #description: CCAN
+    notification_email: ccan@lists.ozlabs.org
+    build_command: make
+    branch_pattern: coverity_scan
+
 matrix:
   include:
   - compiler: gcc
-- 
2.9.3

_______________________________________________
ccan mailing list
ccan@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/ccan

[PATCH 2/7] failtest: Remove memory leak

[David Gibson]

Somewhat ironically, a path in failtest related to detecting leaks in the
tested program itself leaks memory.  This corrects it.

Detected by Coverity.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 ccan/failtest/failtest.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/ccan/failtest/failtest.c b/ccan/failtest/failtest.c
index aab28dd..c61ce44 100644
--- a/ccan/failtest/failtest.c
+++ b/ccan/failtest/failtest.c
@@ -613,8 +613,10 @@ static NORETURN void failtest_cleanup(bool forced_cleanup, int status)
 
 		/* But their program shouldn't leak, even on failure. */
 		if (!forced_cleanup && i->can_leak) {
+			char *p = failpath_string();
 			printf("Leak at %s:%u: --failpath=%s\n",
-			       i->file, i->line, failpath_string());
+			       i->file, i->line, p);
+			free(p);
 			status = 1;
 		}
 	}
-- 
2.9.3

_______________________________________________
ccan mailing list
ccan@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/ccan

[PATCH 3/7] tools: Remove fd leak

[David Gibson]

compile_info() can leak an open file descriptor write_all() fails.  This
corrects it.

Found by Coverity.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 tools/depends.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/depends.c b/tools/depends.c
index e6a79d7..36b5809 100644
--- a/tools/depends.c
+++ b/tools/depends.c
@@ -54,8 +54,10 @@ char *compile_info(const void *ctx, const char *dir)
 	fd = open(info_c_file, O_WRONLY|O_CREAT|O_EXCL, 0600);
 	if (fd < 0)
 		return NULL;
-	if (!write_all(fd, info, tal_count(info)-1))
+	if (!write_all(fd, info, tal_count(info)-1)) {
+		close(fd);
 		return NULL;
+	}
 
 	if (close(fd) != 0)
 		return NULL;
-- 
2.9.3

_______________________________________________
ccan mailing list
ccan@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/ccan

[PATCH 4/7] crypto/hmac_sha256: Remove undefined memset()

[David Gibson]

A memset() with a zero length argument isn't well defined.  It *might* be
a no-op, but then it might not.  So, test for this case in
hmac_sha256_init().

Found by Coverity.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 ccan/crypto/hmac_sha256/hmac_sha256.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/ccan/crypto/hmac_sha256/hmac_sha256.c b/ccan/crypto/hmac_sha256/hmac_sha256.c
index 0392afe..70ca3b2 100644
--- a/ccan/crypto/hmac_sha256/hmac_sha256.c
+++ b/ccan/crypto/hmac_sha256/hmac_sha256.c
@@ -36,7 +36,9 @@ void hmac_sha256_init(struct hmac_sha256_ctx *ctx,
 	 *   appended with 44 zero bytes 0x00)
 	 */
 	memcpy(k_ipad, k, ksize);
-	memset((char *)k_ipad + ksize, 0, HMAC_SHA256_BLOCKSIZE - ksize);
+	if (ksize < HMAC_SHA256_BLOCKSIZE)
+		memset((char *)k_ipad + ksize, 0,
+		       HMAC_SHA256_BLOCKSIZE - ksize);
 
 	/*
 	 * (2) XOR (bitwise exclusive-OR) the B byte string computed
-- 
2.9.3

_______________________________________________
ccan mailing list
ccan@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/ccan

[PATCH 5/7] crypto/ripemd160: Correct badly sized union member

[David Gibson]

struct ripemd160_ctx has a union for converting between u8[] and u32[]
data.  Unfortunately the u32 array has a miscalculated size, half the size
of the u8 array.  That means some accesses which are within the union can
technically overrun the u32 array.

Found by Coverity scan.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 ccan/crypto/ripemd160/ripemd160.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ccan/crypto/ripemd160/ripemd160.h b/ccan/crypto/ripemd160/ripemd160.h
index 377a07d..56854cf 100644
--- a/ccan/crypto/ripemd160/ripemd160.h
+++ b/ccan/crypto/ripemd160/ripemd160.h
@@ -49,7 +49,7 @@ struct ripemd160_ctx {
 	uint32_t s[5];
 	uint64_t bytes;
 	union {
-		uint32_t u32[8];
+		uint32_t u32[16];
 		unsigned char u8[64];
 	} buf;
 #endif
-- 
2.9.3

_______________________________________________
ccan mailing list
ccan@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/ccan

[PATCH 6/7] ccanlint: Fix leak in do_reduce_features()

[David Gibson]

options_avail and options_used get freed, but options does not.

Found by Coverity scan.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 tools/ccanlint/tests/reduce_features.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/ccanlint/tests/reduce_features.c b/tools/ccanlint/tests/reduce_features.c
index b49922b..34a8970 100644
--- a/tools/ccanlint/tests/reduce_features.c
+++ b/tools/ccanlint/tests/reduce_features.c
@@ -181,6 +181,7 @@ static void do_reduce_features(struct manifest *m,
 		err(1, "Creating reduced-features/config.h");
 	if (!write_all(fd, hdr, strlen(hdr)))
 		err(1, "Writing reduced-features/config.h");
+	htable_option_free(options);
 	close(fd);
 	features_were_reduced = true;
 }
-- 
2.9.3

_______________________________________________
ccan mailing list
ccan@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/ccan

[PATCH 7/7] net: Add check for failure of setsockopt()

[David Gibson]

make_listen_fd() didn't check for failure of setsockopt().  There's no
real reason not to, since we have an obvious way to report an error to the
caller.

Found with Coverity Scan.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 ccan/net/net.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/ccan/net/net.c b/ccan/net/net.c
index e919983..11c6b67 100644
--- a/ccan/net/net.c
+++ b/ccan/net/net.c
@@ -238,7 +238,9 @@ static int make_listen_fd(const struct addrinfo *addrinfo)
 	if (fd < 0)
 		return -1;
 
-	setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &on, sizeof(on));
+	if (setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &on, sizeof(on)) != 0)
+		goto fail;
+
 	if (bind(fd, addrinfo->ai_addr, addrinfo->ai_addrlen) != 0)
 		goto fail;
 
-- 
2.9.3

_______________________________________________
ccan mailing list
ccan@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/ccan

Re: [PATCH 1/7] .travis.yml: Add support for Coverity Scan

[David Gibson]

[-- Attachment #1.1: Type: text/plain, Size: 1682 bytes --]

On Mon, Apr 03, 2017 at 09:11:06PM +1000, David Gibson wrote:
> Add support for Travis to upload builds to Coverity Scan.
> 
> Travis encrypted keys are there which should make it work for builds
> at either:
>     https://travis-ci.org/rustyrussell/ccan
> or
>     https://travis-ci.org/dgibson/ccan
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  .travis.yml | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/.travis.yml b/.travis.yml
> index 9f9dbcb..fe3dffc 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -1,6 +1,23 @@
>  language: c
>  sudo: false
>  
> +# Coverity Scan uploads
> +env:
> +  global:
> +  # COVERITY_SCAN_TOKEN (rustyrussell/ccan)
> +  - secure: "RU+r62MYrhaUcFOiXykFt0lpFzQVL6f5gyAKOv39PpYK10rsI/cM6N8wOReH1EmI7ktn7M+g3Fs2jA+i+bZW4rZSfm379hzduWisEo0vEWNwyWiIrl1WjgGptFDwOCzMQHwo+u4zH6E+CAOWnje5Zw+elokVTfC1orLbuQHABG4="
> +  # COVERITY_SCAN_TOKEN (dgibson/ccan)
> +  - secure: "OL7OqYjQPE6rpHwujZbZt1gR5TNYn6Z0b1JxBaL1AG+iHlTbRrFGqec/VpYi2SAlqcDZK3x2lUzsF+hzx4pNBLFu8wvMtDKD4grlME5M+Bu3LT+YnmWSkaCw1/01n/SFyeRAkNDUJhnp0wS5oJ9sWx6trKWf9Xw9HpoKlwbFEGM="
> +
> +addons:
> +  coverity_scan:
> +    project:
> +      name: CCAN
> +      #description: CCAN
> +    notification_email: ccan@lists.ozlabs.org

Oops, this email still needs an update though.

> +    build_command: make
> +    branch_pattern: coverity_scan
> +
>  matrix:
>    include:
>    - compiler: gcc

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
ccan mailing list
ccan@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/ccan

Re: [PATCH 4/7] crypto/hmac_sha256: Remove undefined memset()

[Rusty Russell]

David Gibson <david@gibson.dropbear.id.au> writes:
> A memset() with a zero length argument isn't well defined.  It *might* be
> a no-op, but then it might not.  So, test for this case in
> hmac_sha256_init().

Huh?  That seems really odd.

Rusty.

> Found by Coverity.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  ccan/crypto/hmac_sha256/hmac_sha256.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/ccan/crypto/hmac_sha256/hmac_sha256.c b/ccan/crypto/hmac_sha256/hmac_sha256.c
> index 0392afe..70ca3b2 100644
> --- a/ccan/crypto/hmac_sha256/hmac_sha256.c
> +++ b/ccan/crypto/hmac_sha256/hmac_sha256.c
> @@ -36,7 +36,9 @@ void hmac_sha256_init(struct hmac_sha256_ctx *ctx,
>  	 *   appended with 44 zero bytes 0x00)
>  	 */
>  	memcpy(k_ipad, k, ksize);
> -	memset((char *)k_ipad + ksize, 0, HMAC_SHA256_BLOCKSIZE - ksize);
> +	if (ksize < HMAC_SHA256_BLOCKSIZE)
> +		memset((char *)k_ipad + ksize, 0,
> +		       HMAC_SHA256_BLOCKSIZE - ksize);
>  
>  	/*
>  	 * (2) XOR (bitwise exclusive-OR) the B byte string computed
> -- 
> 2.9.3
>
> _______________________________________________
> ccan mailing list
> ccan@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/ccan
_______________________________________________
ccan mailing list
ccan@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/ccan

Re: [PATCH 7/7] net: Add check for failure of setsockopt()

[Rusty Russell]

David Gibson <david@gibson.dropbear.id.au> writes:
> make_listen_fd() didn't check for failure of setsockopt().  There's no
> real reason not to, since we have an obvious way to report an error to the
> caller.
>
> Found with Coverity Scan.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Great, all but the one I commented on look fine.  Please apply...

Acked-by: Rusty Russell <rusty@rustcorp.com.au>

Thanks,
Rusty.

> ---
>  ccan/net/net.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/ccan/net/net.c b/ccan/net/net.c
> index e919983..11c6b67 100644
> --- a/ccan/net/net.c
> +++ b/ccan/net/net.c
> @@ -238,7 +238,9 @@ static int make_listen_fd(const struct addrinfo *addrinfo)
>  	if (fd < 0)
>  		return -1;
>  
> -	setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &on, sizeof(on));
> +	if (setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &on, sizeof(on)) != 0)
> +		goto fail;
> +
>  	if (bind(fd, addrinfo->ai_addr, addrinfo->ai_addrlen) != 0)
>  		goto fail;
>  
> -- 
> 2.9.3
>
> _______________________________________________
> ccan mailing list
> ccan@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/ccan
_______________________________________________
ccan mailing list
ccan@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/ccan

Re: [PATCH 4/7] crypto/hmac_sha256: Remove undefined memset()

[David Gibson]

[-- Attachment #1.1: Type: text/plain, Size: 1809 bytes --]

On Tue, Apr 04, 2017 at 11:52:39AM +0930, Paul 'Rusty' Russell wrote:
> David Gibson <david@gibson.dropbear.id.au> writes:
> > A memset() with a zero length argument isn't well defined.  It *might* be
> > a no-op, but then it might not.  So, test for this case in
> > hmac_sha256_init().
> 
> Huh?  That seems really odd.

Well.. more precisely, my understanding is that memset(p, x, 0) can't
be counted on not to dereference p.

In this case, k_ipad + ksize is past the end of the array, and so not
a valid pointer to dereference.
> 
> Rusty.
> 
> > Found by Coverity.
> >
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  ccan/crypto/hmac_sha256/hmac_sha256.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/ccan/crypto/hmac_sha256/hmac_sha256.c b/ccan/crypto/hmac_sha256/hmac_sha256.c
> > index 0392afe..70ca3b2 100644
> > --- a/ccan/crypto/hmac_sha256/hmac_sha256.c
> > +++ b/ccan/crypto/hmac_sha256/hmac_sha256.c
> > @@ -36,7 +36,9 @@ void hmac_sha256_init(struct hmac_sha256_ctx *ctx,
> >  	 *   appended with 44 zero bytes 0x00)
> >  	 */
> >  	memcpy(k_ipad, k, ksize);
> > -	memset((char *)k_ipad + ksize, 0, HMAC_SHA256_BLOCKSIZE - ksize);
> > +	if (ksize < HMAC_SHA256_BLOCKSIZE)
> > +		memset((char *)k_ipad + ksize, 0,
> > +		       HMAC_SHA256_BLOCKSIZE - ksize);
> >  
> >  	/*
> >  	 * (2) XOR (bitwise exclusive-OR) the B byte string computed
> >
> > _______________________________________________
> > ccan mailing list
> > ccan@lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/ccan
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
ccan mailing list
ccan@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/ccan

Re: [PATCH 4/7] crypto/hmac_sha256: Remove undefined memset()

[Rusty Russell]

David Gibson <david@gibson.dropbear.id.au> writes:
> [ Unknown signature status ]
> On Tue, Apr 04, 2017 at 11:52:39AM +0930, Paul 'Rusty' Russell wrote:
>> David Gibson <david@gibson.dropbear.id.au> writes:
>> > A memset() with a zero length argument isn't well defined.  It *might* be
>> > a no-op, but then it might not.  So, test for this case in
>> > hmac_sha256_init().
>> 
>> Huh?  That seems really odd.
>
> Well.. more precisely, my understanding is that memset(p, x, 0) can't
> be counted on not to dereference p.

That seems nonsensical, though.

> In this case, k_ipad + ksize is past the end of the array, and so not
> a valid pointer to dereference.

The pointer is valid (C's famous one-past-end exception), dereferencing
it is not.

What's the actual coverity error?

Thanks,
Rusty.
_______________________________________________
ccan mailing list
ccan@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/ccan

Re: [PATCH 4/7] crypto/hmac_sha256: Remove undefined memset()

[Timothy B. Terriberry]

Rusty Russell wrote:
>> Well.. more precisely, my understanding is that memset(p, x, 0) can't
>> be counted on not to dereference p.
>
> That seems nonsensical, though.

It is nonsensical, but welcome to standards.

C99 Section 7.21.1 "String function conventions" (which includes memset, 
as it is in string.h):

"Where an argument declared as size_t n specifies the length of the 
array for a function, n can have the value zero on a call to that 
function. Unless explicitly stated otherwise in the description of a 
particular function in this subclause, pointer arguments on such a call 
shall still have valid values, as described in 7.1.4"

C99 Section 7.1.4 "Use of library functions":

"If an argument to a function has an invalid value (such as a value 
outside the domain of the function, or a pointer outside the address 
space of the program, or a null pointer, or a pointer to non-modifiable 
storage when the corresponding parameter is not const-qualified) or a 
type (after promotion) not expected by a function with variable number 
of arguments, the behavior is undefined."

C99 Section 7.21.6.1 defines memset() as taking an argument size_t n 
that specifies the length of the array for that function, but does not 
explicitly state that it can be a null pointer, so people have 
interpreted this to mean that passing it NULL is undefined behavior.


C89 said the same thing, albeit more briefly:

C89 Section 4.1.6 "Use of library functions":

"Each of the following statements applies unless explicitly stated 
otherwise in the detailed descriptions that follow. If an argument to a 
function has an invalid value (such as a value outside the domain of the 
function, or a pointer outside the address space of the program, or a 
null pointer), the behavior is undefined."

The detailed description of memcpy() in Section 4.11.2.1 does not 
explicitly state otherwise.

_______________________________________________
ccan mailing list
ccan@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/ccan

Re: [PATCH 4/7] crypto/hmac_sha256: Remove undefined memset()

[Rusty Russell]

"Timothy B. Terriberry" <tterribe@xiph.org> writes:
> Rusty Russell wrote:
>>> Well.. more precisely, my understanding is that memset(p, x, 0) can't
>>> be counted on not to dereference p.
>>
>> That seems nonsensical, though.
>
> It is nonsensical, but welcome to standards.
>
> C99 Section 7.21.1 "String function conventions" (which includes memset, 
> as it is in string.h):
>
> "Where an argument declared as size_t n specifies the length of the 
> array for a function, n can have the value zero on a call to that 
> function. Unless explicitly stated otherwise in the description of a 
> particular function in this subclause, pointer arguments on such a call 
> shall still have valid values, as described in 7.1.4"

I think you're confusing "valid pointer" with "dereferencable pointer".

A pointer one past the end is still a valid pointer, just not
dereferencable.  In this case, that's what we have when ksize ==
HMAC_SHA256_BLOCKSIZE:

 	memset((char *)k_ipad + ksize, 0, HMAC_SHA256_BLOCKSIZE - ksize);

ie. we are trying to set the unused trailing bytes of the region to
zero.

Even if you were correct, and code should be testing for zero sizes in
such cases, and don't care that Kernighan and Plauger would be rolling
in their graves (and neither is even dead yet!), you would just cause me
to write a test case.

Because you should *never* give in to insanity except under duress, and
then with loud protest!  Otherwise everyone loses.

Cheers,
Rusty.
_______________________________________________
ccan mailing list
ccan@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/ccan