diff mbox

uefirtvariable: fix decalred VLA have zero size (LP: #1526815)

Message ID 1450339904-23908-1-git-send-email-ivan.hu@canonical.com
State Rejected
Headers show

Commit Message

Ivan Hu Dec. 17, 2015, 8:11 a.m. UTC 
Static analysis from clang scan-build found a declared variable-length
array(VLA) has zero size, setvariable_insertvariable() declares data[datasize],
however, it is called when delete a variable as follows:
ret = setvariable_insertvariable(fw, attributes, 0, variablenametest,
&gtestguid1, datadiff);
so got a declared data[0] declared.

Signed-off-by: Ivan Hu <ivan.hu@canonical.com>
---
 src/uefi/uefirtvariable/uefirtvariable.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

Comments

Alex Hung Dec. 22, 2015, 6:33 a.m. UTC | #1 
On 12/17/2015 04:11 PM, Ivan Hu wrote:
> Static analysis from clang scan-build found a declared variable-length
> array(VLA) has zero size, setvariable_insertvariable() declares data[datasize],
> however, it is called when delete a variable as follows:
> ret = setvariable_insertvariable(fw, attributes, 0, variablenametest,
> &gtestguid1, datadiff);
> so got a declared data[0] declared.
>
> Signed-off-by: Ivan Hu <ivan.hu@canonical.com>
> ---
>   src/uefi/uefirtvariable/uefirtvariable.c | 15 ++++++++++++++-
>   1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/src/uefi/uefirtvariable/uefirtvariable.c b/src/uefi/uefirtvariable/uefirtvariable.c
> index b3c7559..27aa9a1 100644
> --- a/src/uefi/uefirtvariable/uefirtvariable.c
> +++ b/src/uefi/uefirtvariable/uefirtvariable.c
> @@ -867,7 +867,11 @@ static int setvariable_insertvariable(
>   	uint64_t status;
>   	uint64_t dataindex;
>
> -	uint8_t data[datasize];
> +	uint8_t *data = malloc (datasize);
> +	if ((datasize != 0) && !data) {
> +		fwts_skipped(fw, "Unable to alloc memory for data");
> +		return FWTS_SKIP;
> +	}
>
>   	for (dataindex = 0; dataindex < datasize; dataindex++)
>   		data[dataindex] = (uint8_t)dataindex + datadiff;
> @@ -892,6 +896,8 @@ static int setvariable_insertvariable(
>   				"with EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS or "
>   				"EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS "
>   				"EFI_VARIABLE_APPEND_WRITE attributes is set.");
> +			if (data)
> +				free(data);
>   			return FWTS_SKIP;
>   		}
>   		if (datasize == 0)
> @@ -910,6 +916,8 @@ static int setvariable_insertvariable(
>   					"after rebooting. Reboot and test "
>   					"again may be helpful to continue "
>   					"the test.");
> +				if (data)
> +					free(data);
>   				return FWTS_SKIP;
>   			}
>   			fwts_failed(fw, LOG_LEVEL_HIGH,
> @@ -918,8 +926,13 @@ static int setvariable_insertvariable(
>   				"runtime service.");
>   		}
>   		fwts_uefi_print_status_info(fw, status);
> +		if (data)
> +			free(data);
>   		return FWTS_ERROR;
>   	}
> +
> +	if (data)
> +		free(data);
>   	return FWTS_OK;
>   }
>
>

