mkeficapsule: Free up resources used for adding public key to dtb

Message ID 20210121115221.16058-1-sughosh.ganu@linaro.org
State New
Headers show
Series
  • mkeficapsule: Free up resources used for adding public key to dtb
Related show

Commit Message

Sughosh Ganu Jan. 21, 2021, 11:52 a.m.
Fix the issues flagged by Coverity on resources not being released in
the add_public_key function.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>

---
 tools/mkeficapsule.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

-- 
2.17.1

Comments

Heinrich Schuchardt Jan. 21, 2021, 1:32 p.m. | #1
On 21.01.21 12:52, Sughosh Ganu wrote:
> Fix the issues flagged by Coverity on resources not being released in

> the add_public_key function.

>

> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>

> ---

>  tools/mkeficapsule.c | 13 +++++++++++--

>  1 file changed, 11 insertions(+), 2 deletions(-)

>

> diff --git a/tools/mkeficapsule.c b/tools/mkeficapsule.c

> index 270943fc90..caf0a1b231 100644

> --- a/tools/mkeficapsule.c

> +++ b/tools/mkeficapsule.c

> @@ -150,6 +150,7 @@ static int add_public_key(const char *pkey_file, const char *dtb_file,

>  	if (srcfd == -1) {

>  		fprintf(stderr, "%s: Can't open %s: %s\n",

>  			__func__, pkey_file, strerror(errno));

> +		ret = -1;

>  		goto err;

>  	}

>

> @@ -157,6 +158,7 @@ static int add_public_key(const char *pkey_file, const char *dtb_file,

>  	if (ret == -1) {

>  		fprintf(stderr, "%s: Can't stat %s: %s\n",

>  			__func__, pkey_file, strerror(errno));

> +		ret = -1;

>  		goto err;

>  	}

>

> @@ -167,6 +169,7 @@ static int add_public_key(const char *pkey_file, const char *dtb_file,

>  	if ((sptr == MAP_FAILED) || (errno != 0)) {

>  		fprintf(stderr, "%s: Failed to mmap %s:%s\n",

>  			__func__, pkey_file, strerror(errno));

> +		ret = -1;

>  		goto err;

>  	}

>

> @@ -175,6 +178,7 @@ static int add_public_key(const char *pkey_file, const char *dtb_file,

>  	if (destfd == -1) {

>  		fprintf(stderr, "%s: Can't open %s: %s\n",

>  			__func__, dtb_file, strerror(errno));

> +		ret = -1;

>  		goto err;

>  	}

>

> @@ -189,6 +193,7 @@ static int add_public_key(const char *pkey_file, const char *dtb_file,

>  	if (ftruncate(destfd, dtb.st_size)) {

>  		fprintf(stderr, "%s: Can't expand %s: %s\n",

>  			__func__, dtb_file, strerror(errno));

> +		ret = -1;

>  		goto err;;


Thanks for providing the patch.

Please, remove the duplicate semicolon here.

main() should return the constant EXIT_FAILURE (glibc defines this as 1)
if a failure occurs, not -1.

scripts/checkpatch.pl -f tools/mkeficapsule.c
reports some other style issues:

CHECK: Unnecessary parentheses around 'sptr == MAP_FAILED'
#167: FILE: tools/mkeficapsule.c:167:
+       if ((sptr == MAP_FAILED) || (errno != 0)) {

CHECK: Unnecessary parentheses around 'errno != 0'
#167: FILE: tools/mkeficapsule.c:167:
+       if ((sptr == MAP_FAILED) || (errno != 0)) {

WARNING: Statements terminations use 1 semicolon
#192: FILE: tools/mkeficapsule.c:192:
+               goto err;;

CHECK: Unnecessary parentheses around 'dptr == MAP_FAILED'
#199: FILE: tools/mkeficapsule.c:199:
+       if ((dptr == MAP_FAILED) || (errno != 0)) {

CHECK: Unnecessary parentheses around 'errno != 0'
#199: FILE: tools/mkeficapsule.c:199:
+       if ((dptr == MAP_FAILED) || (errno != 0)) {


In U-Boot we prefer to avoid terms like (errno != 0) in a logical statement:

	if (dptr == MAP_FAILED || errno) {

But it is completely unnecessary to check errno at all here because
errno is only set if mmap() returns MAP_FAILED (see the mmap()
man-page). So the line should be:

192
	if (dptr == MAP_FAILED) {

Now you can remove the following line:

195:
-	errno = 0;

Best regards

Heinrich

>  	}

>

> @@ -199,11 +204,13 @@ static int add_public_key(const char *pkey_file, const char *dtb_file,

>  	if ((dptr == MAP_FAILED) || (errno != 0)) {

>  		fprintf(stderr, "%s: Failed to mmap %s:%s\n",

>  			__func__, dtb_file, strerror(errno));

> +		ret = -1;

>  		goto err;

>  	}

>

>  	if (fdt_check_header(dptr)) {

>  		fprintf(stderr, "%s: Invalid FDT header\n", __func__);

> +		ret = -1;

>  		goto err;

>  	}

>

> @@ -211,6 +218,7 @@ static int add_public_key(const char *pkey_file, const char *dtb_file,

>  	if (ret) {

>  		fprintf(stderr, "%s: Cannot expand FDT: %s\n",

>  			__func__, fdt_strerror(ret));

> +		ret = -1;

>  		goto err;

>  	}

>

> @@ -219,10 +227,11 @@ static int add_public_key(const char *pkey_file, const char *dtb_file,

>  	if (ret < 0) {

>  		fprintf(stderr, "%s: Unable to add public key to the FDT\n",

>  			__func__);

> +		ret = -1;

>  		goto err;

>  	}

>

> -	return 0;

> +	ret = 0;

>

>  err:

>  	if (sptr)

> @@ -237,7 +246,7 @@ err:

>  	if (destfd >= 0)

>  		close(destfd);

>

> -	return -1;

> +	return ret;

>  }

>

>  static int create_fwbin(char *path, char *bin, efi_guid_t *guid,

>

Patch

diff --git a/tools/mkeficapsule.c b/tools/mkeficapsule.c
index 270943fc90..caf0a1b231 100644
--- a/tools/mkeficapsule.c
+++ b/tools/mkeficapsule.c
@@ -150,6 +150,7 @@  static int add_public_key(const char *pkey_file, const char *dtb_file,
 	if (srcfd == -1) {
 		fprintf(stderr, "%s: Can't open %s: %s\n",
 			__func__, pkey_file, strerror(errno));
+		ret = -1;
 		goto err;
 	}
 
@@ -157,6 +158,7 @@  static int add_public_key(const char *pkey_file, const char *dtb_file,
 	if (ret == -1) {
 		fprintf(stderr, "%s: Can't stat %s: %s\n",
 			__func__, pkey_file, strerror(errno));
+		ret = -1;
 		goto err;
 	}
 
@@ -167,6 +169,7 @@  static int add_public_key(const char *pkey_file, const char *dtb_file,
 	if ((sptr == MAP_FAILED) || (errno != 0)) {
 		fprintf(stderr, "%s: Failed to mmap %s:%s\n",
 			__func__, pkey_file, strerror(errno));
+		ret = -1;
 		goto err;
 	}
 
@@ -175,6 +178,7 @@  static int add_public_key(const char *pkey_file, const char *dtb_file,
 	if (destfd == -1) {
 		fprintf(stderr, "%s: Can't open %s: %s\n",
 			__func__, dtb_file, strerror(errno));
+		ret = -1;
 		goto err;
 	}
 
@@ -189,6 +193,7 @@  static int add_public_key(const char *pkey_file, const char *dtb_file,
 	if (ftruncate(destfd, dtb.st_size)) {
 		fprintf(stderr, "%s: Can't expand %s: %s\n",
 			__func__, dtb_file, strerror(errno));
+		ret = -1;
 		goto err;;
 	}
 
@@ -199,11 +204,13 @@  static int add_public_key(const char *pkey_file, const char *dtb_file,
 	if ((dptr == MAP_FAILED) || (errno != 0)) {
 		fprintf(stderr, "%s: Failed to mmap %s:%s\n",
 			__func__, dtb_file, strerror(errno));
+		ret = -1;
 		goto err;
 	}
 
 	if (fdt_check_header(dptr)) {
 		fprintf(stderr, "%s: Invalid FDT header\n", __func__);
+		ret = -1;
 		goto err;
 	}
 
@@ -211,6 +218,7 @@  static int add_public_key(const char *pkey_file, const char *dtb_file,
 	if (ret) {
 		fprintf(stderr, "%s: Cannot expand FDT: %s\n",
 			__func__, fdt_strerror(ret));
+		ret = -1;
 		goto err;
 	}
 
@@ -219,10 +227,11 @@  static int add_public_key(const char *pkey_file, const char *dtb_file,
 	if (ret < 0) {
 		fprintf(stderr, "%s: Unable to add public key to the FDT\n",
 			__func__);
+		ret = -1;
 		goto err;
 	}
 
-	return 0;
+	ret = 0;
 
 err:
 	if (sptr)
@@ -237,7 +246,7 @@  err:
 	if (destfd >= 0)
 		close(destfd);
 
-	return -1;
+	return ret;
 }
 
 static int create_fwbin(char *path, char *bin, efi_guid_t *guid,