[PATCH 0/2] tools: iio: clean up ci_array pointer

[PATCH 0/2] tools: iio: clean up ci_array pointer

[Joo Aun Saw]

This patchset cleans up ci_array pointer handling.

[PATCH 1/2] tools: iio: Set caller's ci_array pointer to NULL after free

[Joo Aun Saw]

From: Joo Aun Saw <jasaw@dius.com.au>

On error, caller's ci_array is freed and set to NULL to avoid
potential double free. Counter is reset to zero for consistency.

Signed-off-by: Joo Aun Saw <jasaw@dius.com.au>
---
 tools/iio/iio_utils.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/iio/iio_utils.c b/tools/iio/iio_utils.c
index 1dcdf03..8731905 100644
--- a/tools/iio/iio_utils.c
+++ b/tools/iio/iio_utils.c
@@ -529,6 +529,8 @@ error_cleanup_array:
 		free((*ci_array)[i].generic_name);
 	}
 	free(*ci_array);
+	*ci_array = NULL;
+	*counter = 0;
 error_close_dir:
 	if (dp)
 		if (closedir(dp) == -1)
-- 
1.9.1

Re: [PATCH 1/2] tools: iio: Set caller's ci_array pointer to NULL after free

[Hartmut Knaack]

Joo Aun Saw schrieb am 24.07.2015 um 17:23:
> From: Joo Aun Saw <jasaw@dius.com.au>
> 
> On error, caller's ci_array is freed and set to NULL to avoid
> potential double free. Counter is reset to zero for consistency.
> 

Hi,
I don't see a second attempt to free the array. In here, just dp gets
closed and the scan_el_dir string freed.
And in generic_buffer.c where it is accessed in the main function, there
is a jump into the error handler in case of an error, which just frees a
few strings (datardytrigger, trigger_name and dev_dir_name).
Any opinion welcome.
Thanks,

Hartmut

> Signed-off-by: Joo Aun Saw <jasaw@dius.com.au>
> ---
>  tools/iio/iio_utils.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/tools/iio/iio_utils.c b/tools/iio/iio_utils.c
> index 1dcdf03..8731905 100644
> --- a/tools/iio/iio_utils.c
> +++ b/tools/iio/iio_utils.c
> @@ -529,6 +529,8 @@ error_cleanup_array:
>  		free((*ci_array)[i].generic_name);
>  	}
>  	free(*ci_array);
> +	*ci_array = NULL;
> +	*counter = 0;
>  error_close_dir:
>  	if (dp)
>  		if (closedir(dp) == -1)
> 

Re: [PATCH 1/2] tools: iio: Set caller's ci_array pointer to NULL after free

[Joo Aun Saw]

[-- Attachment #1: Type: text/plain, Size: 2085 bytes --]

On Sat, Jul 25, 2015 at 5:10 AM, Hartmut Knaack <knaack.h@gmx.de> wrote:

> Joo Aun Saw schrieb am 24.07.2015 um 17:23:
> > From: Joo Aun Saw <jasaw@dius.com.au>
> >
> > On error, caller's ci_array is freed and set to NULL to avoid
> > potential double free. Counter is reset to zero for consistency.
> >
>
> Hi,
> I don't see a second attempt to free the array. In here, just dp gets
> closed and the scan_el_dir string freed.
> And in generic_buffer.c where it is accessed in the main function, there
> is a jump into the error handler in case of an error, which just frees a
> few strings (datardytrigger, trigger_name and dev_dir_name).
> Any opinion welcome.
> Thanks,
>
> Hartmut
>
> Currently, there is no second free attempt in those examples, but
iio_utils is meant to be used as a library in the future, and therefore
should not leave a dangling pointer (the caller owns the pointer in this
case). There is no guarantee what other users of iio_utils might do. For
example, a caller may have a single clean up routine that frees ALL
pointers upon failure (to simplify error handling), and will encounter
double free problem if this is not fixed. Additionally, it's good software
engineering practice to set pointers to NULL after free because the caller
may still reference it in the future.

Joo.



> > Signed-off-by: Joo Aun Saw <jasaw@dius.com.au>
> > ---
> >  tools/iio/iio_utils.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/tools/iio/iio_utils.c b/tools/iio/iio_utils.c
> > index 1dcdf03..8731905 100644
> > --- a/tools/iio/iio_utils.c
> > +++ b/tools/iio/iio_utils.c
> > @@ -529,6 +529,8 @@ error_cleanup_array:
> >               free((*ci_array)[i].generic_name);
> >       }
> >       free(*ci_array);
> > +     *ci_array = NULL;
> > +     *counter = 0;
> >  error_close_dir:
> >       if (dp)
> >               if (closedir(dp) == -1)
> >
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