Acked-by: Alex Hung <alex.hung@canonical.com>
Colin Ian King Jan. 4, 2016, 10:18 a.m. UTC | #2 
On 17/12/15 08:11, Ivan Hu wrote:
> Static analysis from clang scan-build found a declared variable-length
> array(VLA) has zero size, setvariable_insertvariable() declares data[datasize],
> however, it is called when delete a variable as follows:
> ret = setvariable_insertvariable(fw, attributes, 0, variablenametest,
> &gtestguid1, datadiff);
> so got a declared data[0] declared.
> 
> Signed-off-by: Ivan Hu <ivan.hu@canonical.com>
> ---
>  src/uefi/uefirtvariable/uefirtvariable.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/src/uefi/uefirtvariable/uefirtvariable.c b/src/uefi/uefirtvariable/uefirtvariable.c
> index b3c7559..27aa9a1 100644
> --- a/src/uefi/uefirtvariable/uefirtvariable.c
> +++ b/src/uefi/uefirtvariable/uefirtvariable.c
> @@ -867,7 +867,11 @@ static int setvariable_insertvariable(
>  	uint64_t status;
>  	uint64_t dataindex;
>  
> -	uint8_t data[datasize];
> +	uint8_t *data = malloc (datasize);

C standard states:

"If the space cannot be allocated, a null pointer is returned. If the
size of the space requested is zero, the behavior is implementation
defined: either a null pointer is returned, or the behavior is as if the
size were some nonzero value, except that the returned pointer shall not
be used to access an object."

So, I'm not entirely sure of this is legitimate code or not as were
using a implementation specific behaviour of malloc(0) here.


> +	if ((datasize != 0) && !data) {
> +		fwts_skipped(fw, "Unable to alloc memory for data");
> +		return FWTS_SKIP;
> +	}
>  
>  	for (dataindex = 0; dataindex < datasize; dataindex++)
>  		data[dataindex] = (uint8_t)dataindex + datadiff;
> @@ -892,6 +896,8 @@ static int setvariable_insertvariable(
>  				"with EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS or "
>  				"EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS "
>  				"EFI_VARIABLE_APPEND_WRITE attributes is set.");
> +			if (data)
> +				free(data);
>  			return FWTS_SKIP;
>  		}
>  		if (datasize == 0)
> @@ -910,6 +916,8 @@ static int setvariable_insertvariable(
>  					"after rebooting. Reboot and test "
>  					"again may be helpful to continue "
>  					"the test.");
> +				if (data)
> +					free(data);
>  				return FWTS_SKIP;
>  			}
>  			fwts_failed(fw, LOG_LEVEL_HIGH,
> @@ -918,8 +926,13 @@ static int setvariable_insertvariable(
>  				"runtime service.");
>  		}
>  		fwts_uefi_print_status_info(fw, status);
> +		if (data)
> +			free(data);
>  		return FWTS_ERROR;
>  	}
> +
> +	if (data)
> +		free(data);
>  	return FWTS_OK;
>  }
>  
>
Colin Ian King Jan. 4, 2016, 1:10 p.m. UTC | #3 
On 04/01/16 10:18, Colin Ian King wrote:
> On 17/12/15 08:11, Ivan Hu wrote:
>> Static analysis from clang scan-build found a declared variable-length
>> array(VLA) has zero size, setvariable_insertvariable() declares data[datasize],
>> however, it is called when delete a variable as follows:
>> ret = setvariable_insertvariable(fw, attributes, 0, variablenametest,
>> &gtestguid1, datadiff);
>> so got a declared data[0] declared.
>>
>> Signed-off-by: Ivan Hu <ivan.hu@canonical.com>
>> ---
>>  src/uefi/uefirtvariable/uefirtvariable.c | 15 ++++++++++++++-
>>  1 file changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/uefi/uefirtvariable/uefirtvariable.c b/src/uefi/uefirtvariable/uefirtvariable.c
>> index b3c7559..27aa9a1 100644
>> --- a/src/uefi/uefirtvariable/uefirtvariable.c
>> +++ b/src/uefi/uefirtvariable/uefirtvariable.c
>> @@ -867,7 +867,11 @@ static int setvariable_insertvariable(
>>  	uint64_t status;
>>  	uint64_t dataindex;
>>  
>> -	uint8_t data[datasize];
>> +	uint8_t *data = malloc (datasize);
> 
> C standard states:
> 
> "If the space cannot be allocated, a null pointer is returned. If the
> size of the space requested is zero, the behavior is implementation
> defined: either a null pointer is returned, or the behavior is as if the
> size were some nonzero value, except that the returned pointer shall not
> be used to access an object."
> 
> So, I'm not entirely sure of this is legitimate code or not as were
> using a implementation specific behaviour of malloc(0) here.

uefi/uefirtvariable/uefirtvariable.c:870:18: warning: Call to 'malloc'
has an allocation size of 0 bytes
        uint8_t *data = malloc (datasize);
                        ^~~~~~~~~~~~~~~~~

clang scan-build also doesn't like it :-(
> 
> 
>> +	if ((datasize != 0) && !data) {
>> +		fwts_skipped(fw, "Unable to alloc memory for data");
>> +		return FWTS_SKIP;
>> +	}
>>  
>>  	for (dataindex = 0; dataindex < datasize; dataindex++)
>>  		data[dataindex] = (uint8_t)dataindex + datadiff;
>> @@ -892,6 +896,8 @@ static int setvariable_insertvariable(
>>  				"with EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS or "
>>  				"EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS "
>>  				"EFI_VARIABLE_APPEND_WRITE attributes is set.");
>> +			if (data)
>> +				free(data);
>>  			return FWTS_SKIP;
>>  		}
>>  		if (datasize == 0)
>> @@ -910,6 +916,8 @@ static int setvariable_insertvariable(
>>  					"after rebooting. Reboot and test "
>>  					"again may be helpful to continue "
>>  					"the test.");
>> +				if (data)
>> +					free(data);
>>  				return FWTS_SKIP;
>>  			}
>>  			fwts_failed(fw, LOG_LEVEL_HIGH,
>> @@ -918,8 +926,13 @@ static int setvariable_insertvariable(
>>  				"runtime service.");
>>  		}
>>  		fwts_uefi_print_status_info(fw, status);
>> +		if (data)
>> +			free(data);
>>  		return FWTS_ERROR;
>>  	}
>> +
>> +	if (data)
>> +		free(data);
>>  	return FWTS_OK;
>>  }
>>  
>>
>
diff mbox

