diff mbox

lib: fwts_cpu: fix memory leak and incorrect return (LP: #1549723)

Message ID 1456398667-15907-1-git-send-email-colin.king@canonical.com
State Accepted
Headers show

Commit Message

Colin Ian King Feb. 25, 2016, 11:11 a.m. UTC 
From: Colin Ian King <colin.king@canonical.com>

Latest CoverityScan run picked up a minor memory leak:

if (fwts_cpu_readmsr(0, MSR_AMD64_OSVW_STATUS, &val) != FWTS_OK)
   CID 1351543 (#2 of 2): Resource leak (RESOURCE_LEAK)
   leaked_storage: Variable cpu going out of scope leaks the storage
   it points to.

While fixing this bug I noticed that the case when family 0x0f
models < rev F do not have C1E the code was returning FWTS_TRUE and
should be returning FWTS_FALSE.

Also, for non-x86 builds, skip all the x86 specific checks and always
return FWTS_FALSE.

Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 src/lib/src/fwts_cpu.c | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

Comments

Alex Hung Feb. 26, 2016, 7:30 a.m. UTC | #1 
On 02/25/2016 07:11 PM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> Latest CoverityScan run picked up a minor memory leak:
>
> if (fwts_cpu_readmsr(0, MSR_AMD64_OSVW_STATUS, &val) != FWTS_OK)
>     CID 1351543 (#2 of 2): Resource leak (RESOURCE_LEAK)
>     leaked_storage: Variable cpu going out of scope leaks the storage
>     it points to.
>
> While fixing this bug I noticed that the case when family 0x0f
> models < rev F do not have C1E the code was returning FWTS_TRUE and
> should be returning FWTS_FALSE.
>
> Also, for non-x86 builds, skip all the x86 specific checks and always
> return FWTS_FALSE.
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>   src/lib/src/fwts_cpu.c | 32 +++++++++++++++++++++-----------
>   1 file changed, 21 insertions(+), 11 deletions(-)
>
> diff --git a/src/lib/src/fwts_cpu.c b/src/lib/src/fwts_cpu.c
> index 7a4b5b2..bcdb89d 100644
> --- a/src/lib/src/fwts_cpu.c
> +++ b/src/lib/src/fwts_cpu.c
> @@ -195,22 +195,25 @@ int fwts_cpu_is_AMD(bool *is_amd)
>    */
>   fwts_bool fwts_cpu_has_c1e(void)
>   {
> +#if FWTS_ARCH_X86
>   	uint64_t val;
> +	fwts_bool rc = FWTS_FALSE;
>
>   	fwts_cpuinfo_x86 *cpu;
>
>   	if ((cpu = fwts_cpu_get_info(0)) == NULL)
>   		return FWTS_BOOL_ERROR;
>
> +	/* no C1E on AMD */
>           if (strstr(cpu->vendor_id, "AuthenticAMD") == NULL) {
> -		fwts_cpu_free_info(cpu);
> -		return FWTS_FALSE;
> +		rc = FWTS_FALSE;
> +		goto free_info;
>   	}
>
>           /* Family 0x0f models < rev F do not have C1E */
>           if (cpu->x86 == 0x0F && cpu->x86_model >= 0x40) {
> -		fwts_cpu_free_info(cpu);
> -                return FWTS_TRUE;
> +                rc = FWTS_FALSE;
> +		goto free_info;
>   	}
>
>           if (cpu->x86 == 0x10) {
> @@ -219,23 +222,30 @@ fwts_bool fwts_cpu_has_c1e(void)
>                    * by erratum #400
>                    */
>   		if (strstr(cpu->flags, "osvw") != NULL) {
> -			if (fwts_cpu_readmsr(0, MSR_AMD64_OSVW_ID_LENGTH, &val) != FWTS_OK)
> -				return FWTS_BOOL_ERROR;
> +			if (fwts_cpu_readmsr(0, MSR_AMD64_OSVW_ID_LENGTH, &val) != FWTS_OK) {
> +				rc = FWTS_BOOL_ERROR;
> +				goto free_info;
> +			}
>
>                           if (val >= 2) {
>                                   if (fwts_cpu_readmsr(0, MSR_AMD64_OSVW_STATUS, &val) != FWTS_OK)
> -					return FWTS_BOOL_ERROR;
> +					rc = FWTS_BOOL_ERROR;
> +					goto free_info;
>                                   if (!(val & 2)) {
> -					fwts_cpu_free_info(cpu);
> -					return FWTS_FALSE;
> +					rc = FWTS_FALSE;
> +					goto free_info;
>   				}
>                           }
>                   }
> -		fwts_cpu_free_info(cpu);
> -                return FWTS_TRUE;
> +                rc = FWTS_TRUE;
>           }
> +
> +free_info:
>   	fwts_cpu_free_info(cpu);
> +	return rc;
> +#else
>   	return FWTS_FALSE;
> +#endif
>   }
>
>   /*
>

Acked-by: Alex Hung <alex.hung@canonical.com>
Ivan Hu March 3, 2016, 9:14 a.m. UTC | #2 
On 2016年02月25日 19:11, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> Latest CoverityScan run picked up a minor memory leak:
>
> if (fwts_cpu_readmsr(0, MSR_AMD64_OSVW_STATUS, &val) != FWTS_OK)
>     CID 1351543 (#2 of 2): Resource leak (RESOURCE_LEAK)
>     leaked_storage: Variable cpu going out of scope leaks the storage
>     it points to.
>
> While fixing this bug I noticed that the case when family 0x0f
> models < rev F do not have C1E the code was returning FWTS_TRUE and
> should be returning FWTS_FALSE.
>
> Also, for non-x86 builds, skip all the x86 specific checks and always
> return FWTS_FALSE.
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>   src/lib/src/fwts_cpu.c | 32 +++++++++++++++++++++-----------
>   1 file changed, 21 insertions(+), 11 deletions(-)
>
> diff --git a/src/lib/src/fwts_cpu.c b/src/lib/src/fwts_cpu.c
> index 7a4b5b2..bcdb89d 100644
> --- a/src/lib/src/fwts_cpu.c
> +++ b/src/lib/src/fwts_cpu.c
> @@ -195,22 +195,25 @@ int fwts_cpu_is_AMD(bool *is_amd)
>    */
>   fwts_bool fwts_cpu_has_c1e(void)
>   {
> +#if FWTS_ARCH_X86
>   	uint64_t val;
> +	fwts_bool rc = FWTS_FALSE;
>
>   	fwts_cpuinfo_x86 *cpu;
>
>   	if ((cpu = fwts_cpu_get_info(0)) == NULL)
>   		return FWTS_BOOL_ERROR;
>
> +	/* no C1E on AMD */
>           if (strstr(cpu->vendor_id, "AuthenticAMD") == NULL) {
> -		fwts_cpu_free_info(cpu);
> -		return FWTS_FALSE;
> +		rc = FWTS_FALSE;
> +		goto free_info;
>   	}
>
>           /* Family 0x0f models < rev F do not have C1E */
>           if (cpu->x86 == 0x0F && cpu->x86_model >= 0x40) {
> -		fwts_cpu_free_info(cpu);
> -                return FWTS_TRUE;
> +                rc = FWTS_FALSE;
> +		goto free_info;
>   	}
>
>           if (cpu->x86 == 0x10) {
> @@ -219,23 +222,30 @@ fwts_bool fwts_cpu_has_c1e(void)
>                    * by erratum #400
>                    */
>   		if (strstr(cpu->flags, "osvw") != NULL) {
> -			if (fwts_cpu_readmsr(0, MSR_AMD64_OSVW_ID_LENGTH, &val) != FWTS_OK)
> -				return FWTS_BOOL_ERROR;
> +			if (fwts_cpu_readmsr(0, MSR_AMD64_OSVW_ID_LENGTH, &val) != FWTS_OK) {
> +				rc = FWTS_BOOL_ERROR;
> +				goto free_info;
> +			}
>
>                           if (val >= 2) {
>                                   if (fwts_cpu_readmsr(0, MSR_AMD64_OSVW_STATUS, &val) != FWTS_OK)
> -					return FWTS_BOOL_ERROR;
> +					rc = FWTS_BOOL_ERROR;
> +					goto free_info;
>                                   if (!(val & 2)) {
> -					fwts_cpu_free_info(cpu);
> -					return FWTS_FALSE;
> +					rc = FWTS_FALSE;
> +					goto free_info;
>   				}
>                           }
>                   }
> -		fwts_cpu_free_info(cpu);
> -                return FWTS_TRUE;
> +                rc = FWTS_TRUE;
>           }
> +
> +free_info:
>   	fwts_cpu_free_info(cpu);
> +	return rc;
> +#else
>   	return FWTS_FALSE;
> +#endif
>   }
>
>   /*
>

Acked-by: Ivan Hu <ivan.hu@canonical.com>
diff mbox

Patch

diff --git a/src/lib/src/fwts_cpu.c b/src/lib/src/fwts_cpu.c
index 7a4b5b2..bcdb89d 100644
--- a/src/lib/src/fwts_cpu.c
+++ b/src/lib/src/fwts_cpu.c
@@ -195,22 +195,25 @@  int fwts_cpu_is_AMD(bool *is_amd)
  */
 fwts_bool fwts_cpu_has_c1e(void)
 {
+#if FWTS_ARCH_X86
 	uint64_t val;
+	fwts_bool rc = FWTS_FALSE;
 
 	fwts_cpuinfo_x86 *cpu;
 
 	if ((cpu = fwts_cpu_get_info(0)) == NULL)
 		return FWTS_BOOL_ERROR;
 
+	/* no C1E on AMD */
         if (strstr(cpu->vendor_id, "AuthenticAMD") == NULL) {
-		fwts_cpu_free_info(cpu);
-		return FWTS_FALSE;
+		rc = FWTS_FALSE;
+		goto free_info;
 	}
 
         /* Family 0x0f models < rev F do not have C1E */
         if (cpu->x86 == 0x0F && cpu->x86_model >= 0x40) {
-		fwts_cpu_free_info(cpu);
-                return FWTS_TRUE;
+                rc = FWTS_FALSE;
+		goto free_info;
 	}
 
         if (cpu->x86 == 0x10) {
@@ -219,23 +222,30 @@  fwts_bool fwts_cpu_has_c1e(void)
                  * by erratum #400
                  */
 		if (strstr(cpu->flags, "osvw") != NULL) {
-			if (fwts_cpu_readmsr(0, MSR_AMD64_OSVW_ID_LENGTH, &val) != FWTS_OK)
-				return FWTS_BOOL_ERROR;
+			if (fwts_cpu_readmsr(0, MSR_AMD64_OSVW_ID_LENGTH, &val) != FWTS_OK) {
+				rc = FWTS_BOOL_ERROR;
+				goto free_info;
+			}
 
                         if (val >= 2) {
                                 if (fwts_cpu_readmsr(0, MSR_AMD64_OSVW_STATUS, &val) != FWTS_OK)
-					return FWTS_BOOL_ERROR;
+					rc = FWTS_BOOL_ERROR;
+					goto free_info;
                                 if (!(val & 2)) {
-					fwts_cpu_free_info(cpu);
-					return FWTS_FALSE;
+					rc = FWTS_FALSE;
+					goto free_info;
 				}
                         }
                 }
-		fwts_cpu_free_info(cpu);
-                return FWTS_TRUE;
+                rc = FWTS_TRUE;
         }
+
+free_info:
 	fwts_cpu_free_info(cpu);
+	return rc;
+#else
 	return FWTS_FALSE;
+#endif
 }
 
 /*