Re: [PATCH v4 3/4] device property: constify property arrays values

[Andy Shevchenko]

On Thu, 2017-02-02 at 08:39 -0800, Dmitry Torokhov wrote:
> From: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
> 
> Data that is fed into property arrays should not be modified, so let's
> mark
> relevant pointers as const. This will allow us making source arrays as
> const/__initconst.
> 

> Also fix memory leaks on errors in property_entry_copy().

While the code looks okay, I'm not sure what memory leaks you are
referring to. The idea as far as I remember was to run *free() function
if *copy() fails.

> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
> ---
> 
> Not sending the rest of the series as to not clutter mailing lists too
> much...
> 
> Âdrivers/base/property.cÂÂ|ÂÂÂ66 +++++++++++++++++++++++++++++++----
> -----------
> Âinclude/linux/property.h |ÂÂÂ12 ++++----
> Â2 files changed, 51 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index edc09854520b..09fb9757e086 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -682,44 +682,64 @@ int fwnode_property_match_string(struct
> fwnode_handle *fwnode,
> Â}
> ÂEXPORT_SYMBOL_GPL(fwnode_property_match_string);
> Â
> +static int property_copy_string_array(struct property_entry *dst,
> +				ÂÂÂÂÂÂconst struct property_entry
> *src)
> +{
> +	char **d;
> +	size_t nval = src->length / sizeof(*d);
> +	size_t i;
> +
> +	d = kcalloc(nval, sizeof(*d), GFP_KERNEL);
> +	if (!d)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < nval; i++) {
> +		d[i] = kstrdup(src->pointer.str[i], GFP_KERNEL);
> +		if (!d[i] && src->pointer.str[i]) {
> +			while (--i >= 0)
> +				kfree(d[i]);
> +			kfree(d);
> +			return -ENOMEM;
> +		}
> +	}
> +
> +	dst->pointer.str = (void *)d;
> +	return 0;
> +}
> +
> Âstatic int property_entry_copy(struct property_entry *dst,
> Â			ÂÂÂÂÂÂÂconst struct property_entry *src)
> Â{
> -	const char **d, **s;
> -	size_t i, nval;
> +	int error;
> Â
> Â	dst->name = kstrdup(src->name, GFP_KERNEL);
> Â	if (!dst->name)
> Â		return -ENOMEM;
> Â
> Â	if (src->is_array) {
> -		if (!src->length)
> -			return -ENODATA;
> +		if (!src->length) {
> +			error = -ENODATA;
> +			goto out_free_name;
> +		}
> Â
> Â		if (src->is_string) {
> -			nval = src->length / sizeof(const char *);
> -			dst->pointer.str = kcalloc(nval, sizeof(const
> char *),
> -						ÂÂÂGFP_KERNEL);
> -			if (!dst->pointer.str)
> -				return -ENOMEM;
> -
> -			d = dst->pointer.str;
> -			s = src->pointer.str;
> -			for (i = 0; i < nval; i++) {
> -				d[i] = kstrdup(s[i], GFP_KERNEL);
> -				if (!d[i] && s[i])
> -					return -ENOMEM;
> -			}
> +			error = property_copy_string_array(dst, src);
> +			if (error)
> +				goto out_free_name;
> Â		} else {
> Â			dst->pointer.raw_data = kmemdup(src-
> >pointer.raw_data,
> Â							src->length,
> GFP_KERNEL);
> -			if (!dst->pointer.raw_data)
> -				return -ENOMEM;
> +			if (!dst->pointer.raw_data) {
> +				error = -ENOMEM;
> +				goto out_free_name;
> +			}
> Â		}
> Â	} else if (src->is_string) {
> Â		dst->value.str = kstrdup(src->value.str, GFP_KERNEL);
> -		if (!dst->value.str && src->value.str)
> -			return -ENOMEM;
> +		if (!dst->value.str && src->value.str) {
> +			error = -ENOMEM;
> +			goto out_free_name;
> +		}
> Â	} else {
> Â		dst->value.raw_data = src->value.raw_data;
> Â	}
> @@ -729,6 +749,10 @@ static int property_entry_copy(struct
> property_entry *dst,
> Â	dst->is_string = src->is_string;
> Â
> Â	return 0;
> +
> +out_free_name:
> +	kfree(dst->name);
> +	return error;
> Â}
> Â
> Â/**
> diff --git a/include/linux/property.h b/include/linux/property.h
> index 5746e9927016..64e3a9c6d95f 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -160,12 +160,12 @@ struct property_entry {
> Â	bool is_string;
> Â	union {
> Â		union {
> -			void *raw_data;
> -			u8 *u8_data;
> -			u16 *u16_data;
> -			u32 *u32_data;
> -			u64 *u64_data;
> -			const char **str;
> +			const void *raw_data;
> +			const u8 *u8_data;
> +			const u16 *u16_data;
> +			const u32 *u32_data;
> +			const u64 *u64_data;
> +			const char * const *str;
> Â		} pointer;
> Â		union {
> Â			unsigned long long raw_data;

-- 
Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
Intel Finland Oy