ACK: [PATCH] uefivarinfo: allocate buffer rewrite to avoid realloc failure (LP: #1362540)

[Colin Ian King]

On 15/09/14 07:34, Ivan Hu wrote:
> The data buffer which was brought into firmware seems to be changed that caused
> the realloc function failed below. Rewrite the function that allocate buffer
> with the max allowed variable size without reallocing the memory.
> 
> *** glibc detected *** fwts: realloc(): invalid next size: 0x00007f5e807a7080 **
> 
> Signed-off-by: Ivan Hu <ivan.hu at canonical.com>
> ---
>  src/uefi/uefivarinfo/uefivarinfo.c |   55 +++++++++++++++++++-----------------
>  1 file changed, 29 insertions(+), 26 deletions(-)
> 
> diff --git a/src/uefi/uefivarinfo/uefivarinfo.c b/src/uefi/uefivarinfo/uefivarinfo.c
> index 84e960b..a4d2783 100644
> --- a/src/uefi/uefivarinfo/uefivarinfo.c
> +++ b/src/uefi/uefivarinfo/uefivarinfo.c
> @@ -29,7 +29,6 @@
>  #include "fwts_efi_module.h"
>  
>  #define MAX_VARNAME_LENGTH	1024
> -#define DATA_LENGTH		1024
>  
>  static int fd;
>  
> @@ -66,7 +65,7 @@ static int do_checkvariables(
>  	fwts_framework *fw,
>  	uint64_t *usedvars,
>  	uint64_t *usedvarssize,
> -	uint64_t maxvarsize)
> +	const uint64_t maxvarsize)
>  {
>  	long ioret;
>  	uint64_t status;
> @@ -114,46 +113,50 @@ static int do_checkvariables(
>  
>  		(*usedvars)++;
>  
> -		data = malloc(DATA_LENGTH);
> +		data = malloc(maxvarsize);
>  		if (!data) {
>  			fwts_log_info(fw, "Failed to allocate memory for test.");
>  			return FWTS_ERROR;
>  		}
>  
> -		getdatasize = DATA_LENGTH;
> +		getdatasize = maxvarsize;
>  		getvariable.VariableName = variablename;
>  		getvariable.VendorGuid = &vendorguid;
>  		getvariable.DataSize = &getdatasize;
>  		getvariable.Data = data;
> -		while (true) {
> -			ioret = ioctl(fd, EFI_RUNTIME_GET_VARIABLE, &getvariable);
> -			if (ioret == -1) {
> -				if (status != EFI_BUFFER_TOO_SMALL) {
> -					fwts_log_info(fw, "Failed to get variable with UEFI runtime service.");
> -					fwts_uefi_print_status_info(fw, status);
> -					free(data);
> +
> +		ioret = ioctl(fd, EFI_RUNTIME_GET_VARIABLE, &getvariable);
> +		if (ioret == -1) {
> +			free(data);
> +			if (status != EFI_BUFFER_TOO_SMALL) {
> +				fwts_log_info(fw, "Failed to get variable with UEFI runtime service.");
> +				fwts_uefi_print_status_info(fw, status);
> +				return FWTS_ERROR;
> +			} else if (getdatasize > maxvarsize) {
> +				fwts_log_info(fw, "Variable is larger than maximum variable length.");
> +				fwts_uefi_print_status_info(fw, status);
> +
> +				/*
> +				 * Although the variable is larger than maximum variable length,
> +				 * still try to calculate the total sizes of the used variables.
> +				 */
> +				data = malloc(getdatasize);
> +				if (!data) {
> +					fwts_log_info(fw, "Failed to allocate memory for test.");
>  					return FWTS_ERROR;
>  				}
> -				if (getdatasize == maxvarsize) {
> -					fwts_log_info(fw, "Variable is larger than maximum variable length.");
> +
> +				getvariable.Data = data;
> +				ioret = ioctl(fd, EFI_RUNTIME_GET_VARIABLE, &getvariable);
> +				if (ioret == -1) {
> +					fwts_log_info(fw, "Failed to get variable with variable larger than maximum variable length.");
>  					fwts_uefi_print_status_info(fw, status);
>  					free(data);
>  					return FWTS_ERROR;
>  				}
> -				getdatasize += DATA_LENGTH;
> -				getdatasize = (getdatasize > maxvarsize) ? maxvarsize : getdatasize;
> -				data = realloc(data, getdatasize);
> -				if (data) {
> -					getvariable.DataSize = &getdatasize;
> -					getvariable.Data = data;
> -					continue;
> -				} else {
> -					fwts_log_info(fw, "Failed to allocate memory for test.");
> -					return FWTS_ERROR;
> -				}
>  			}
> -			break;
> -		};
> +		}
> +
>  		free(data);
>  
>  		(*usedvarssize) += getdatasize;
> 
Looks very reasonable to me, I've not tested it though.

Acked-by: Colin Ian King <colin.king at canonical.com>