[PATCH 1/5] configure.ac: Don't search for c++ compiler

[PATCH 1/5] configure.ac: Don't search for c++ compiler

[Patrick Georgi]

There is no C++ code to be compiled in the repository.

Signed-off-by: Patrick Georgi <patrick-U1IrdZU7gScKt1dFYRw6Gg@public.gmane.org>
---
 configure.ac | 1 -
 1 file changed, 1 deletion(-)

diff --git a/configure.ac b/configure.ac
index cc3999e..c720f93 100644
--- a/configure.ac
+++ b/configure.ac
@@ -8,7 +8,6 @@ AC_CONFIG_SRCDIR([src/cbootimage.c])
 AC_CONFIG_HEADERS([config.h])
 
 # Checks for programs.
-AC_PROG_CXX
 AC_PROG_CC
 AM_PROG_CC_C_O
 PKG_PROG_PKG_CONFIG
-- 
1.9.1

[PATCH 2/5] cbootimage: simplify code

[Patrick Georgi]

Testing for e == 0 after exiting the function in any other
case a couple of lines earlier is useless.

Found-by: Coverity Scan
Signed-off-by: Patrick Georgi <patrick-U1IrdZU7gScKt1dFYRw6Gg@public.gmane.org>
---
 src/cbootimage.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/cbootimage.c b/src/cbootimage.c
index 5f746e3..62328b4 100644
--- a/src/cbootimage.c
+++ b/src/cbootimage.c
@@ -211,7 +211,7 @@ main(int argc, char *argv[])
 
 	if (enable_debug) {
 		/* Debugging information... */
-		printf("bct size: %d\n", e == 0 ? context.bct_size : -1);
+		printf("bct size: %d\n", context.bct_size);
 	}
 
 	/* Open the raw output file. */
-- 
1.9.1

[PATCH 3/5] data_layout: improve memory handling

[Patrick Georgi]

- free empty_blk if it's allocated and there's an error
- only free empty_blk if it's non-NULL. While POSIX
  requests such free()s to be safe, some implementations
  (eg Solaris) aren't compliant.

Found-by: Coverity Scan
Signed-off-by: Patrick Georgi <patrick-U1IrdZU7gScKt1dFYRw6Gg@public.gmane.org>
---
 src/data_layout.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/data_layout.c b/src/data_layout.c
index 01f00ab..db0a0f0 100644
--- a/src/data_layout.c
+++ b/src/data_layout.c
@@ -1004,12 +1004,14 @@ write_block_raw(build_image_context *context)
 		{
 			size_t bytes = pages_to_write * context->page_size;
 
-			if (fwrite(data, 1, bytes, context->raw_file) != bytes)
+			if (fwrite(data, 1, bytes, context->raw_file) != bytes) {
+				if (empty_blk) free(empty_blk);
 				return -1;
+			}
 		}
 	}
 
-	free(empty_blk);
+	if (empty_blk) free(empty_blk);
 	return 0;
 }
 
-- 
1.9.1

[PATCH 4/5] set: check seek success

[Patrick Georgi]

This could silently fail which leads to surprising behaviour.

Found-by: Coverity Scan
Signed-off-by: Patrick Georgi <patrick-U1IrdZU7gScKt1dFYRw6Gg@public.gmane.org>
---
 src/set.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/set.c b/src/set.c
index ff32b53..907d640 100644
--- a/src/set.c
+++ b/src/set.c
@@ -59,7 +59,11 @@ read_from_image(char	*filename,
 		return result;
 	}
 