[-- Attachment #2: Type: text/html, Size: 3088 bytes --]

Re: [PATCH 1/2] tools: iio: Set caller's ci_array pointer to NULL after free

[Joo Aun Saw]

On Sat, Jul 25, 2015 at 5:10 AM, Hartmut Knaack <knaack.h@gmx.de> wrote:
>
> Joo Aun Saw schrieb am 24.07.2015 um 17:23:
> > From: Joo Aun Saw <jasaw@dius.com.au>
> >
> > On error, caller's ci_array is freed and set to NULL to avoid
> > potential double free. Counter is reset to zero for consistency.
> >
>
> Hi,
> I don't see a second attempt to free the array. In here, just dp gets
> closed and the scan_el_dir string freed.
> And in generic_buffer.c where it is accessed in the main function, there
> is a jump into the error handler in case of an error, which just frees a
> few strings (datardytrigger, trigger_name and dev_dir_name).
> Any opinion welcome.
> Thanks,
>
> Hartmut
>
Currently, there is no second free attempt in those examples, but
iio_utils is meant to be used as a library in the future, and
therefore should not leave a dangling pointer (the caller owns the
pointer in this case). There is no guarantee what other users of
iio_utils might do. For example, a caller may have a single clean up
routine that frees ALL pointers upon failure (to simplify error
handling), and will encounter double free problem if this is not
fixed. Additionally, it's good software engineering practice to set
pointers to NULL after free because the caller may still reference it
in the future.

Joo.

> > Signed-off-by: Joo Aun Saw <jasaw@dius.com.au>
> > ---
> >  tools/iio/iio_utils.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/tools/iio/iio_utils.c b/tools/iio/iio_utils.c
> > index 1dcdf03..8731905 100644
> > --- a/tools/iio/iio_utils.c
> > +++ b/tools/iio/iio_utils.c
> > @@ -529,6 +529,8 @@ error_cleanup_array:
> >               free((*ci_array)[i].generic_name);
> >       }
> >       free(*ci_array);
> > +     *ci_array = NULL;
> > +     *counter = 0;
> >  error_close_dir:
> >       if (dp)
> >               if (closedir(dp) == -1)
> >
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] tools: iio: Set caller's ci_array pointer to NULL after free

[Jonathan Cameron]

On 25/07/15 06:12, Joo Aun Saw wrote:
> On Sat, Jul 25, 2015 at 5:10 AM, Hartmut Knaack <knaack.h@gmx.de> wrote:
>>
>> Joo Aun Saw schrieb am 24.07.2015 um 17:23:
>>> From: Joo Aun Saw <jasaw@dius.com.au>
>>>
>>> On error, caller's ci_array is freed and set to NULL to avoid
>>> potential double free. Counter is reset to zero for consistency.
>>>
>>
>> Hi,
>> I don't see a second attempt to free the array. In here, just dp gets
>> closed and the scan_el_dir string freed.
>> And in generic_buffer.c where it is accessed in the main function, there
>> is a jump into the error handler in case of an error, which just frees a
>> few strings (datardytrigger, trigger_name and dev_dir_name).
>> Any opinion welcome.
>> Thanks,
>>
>> Hartmut
>>
> Currently, there is no second free attempt in those examples, but
> iio_utils is meant to be used as a library in the future, and
> therefore should not leave a dangling pointer (the caller owns the
> pointer in this case). There is no guarantee what other users of
> iio_utils might do. For example, a caller may have a single clean up
> routine that frees ALL pointers upon failure (to simplify error
> handling), and will encounter double free problem if this is not
> fixed. Additionally, it's good software engineering practice to set
> pointers to NULL after free because the caller may still reference it
> in the future.
Applied with a slight change to the description to say that this was
basically for convenience if some other user was being stupid (but
politer).  Setting pointers after free to null is part of 'some'
best practice.  Assuming that pointers can be anything
after a free is part of others (and by far the safer option).
Still no harm done and it's hardly a fast path.

