[PATCH] android/client: Fix memory leak while using realloc()

[PATCH] android/client: Fix memory leak while using realloc()

[Atul Rai]

While reallocating space to store additional "remote device set" using
realloc, if realloc() fails, the original block is left untouched but
reference to that block is lost as NULL is assigned to remote_devices.
The original block needs to be freed before return.
---
 android/client/if-bt.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/android/client/if-bt.c b/android/client/if-bt.c
index 4723024..4249f78 100644
--- a/android/client/if-bt.c
+++ b/android/client/if-bt.c
@@ -94,6 +94,7 @@ static int remote_devices_capacity = 0;
 void add_remote_device(const bt_bdaddr_t *addr)
 {
 	int i;
+	bt_bdaddr_t *tmp;
 
 	if (remote_devices == NULL) {
 		remote_devices = malloc(4 * sizeof(bt_bdaddr_t));
@@ -119,9 +120,16 @@ void add_remote_device(const bt_bdaddr_t *addr)
 	/* Realloc space if needed */
 	if (remote_devices_cnt >= remote_devices_capacity) {
 		remote_devices_capacity *= 2;
+		/*
+		 * Save reference to previously allocated memory block so that
+		 * it can be freed in case realloc fails.
+		 */
+		tmp = remote_devices;
+
 		remote_devices = realloc(remote_devices, sizeof(bt_bdaddr_t) *
 						remote_devices_capacity);
 		if (remote_devices == NULL) {
+			free(tmp);
 			remote_devices_capacity = 0;
 			remote_devices_cnt = 0;
 			return;
-- 
2.1.4

Re: [PATCH] android/client: Fix memory leak while using realloc()

[Vinicius Costa Gomes]

Hi,

Atul Rai <a.rai@samsung.com> writes:

> While reallocating space to store additional "remote device set" using
> realloc, if realloc() fails, the original block is left untouched but
> reference to that block is lost as NULL is assigned to remote_devices.
> The original block needs to be freed before return.
> ---
>  android/client/if-bt.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/android/client/if-bt.c b/android/client/if-bt.c
> index 4723024..4249f78 100644
> --- a/android/client/if-bt.c
> +++ b/android/client/if-bt.c
> @@ -94,6 +94,7 @@ static int remote_devices_capacity = 0;
>  void add_remote_device(const bt_bdaddr_t *addr)
>  {
>  	int i;
> +	bt_bdaddr_t *tmp;
>
>  	if (remote_devices == NULL) {
>  		remote_devices = malloc(4 * sizeof(bt_bdaddr_t));
> @@ -119,9 +120,16 @@ void add_remote_device(const bt_bdaddr_t *addr)
>  	/* Realloc space if needed */
>  	if (remote_devices_cnt >= remote_devices_capacity) {

I would move 'tmp' declaration here. Makes it clear that it is only used
in this context.

Apart from that, looks OK.

>  		remote_devices_capacity *= 2;
> +		/*
> +		 * Save reference to previously allocated memory block so that
> +		 * it can be freed in case realloc fails.
> +		 */
> +		tmp = remote_devices;
> +
>  		remote_devices = realloc(remote_devices, sizeof(bt_bdaddr_t) *
>  						remote_devices_capacity);
>  		if (remote_devices == NULL) {
> +			free(tmp);
>  			remote_devices_capacity = 0;
>  			remote_devices_cnt = 0;
>  			return;
> --
> 2.1.4


Cheers,
--
Vinicius

Re: [PATCH] android/client: Fix memory leak while using realloc()

[Atul Rai]

Hi Vinicius,

Thanks for the review.
I will upload patch version 2 after incorporating suggested changes.

> Hi,
> 
> Atul Rai <a.rai@samsung.com> writes:
> 
>> While reallocating space to store additional "remote device set" using
>> realloc, if realloc() fails, the original block is left untouched but
>> reference to that block is lost as NULL is assigned to remote_devices.
>> The original block needs to be freed before return.
>> ---
>>  android/client/if-bt.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/android/client/if-bt.c b/android/client/if-bt.c
>> index 4723024..4249f78 100644
>> --- a/android/client/if-bt.c
>> +++ b/android/client/if-bt.c
>> @@ -94,6 +94,7 @@ static int remote_devices_capacity = 0;
>>  void add_remote_device(const bt_bdaddr_t *addr)
>>  {
>>  	int i;
>> +	bt_bdaddr_t *tmp;
>>
>>  	if (remote_devices == NULL) {
>>  		remote_devices = malloc(4 * sizeof(bt_bdaddr_t));
>> @@ -119,9 +120,16 @@ void add_remote_device(const bt_bdaddr_t *addr)
>>  	/* Realloc space if needed */
>>  	if (remote_devices_cnt >= remote_devices_capacity) {
> 
> I would move 'tmp' declaration here. Makes it clear that it is only used
> in this context.
> 
> Apart from that, looks OK.
> 
>>  		remote_devices_capacity *= 2;
>> +		/*
>> +		 * Save reference to previously allocated memory block so that
>> +		 * it can be freed in case realloc fails.
>> +		 */
>> +		tmp = remote_devices;
>> +
>>  		remote_devices = realloc(remote_devices, sizeof(bt_bdaddr_t) *
>>  						remote_devices_capacity);
>>  		if (remote_devices == NULL) {
>> +			free(tmp);
>>  			remote_devices_capacity = 0;
>>  			remote_devices_cnt = 0;
>>  			return;
>> --
>> 2.1.4
> 
> 
> Cheers,
> --
> Vinicius
>

 Regards,
 -Atul Rai