Patch

diff --git a/src/uefi/uefirtvariable/uefirtvariable.c b/src/uefi/uefirtvariable/uefirtvariable.c
index b3c7559..27aa9a1 100644
--- a/src/uefi/uefirtvariable/uefirtvariable.c
+++ b/src/uefi/uefirtvariable/uefirtvariable.c
@@ -867,7 +867,11 @@  static int setvariable_insertvariable(
 	uint64_t status;
 	uint64_t dataindex;
 
-	uint8_t data[datasize];
+	uint8_t *data = malloc (datasize);
+	if ((datasize != 0) && !data) {
+		fwts_skipped(fw, "Unable to alloc memory for data");
+		return FWTS_SKIP;
+	}
 
 	for (dataindex = 0; dataindex < datasize; dataindex++)
 		data[dataindex] = (uint8_t)dataindex + datadiff;
@@ -892,6 +896,8 @@  static int setvariable_insertvariable(
 				"with EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS or "
 				"EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS "
 				"EFI_VARIABLE_APPEND_WRITE attributes is set.");
+			if (data)
+				free(data);
 			return FWTS_SKIP;
 		}
 		if (datasize == 0)
@@ -910,6 +916,8 @@  static int setvariable_insertvariable(
 					"after rebooting. Reboot and test "
 					"again may be helpful to continue "
 					"the test.");
+				if (data)
+					free(data);
 				return FWTS_SKIP;
 			}
 			fwts_failed(fw, LOG_LEVEL_HIGH,
@@ -918,8 +926,13 @@  static int setvariable_insertvariable(
 				"runtime service.");
 		}
 		fwts_uefi_print_status_info(fw, status);
+		if (data)
+			free(data);
 		return FWTS_ERROR;
 	}
+
+	if (data)
+		free(data);
 	return FWTS_OK;
 }