Jonathan
> 
> Joo.
> 
>>> Signed-off-by: Joo Aun Saw <jasaw@dius.com.au>
>>> ---
>>>  tools/iio/iio_utils.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/tools/iio/iio_utils.c b/tools/iio/iio_utils.c
>>> index 1dcdf03..8731905 100644
>>> --- a/tools/iio/iio_utils.c
>>> +++ b/tools/iio/iio_utils.c
>>> @@ -529,6 +529,8 @@ error_cleanup_array:
>>>               free((*ci_array)[i].generic_name);
>>>       }
>>>       free(*ci_array);
>>> +     *ci_array = NULL;
>>> +     *counter = 0;
>>>  error_close_dir:
>>>       if (dp)
>>>               if (closedir(dp) == -1)
>>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH 2/2] tools: iio: remove unnecessary double pointer

[Joo Aun Saw]

From: Joo Aun Saw <jasaw@dius.com.au>

Remove unnecessary double pointer from channel sorting function.

Signed-off-by: Joo Aun Saw <jasaw@dius.com.au>
---
 tools/iio/iio_utils.c | 12 ++++++------
 tools/iio/iio_utils.h |  2 +-
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/tools/iio/iio_utils.c b/tools/iio/iio_utils.c
index 8731905..5b0beea 100644
--- a/tools/iio/iio_utils.c
+++ b/tools/iio/iio_utils.c
@@ -289,17 +289,17 @@ error_free_builtname:
  * @cnt: the amount of array elements
  **/
 