-	fseek(fp, offset, SEEK_SET);
+	if (!fseek(fp, offset, SEEK_SET)) {
+		printf("Error: Couldn't seek to %s(%d)\n", filename, offset);
+		result = 1;
+		goto cleanup;
+	}
 
 	if (stat(filename, &stats) != 0) {
 		printf("Error: Unable to query info on bootloader path %s\n",
-- 
1.9.1

[PATCH 5/5] data_layout: fail better on file access errors

[Patrick Georgi]

fread could return only a partial result
(eg. NVBOOT_CONFIG_TABLE_SIZE_MAX - 1 bytes),
which right now would be accepted and only
resolved by later code.

Found-by: Coverity Scan
Signed-off-by: Patrick Georgi <patrick-U1IrdZU7gScKt1dFYRw6Gg@public.gmane.org>
---
 src/data_layout.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/data_layout.c b/src/data_layout.c
index db0a0f0..11a0761 100644
--- a/src/data_layout.c
+++ b/src/data_layout.c
@@ -1050,7 +1050,7 @@ int get_bct_size_from_image(build_image_context *context)
 	if (!fp)
 		return ENODATA;
 
-	if (!fread(buffer, 1, NVBOOT_CONFIG_TABLE_SIZE_MAX, fp)) {
+	if (fread(buffer, 1, NVBOOT_CONFIG_TABLE_SIZE_MAX, fp) != NVBOOT_CONFIG_TABLE_SIZE_MAX) {
 		fclose(fp);
 		return ENODATA;
 	}
-- 
1.9.1

Re: [PATCH 4/5] set: check seek success

[Stephen Warren]

On 10/02/2014 02:16 AM, Patrick Georgi wrote:
> This could silently fail which leads to surprising behaviour.

> diff --git a/src/set.c b/src/set.c

> -	fseek(fp, offset, SEEK_SET);
> +	if (!fseek(fp, offset, SEEK_SET)) {

fseek is supposed to return the current offset, or -1 on error. I think 
that should say if (fseek(...) < 0) or if (fseek(...) == -1)) or if 
(fseek(...) != offset).

Re: [PATCH 1/5] configure.ac: Don't search for c++ compiler

[Stephen Warren]

On 10/02/2014 02:16 AM, Patrick Georgi wrote:
> There is no C++ code to be compiled in the repository.

I've applied patches 1, 2, 3, 5. Thanks!

Re: [PATCH 4/5] set: check seek success

[Patrick Georgi]

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

Am 03.10.2014 um 00:58 schrieb Stephen Warren:
> fseek is supposed to return the current offset, or -1 on error. I 
The man page on my system disagrees: "Upon successful completion, fgetpos(), fseek(), fsetpos() return 0, and ftell() returns the current offset.   Otherwise,  -1  is  returned and errno is set to indicate the error."


Patrick


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 4/5] set: check seek success

[Stephen Warren]

On 10/03/2014 12:54 AM, Patrick Georgi wrote:
> Am 03.10.2014 um 00:58 schrieb Stephen Warren:
>> fseek is supposed to return the current offset, or -1 on error. I
 >
> The man page on my system disagrees: "Upon successful completion, fgetpos(), fseek(), fsetpos() return 0, and ftell() returns the current offset.   Otherwise,  -1  is  returned and errno is set to indicate the error."

That seems to exactly match what I said.

[PATCH 4/5 v2] set: check seek success

[Patrick Georgi]

This could silently fail which leads to surprising behaviour.

Found-by: Coverity Scan
Signed-off-by: Patrick Georgi <patrick-U1IrdZU7gScKt1dFYRw6Gg@public.gmane.org>
---
 src/set.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/set.c b/src/set.c
index ff32b53..907d640 100644
--- a/src/set.c
+++ b/src/set.c
@@ -59,7 +59,11 @@ read_from_image(char	*filename,
 		return result;
 	}
 
-	fseek(fp, offset, SEEK_SET);
+	if (fseek(fp, offset, SEEK_SET) == -1) {
+		printf("Error: Couldn't seek to %s(%d)\n", filename, offset);
+		result = 1;
+		goto cleanup;
+	}
 
 	if (stat(filename, &stats) != 0) {
 		printf("Error: Unable to query info on bootloader path %s\n",
-- 
1.9.1

Re: [PATCH 4/5 v2] set: check seek success

[Thierry Reding]

On November 8, 2014 11:22:54 PM Patrick Georgi <patrick-U1IrdZU7gScKt1dFYRw6Gg@public.gmane.org> wrote:
> This could silently fail which leads to surprising behaviour.
>
> Found-by: Coverity Scan
> Signed-off-by: Patrick Georgi <patrick-U1IrdZU7gScKt1dFYRw6Gg@public.gmane.org>
> ---
>  src/set.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)

This clearly isn't a patch against the Linux kernel, so it'd be good to
clarify what repository it should be applied to so people don't have
to guess.

Thierry

Re: [PATCH 4/5 v2] set: check seek success

[Patrick Georgi]

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



Am 09.11.2014 um 02:04 schrieb Thierry Reding:
> On November 8, 2014 11:22:54 PM Patrick Georgi <patrick-U1IrdZU7gScKt1dFYRw6Gg@public.gmane.org> wrote:
>> This could silently fail which leads to surprising behaviour.
>>
>> Found-by: Coverity Scan
>> Signed-off-by: Patrick Georgi <patrick-U1IrdZU7gScKt1dFYRw6Gg@public.gmane.org>
>> ---
>>  src/set.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> This clearly isn't a patch against the Linux kernel, so it'd be good to
> clarify what repository it should be applied to so people don't have
> to guess.
Oops, I'm sorry. This is for https://github.com/NVIDIA/cbootimage


Thanks,
Patrick


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 4/5 v2] set: check seek success

[Thierry Reding]

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

On Sat, Nov 08, 2014 at 11:14:09PM +0100, Patrick Georgi wrote:
> This could silently fail which leads to surprising behaviour.
> 
> Found-by: Coverity Scan
> Signed-off-by: Patrick Georgi <patrick-U1IrdZU7gScKt1dFYRw6Gg@public.gmane.org>
> ---
>  src/set.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/src/set.c b/src/set.c
> index ff32b53..907d640 100644
> --- a/src/set.c
> +++ b/src/set.c
> @@ -59,7 +59,11 @@ read_from_image(char	*filename,
>  		return result;
>  	}
>  
> -	fseek(fp, offset, SEEK_SET);
> +	if (fseek(fp, offset, SEEK_SET) == -1) {
> +		printf("Error: Couldn't seek to %s(%d)\n", filename, offset);

offset is unsigned, so the format specifier should be %u. I also wonder
whether it would be good to output errno along with the message (or the
string representation thereof) to increase the information content. But
given that none of the other error messages have that it could be a
separate patch.

With the %d -> %u for the format specifier, this is:

Reviewed-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

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

Re: [PATCH 4/5 v2] set: check seek success

[Thierry Reding]

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

On Mon, Nov 10, 2014 at 09:51:03AM +0100, Thierry Reding wrote:
> On Sat, Nov 08, 2014 at 11:14:09PM +0100, Patrick Georgi wrote:
> > This could silently fail which leads to surprising behaviour.
> > 
> > Found-by: Coverity Scan
> > Signed-off-by: Patrick Georgi <patrick-U1IrdZU7gScKt1dFYRw6Gg@public.gmane.org>
> > ---
> >  src/set.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/set.c b/src/set.c
> > index ff32b53..907d640 100644
> > --- a/src/set.c
> > +++ b/src/set.c
> > @@ -59,7 +59,11 @@ read_from_image(char	*filename,
> >  		return result;
> >  	}
> >  
> > -	fseek(fp, offset, SEEK_SET);
> > +	if (fseek(fp, offset, SEEK_SET) == -1) {
> > +		printf("Error: Couldn't seek to %s(%d)\n", filename, offset);
> 
> offset is unsigned, so the format specifier should be %u. I also wonder
> whether it would be good to output errno along with the message (or the
> string representation thereof) to increase the information content. But
> given that none of the other error messages have that it could be a
> separate patch.
> 
> With the %d -> %u for the format specifier, this is:
> 
> Reviewed-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

I just realized that I have commit access to the cbootimage repository,
so I just made the %d -> %u change myself and pushed.

Thanks,
Thierry

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

Re: [PATCH 4/5 v2] set: check seek success

[Stephen Warren]

On 11/10/2014 01:51 AM, Thierry Reding wrote:
> On Sat, Nov 08, 2014 at 11:14:09PM +0100, Patrick Georgi wrote:
>> This could silently fail which leads to surprising behaviour.
>>
>> Found-by: Coverity Scan
>> Signed-off-by: Patrick Georgi <patrick-U1IrdZU7gScKt1dFYRw6Gg@public.gmane.org>
>> ---
>>   src/set.c | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/set.c b/src/set.c
>> index ff32b53..907d640 100644
>> --- a/src/set.c
>> +++ b/src/set.c
>> @@ -59,7 +59,11 @@ read_from_image(char	*filename,
>>   		return result;
>>   	}
>>
>> -	fseek(fp, offset, SEEK_SET);
>> +	if (fseek(fp, offset, SEEK_SET) == -1) {
>> +		printf("Error: Couldn't seek to %s(%d)\n", filename, offset);
>
> offset is unsigned, so the format specifier should be %u. I also wonder
> whether it would be good to output errno along with the message (or the
> string representation thereof) to increase the information content. But
> given that none of the other error messages have that it could be a
> separate patch.

Using perror() might be useful?

Re: [PATCH 4/5 v2] set: check seek success

[Thierry Reding]

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

On Mon, Nov 10, 2014 at 09:31:55AM -0700, Stephen Warren wrote:
> On 11/10/2014 01:51 AM, Thierry Reding wrote:
> >On Sat, Nov 08, 2014 at 11:14:09PM +0100, Patrick Georgi wrote:
> >>This could silently fail which leads to surprising behaviour.
> >>
> >>Found-by: Coverity Scan
> >>Signed-off-by: Patrick Georgi <patrick-U1IrdZU7gScKt1dFYRw6Gg@public.gmane.org>
> >>---
> >>  src/set.c | 6 +++++-
> >>  1 file changed, 5 insertions(+), 1 deletion(-)
> >>
> >>diff --git a/src/set.c b/src/set.c
> >>index ff32b53..907d640 100644
> >>--- a/src/set.c
> >>+++ b/src/set.c
> >>@@ -59,7 +59,11 @@ read_from_image(char	*filename,
> >>  		return result;
> >>  	}
> >>
> >>-	fseek(fp, offset, SEEK_SET);
> >>+	if (fseek(fp, offset, SEEK_SET) == -1) {
> >>+		printf("Error: Couldn't seek to %s(%d)\n", filename, offset);
> >
> >offset is unsigned, so the format specifier should be %u. I also wonder
> >whether it would be good to output errno along with the message (or the
> >string representation thereof) to increase the information content. But
> >given that none of the other error messages have that it could be a
> >separate patch.
> 
> Using perror() might be useful?

Indeed, that would also make these messages go to stderr instead of
stdout and be more idiomatic.

Thierry

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