-void bsort_channel_array_by_index(struct iio_channel_info **ci_array, int cnt)
+void bsort_channel_array_by_index(struct iio_channel_info *ci_array, int cnt)
 {
 	struct iio_channel_info temp;
 	int x, y;
 
 	for (x = 0; x < cnt; x++)
 		for (y = 0; y < (cnt - 1); y++)
-			if ((*ci_array)[y].index > (*ci_array)[y + 1].index) {
-				temp = (*ci_array)[y + 1];
-				(*ci_array)[y + 1] = (*ci_array)[y];
-				(*ci_array)[y] = temp;
+			if (ci_array[y].index > ci_array[y + 1].index) {
+				temp = ci_array[y + 1];
+				ci_array[y + 1] = ci_array[y];
+				ci_array[y] = temp;
 			}
 }
 
@@ -519,7 +519,7 @@ int build_channel_array(const char *device_dir,
 
 	free(scan_el_dir);
 	/* reorder so that the array is in index order */
-	bsort_channel_array_by_index(ci_array, *counter);
+	bsort_channel_array_by_index(*ci_array, *counter);
 
 	return 0;
 
diff --git a/tools/iio/iio_utils.h b/tools/iio/iio_utils.h
index f30a109..e3503bf 100644
--- a/tools/iio/iio_utils.h
+++ b/tools/iio/iio_utils.h
@@ -60,7 +60,7 @@ int iioutils_get_type(unsigned *is_signed, unsigned *bytes, unsigned *bits_used,
 int iioutils_get_param_float(float *output, const char *param_name,
 			     const char *device_dir, const char *name,
 			     const char *generic_name);
-void bsort_channel_array_by_index(struct iio_channel_info **ci_array, int cnt);
+void bsort_channel_array_by_index(struct iio_channel_info *ci_array, int cnt);
 int build_channel_array(const char *device_dir,
 			struct iio_channel_info **ci_array, int *counter);
 int find_type_by_name(const char *name, const char *type);
-- 
1.9.1

Re: [PATCH 2/2] tools: iio: remove unnecessary double pointer

[Hartmut Knaack]

Joo Aun Saw schrieb am 24.07.2015 um 17:23:
> From: Joo Aun Saw <jasaw@dius.com.au>
> 
> Remove unnecessary double pointer from channel sorting function.

That is not really unnecessary. The way you propose expresses that
ci_array would be a pointer to an element of type iio_channel_info. But
what we want ci_array to be is a pointer to an array of elements
of type iio_channel_info.
Not sure if this is a good explanation.

> 
> Signed-off-by: Joo Aun Saw <jasaw@dius.com.au>
> ---
>  tools/iio/iio_utils.c | 12 ++++++------
>  tools/iio/iio_utils.h |  2 +-
>  2 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/iio/iio_utils.c b/tools/iio/iio_utils.c
> index 8731905..5b0beea 100644
> --- a/tools/iio/iio_utils.c
> +++ b/tools/iio/iio_utils.c
> @@ -289,17 +289,17 @@ error_free_builtname:
>   * @cnt: the amount of array elements
>   **/
>  
> -void bsort_channel_array_by_index(struct iio_channel_info **ci_array, int cnt)
> +void bsort_channel_array_by_index(struct iio_channel_info *ci_array, int cnt)
>  {
>  	struct iio_channel_info temp;
>  	int x, y;
>  
>  	for (x = 0; x < cnt; x++)
>  		for (y = 0; y < (cnt - 1); y++)
> -			if ((*ci_array)[y].index > (*ci_array)[y + 1].index) {
> -				temp = (*ci_array)[y + 1];
> -				(*ci_array)[y + 1] = (*ci_array)[y];
> -				(*ci_array)[y] = temp;
> +			if (ci_array[y].index > ci_array[y + 1].index) {
> +				temp = ci_array[y + 1];
> +				ci_array[y + 1] = ci_array[y];
> +				ci_array[y] = temp;
>  			}
>  }
>  
> @@ -519,7 +519,7 @@ int build_channel_array(const char *device_dir,
>  
>  	free(scan_el_dir);
>  	/* reorder so that the array is in index order */
> -	bsort_channel_array_by_index(ci_array, *counter);
> +	bsort_channel_array_by_index(*ci_array, *counter);
>  
>  	return 0;
>  
> diff --git a/tools/iio/iio_utils.h b/tools/iio/iio_utils.h
> index f30a109..e3503bf 100644
> --- a/tools/iio/iio_utils.h
> +++ b/tools/iio/iio_utils.h
> @@ -60,7 +60,7 @@ int iioutils_get_type(unsigned *is_signed, unsigned *bytes, unsigned *bits_used,
>  int iioutils_get_param_float(float *output, const char *param_name,
>  			     const char *device_dir, const char *name,
>  			     const char *generic_name);
> -void bsort_channel_array_by_index(struct iio_channel_info **ci_array, int cnt);
> +void bsort_channel_array_by_index(struct iio_channel_info *ci_array, int cnt);
>  int build_channel_array(const char *device_dir,
>  			struct iio_channel_info **ci_array, int *counter);
>  int find_type_by_name(const char *name, const char *type);
> 

Re: [PATCH 2/2] tools: iio: remove unnecessary double pointer

[Joo Aun Saw]

On Sat, Jul 25, 2015 at 5:19 AM, Hartmut Knaack <knaack.h@gmx.de> wrote:
>
> Joo Aun Saw schrieb am 24.07.2015 um 17:23:
> > From: Joo Aun Saw <jasaw@dius.com.au>
> >
> > Remove unnecessary double pointer from channel sorting function.
>
> That is not really unnecessary. The way you propose expresses that
> ci_array would be a pointer to an element of type iio_channel_info. But
> what we want ci_array to be is a pointer to an array of elements
> of type iio_channel_info.
> Not sure if this is a good explanation.
>
1.) When passing an array to a function, the C convention is to pass a
pointer to the start of the array and the length of the array. It
would be best to stick to the convention to minimize error on the
caller part. If the parameters need to be clarified, I propose
renaming the ci_array and cnt to something more meaningful.
2.) It is good software engineering practice to minimize the
information available to a function. If a function does not need
access to a double pointer to perform its job, then we should not give
it a double pointer.

Joo.

>
> >
> > Signed-off-by: Joo Aun Saw <jasaw@dius.com.au>
> > ---
> >  tools/iio/iio_utils.c | 12 ++++++------
> >  tools/iio/iio_utils.h |  2 +-
> >  2 files changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/tools/iio/iio_utils.c b/tools/iio/iio_utils.c
> > index 8731905..5b0beea 100644
> > --- a/tools/iio/iio_utils.c
> > +++ b/tools/iio/iio_utils.c
> > @@ -289,17 +289,17 @@ error_free_builtname:
> >   * @cnt: the amount of array elements
> >   **/
> >
> > -void bsort_channel_array_by_index(struct iio_channel_info **ci_array, int cnt)
> > +void bsort_channel_array_by_index(struct iio_channel_info *ci_array, int cnt)
> >  {
> >       struct iio_channel_info temp;
> >       int x, y;
> >
> >       for (x = 0; x < cnt; x++)
> >               for (y = 0; y < (cnt - 1); y++)
> > -                     if ((*ci_array)[y].index > (*ci_array)[y + 1].index) {
> > -                             temp = (*ci_array)[y + 1];
> > -                             (*ci_array)[y + 1] = (*ci_array)[y];
> > -                             (*ci_array)[y] = temp;
> > +                     if (ci_array[y].index > ci_array[y + 1].index) {
> > +                             temp = ci_array[y + 1];
> > +                             ci_array[y + 1] = ci_array[y];
> > +                             ci_array[y] = temp;
> >                       }
> >  }
> >
> > @@ -519,7 +519,7 @@ int build_channel_array(const char *device_dir,
> >
> >       free(scan_el_dir);
> >       /* reorder so that the array is in index order */
> > -     bsort_channel_array_by_index(ci_array, *counter);
> > +     bsort_channel_array_by_index(*ci_array, *counter);
> >
> >       return 0;
> >
> > diff --git a/tools/iio/iio_utils.h b/tools/iio/iio_utils.h
> > index f30a109..e3503bf 100644
> > --- a/tools/iio/iio_utils.h
> > +++ b/tools/iio/iio_utils.h
> > @@ -60,7 +60,7 @@ int iioutils_get_type(unsigned *is_signed, unsigned *bytes, unsigned *bits_used,
> >  int iioutils_get_param_float(float *output, const char *param_name,
> >                            const char *device_dir, const char *name,
> >                            const char *generic_name);
> > -void bsort_channel_array_by_index(struct iio_channel_info **ci_array, int cnt);
> > +void bsort_channel_array_by_index(struct iio_channel_info *ci_array, int cnt);
> >  int build_channel_array(const char *device_dir,
> >                       struct iio_channel_info **ci_array, int *counter);
> >  int find_type_by_name(const char *name, const char *type);
> >
>

Re: [PATCH 2/2] tools: iio: remove unnecessary double pointer

[Jonathan Cameron]

On 24/07/15 16:23, Joo Aun Saw wrote:
> From: Joo Aun Saw <jasaw@dius.com.au>
> 
> Remove unnecessary double pointer from channel sorting function.
> 
> Signed-off-by: Joo Aun Saw <jasaw@dius.com.au>
Applied to the togreg branch of iio.git

Thanks,

Jonathan
> ---
>  tools/iio/iio_utils.c | 12 ++++++------
>  tools/iio/iio_utils.h |  2 +-
>  2 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/iio/iio_utils.c b/tools/iio/iio_utils.c
> index 8731905..5b0beea 100644
> --- a/tools/iio/iio_utils.c
> +++ b/tools/iio/iio_utils.c
> @@ -289,17 +289,17 @@ error_free_builtname:
>   * @cnt: the amount of array elements
>   **/
>  
> -void bsort_channel_array_by_index(struct iio_channel_info **ci_array, int cnt)
> +void bsort_channel_array_by_index(struct iio_channel_info *ci_array, int cnt)
>  {
>  	struct iio_channel_info temp;
>  	int x, y;
>  
>  	for (x = 0; x < cnt; x++)
>  		for (y = 0; y < (cnt - 1); y++)
> -			if ((*ci_array)[y].index > (*ci_array)[y + 1].index) {
> -				temp = (*ci_array)[y + 1];
> -				(*ci_array)[y + 1] = (*ci_array)[y];
> -				(*ci_array)[y] = temp;
> +			if (ci_array[y].index > ci_array[y + 1].index) {
> +				temp = ci_array[y + 1];
> +				ci_array[y + 1] = ci_array[y];
> +				ci_array[y] = temp;
>  			}
>  }
>  
> @@ -519,7 +519,7 @@ int build_channel_array(const char *device_dir,
>  
>  	free(scan_el_dir);
>  	/* reorder so that the array is in index order */
> -	bsort_channel_array_by_index(ci_array, *counter);
> +	bsort_channel_array_by_index(*ci_array, *counter);
>  
>  	return 0;
>  
> diff --git a/tools/iio/iio_utils.h b/tools/iio/iio_utils.h
> index f30a109..e3503bf 100644
> --- a/tools/iio/iio_utils.h
> +++ b/tools/iio/iio_utils.h
> @@ -60,7 +60,7 @@ int iioutils_get_type(unsigned *is_signed, unsigned *bytes, unsigned *bits_used,
>  int iioutils_get_param_float(float *output, const char *param_name,
>  			     const char *device_dir, const char *name,
>  			     const char *generic_name);
> -void bsort_channel_array_by_index(struct iio_channel_info **ci_array, int cnt);
> +void bsort_channel_array_by_index(struct iio_channel_info *ci_array, int cnt);
>  int build_channel_array(const char *device_dir,
>  			struct iio_channel_info **ci_array, int *counter);
>  int find_type_by_name(const char *name, const char *type);
>