Patchwork acpica: fwts_acpica.c: Override ACPICA Semaphores to fix memory leak bug

login
register
mail settings
Submitter Colin King
Date May 16, 2012, 9:03 a.m.
Message ID <1337159028-20597-1-git-send-email-colin.king@canonical.com>
Download mbox | patch
Permalink /patch/159551/
State Accepted
Headers show

Comments

Colin King - May 16, 2012, 9:03 a.m.
From: Colin Ian King <colin.king@canonical.com>

The ACPICA Semaphore deletion does not free the allocated semaphore so
we re-implement the creation and deletion for fwts to fix this ACPICA core
bug.

Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 src/acpica/Makefile.am   |    2 ++
 src/acpica/fwts_acpica.c |   47 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 49 insertions(+)
Keng-Yu Lin - May 17, 2012, 6:43 a.m.
On Wed, May 16, 2012 at 5:03 PM, Colin King <colin.king@canonical.com> wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> The ACPICA Semaphore deletion does not free the allocated semaphore so
> we re-implement the creation and deletion for fwts to fix this ACPICA core
> bug.
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  src/acpica/Makefile.am   |    2 ++
>  src/acpica/fwts_acpica.c |   47 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 49 insertions(+)
>
> diff --git a/src/acpica/Makefile.am b/src/acpica/Makefile.am
> index 5a475e9..7d1249c 100644
> --- a/src/acpica/Makefile.am
> +++ b/src/acpica/Makefile.am
> @@ -20,6 +20,8 @@ osunixxf_munged.c: $(ACPICA_OSL)/osunixxf.c
>        sed 's/^AcpiOsReadPciConfiguration/__AcpiOsReadPciConfiguration/' | \
>        sed 's/^AcpiOsSignalSemaphore/__AcpiOsSignalSemaphore/' | \
>        sed 's/^AcpiOsWaitSemaphore/__AcpiOsWaitSemaphore/' | \
> +       sed 's/^AcpiOsCreateSemaphore/__AcpiOsCreateSemaphore/' | \
> +       sed 's/^AcpiOsDeleteSemaphore/__AcpiOsDeleteSemaphore/' | \
>        sed 's/^AcpiOsVprintf/__AcpiOsVprintf/' | \
>        sed 's/^AcpiOsSignal/__AcpiOsSignal/' | \
>        sed 's/^AcpiOsSleep/__AcpiOsSleep/' \
> diff --git a/src/acpica/fwts_acpica.c b/src/acpica/fwts_acpica.c
> index 3dc5ced..b8119cb 100644
> --- a/src/acpica/fwts_acpica.c
> +++ b/src/acpica/fwts_acpica.c
> @@ -485,6 +485,53 @@ void AcpiOsVprintf(const char *fmt, va_list args)
>  }
>
>  /*
> + *  AcpiOsCreateSemaphore()
> + *     Override ACPICA AcpiOsCreateSemaphore
> + */
> +ACPI_STATUS AcpiOsCreateSemaphore(UINT32 MaxUnits,
> +       UINT32 InitialUnits,
> +       ACPI_HANDLE *OutHandle)
> +{
> +       sem_t *sem;
> +
> +       if (!OutHandle)
> +               return AE_BAD_PARAMETER;
> +
> +       sem = AcpiOsAllocate(sizeof(sem_t));
> +       if (!sem)
> +               return AE_NO_MEMORY;
> +
> +       if (sem_init(sem, 0, InitialUnits) == -1) {
> +               AcpiOsFree(sem);
> +               return AE_BAD_PARAMETER;
> +       }
> +
> +       *OutHandle = (ACPI_HANDLE)sem;
> +
> +       return AE_OK;
> +}
> +
> +/*
> + *  AcpiOsDeleteSemaphore()
> + *     Override ACPICA AcpiOsDeleteSemaphore to fix semaphore memory freeing
> + */
> +ACPI_STATUS AcpiOsDeleteSemaphore(ACPI_HANDLE Handle)
> +{
> +       sem_t *sem = (sem_t *)Handle;
> +
> +       if (!sem)
> +               return AE_BAD_PARAMETER;
> +
> +       if (sem_destroy(sem) == -1)
> +               return AE_BAD_PARAMETER;
> +
> +       AcpiOsFree(sem);
> +
> +       return AE_OK;
> +}
> +
> +
> +/*
>  *  AcpiOsWaitSemaphore()
>  *     Override ACPICA AcpiOsWaitSemaphore to keep track of semaphore acquires
>  *     so that we can see if any methods are sloppy in their releases.
> --
> 1.7.10
>

Hi Colin,
  Is there a leak detected by valgrind on this?
  sem_destroy() seems to be supposed to do the house keeping.

  cheers,
-kengyu
Colin King - May 17, 2012, 8:14 a.m.
On 17/05/12 07:43, Keng-Yu Lin wrote:
> On Wed, May 16, 2012 at 5:03 PM, Colin King <colin.king@canonical.com> wrote:
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> The ACPICA Semaphore deletion does not free the allocated semaphore so
>> we re-implement the creation and deletion for fwts to fix this ACPICA core
>> bug.
>>
>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>> ---
>>   src/acpica/Makefile.am   |    2 ++
>>   src/acpica/fwts_acpica.c |   47 ++++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 49 insertions(+)
>>
>> diff --git a/src/acpica/Makefile.am b/src/acpica/Makefile.am
>> index 5a475e9..7d1249c 100644
>> --- a/src/acpica/Makefile.am
>> +++ b/src/acpica/Makefile.am
>> @@ -20,6 +20,8 @@ osunixxf_munged.c: $(ACPICA_OSL)/osunixxf.c
>>         sed 's/^AcpiOsReadPciConfiguration/__AcpiOsReadPciConfiguration/' | \
>>         sed 's/^AcpiOsSignalSemaphore/__AcpiOsSignalSemaphore/' | \
>>         sed 's/^AcpiOsWaitSemaphore/__AcpiOsWaitSemaphore/' | \
>> +       sed 's/^AcpiOsCreateSemaphore/__AcpiOsCreateSemaphore/' | \
>> +       sed 's/^AcpiOsDeleteSemaphore/__AcpiOsDeleteSemaphore/' | \
>>         sed 's/^AcpiOsVprintf/__AcpiOsVprintf/' | \
>>         sed 's/^AcpiOsSignal/__AcpiOsSignal/' | \
>>         sed 's/^AcpiOsSleep/__AcpiOsSleep/' \
>> diff --git a/src/acpica/fwts_acpica.c b/src/acpica/fwts_acpica.c
>> index 3dc5ced..b8119cb 100644
>> --- a/src/acpica/fwts_acpica.c
>> +++ b/src/acpica/fwts_acpica.c
>> @@ -485,6 +485,53 @@ void AcpiOsVprintf(const char *fmt, va_list args)
>>   }
>>
>>   /*
>> + *  AcpiOsCreateSemaphore()
>> + *     Override ACPICA AcpiOsCreateSemaphore
>> + */
>> +ACPI_STATUS AcpiOsCreateSemaphore(UINT32 MaxUnits,
>> +       UINT32 InitialUnits,
>> +       ACPI_HANDLE *OutHandle)
>> +{
>> +       sem_t *sem;
>> +
>> +       if (!OutHandle)
>> +               return AE_BAD_PARAMETER;
>> +
>> +       sem = AcpiOsAllocate(sizeof(sem_t));
>> +       if (!sem)
>> +               return AE_NO_MEMORY;
>> +
>> +       if (sem_init(sem, 0, InitialUnits) == -1) {
>> +               AcpiOsFree(sem);
>> +               return AE_BAD_PARAMETER;
>> +       }
>> +
>> +       *OutHandle = (ACPI_HANDLE)sem;
>> +
>> +       return AE_OK;
>> +}
>> +
>> +/*
>> + *  AcpiOsDeleteSemaphore()
>> + *     Override ACPICA AcpiOsDeleteSemaphore to fix semaphore memory freeing
>> + */
>> +ACPI_STATUS AcpiOsDeleteSemaphore(ACPI_HANDLE Handle)
>> +{
>> +       sem_t *sem = (sem_t *)Handle;
>> +
>> +       if (!sem)
>> +               return AE_BAD_PARAMETER;
>> +
>> +       if (sem_destroy(sem) == -1)
>> +               return AE_BAD_PARAMETER;
>> +
>> +       AcpiOsFree(sem);
>> +
>> +       return AE_OK;
>> +}
>> +
>> +
>> +/*
>>   *  AcpiOsWaitSemaphore()
>>   *     Override ACPICA AcpiOsWaitSemaphore to keep track of semaphore acquires
>>   *     so that we can see if any methods are sloppy in their releases.
>> --
>> 1.7.10
>>
>
> Hi Colin,
>    Is there a leak detected by valgrind on this?

Yes there is, this is how I found it.

>    sem_destroy() seems to be supposed to do the house keeping.
>

sem_destroy() destroys the semaphore but we still have the memory 
allocated for the semaphore to free afterwards, as allocated by the 
AcpiOsCreateSemaphore().   Normal use where the semaphore is a stack 
based variable is as follows:

sem_t sem;

sem_init(&sem, ...);
.. do stuff..

sem_destroy(&sem);

and for a heap bases semaphore:

sem_t *sem;

sem = malloc(sizeof(sem_t));

sem_init(sem, ...);
... do stuff ..

sem_destroy(sem);
free(sem);

Colin






>    cheers,
> -kengyu
>
Keng-Yu Lin - May 17, 2012, 8:21 a.m.
On Thu, May 17, 2012 at 4:14 PM, Colin Ian King
<colin.king@canonical.com> wrote:
> On 17/05/12 07:43, Keng-Yu Lin wrote:
>>
>> On Wed, May 16, 2012 at 5:03 PM, Colin King <colin.king@canonical.com>
>> wrote:
>>>
>>> From: Colin Ian King <colin.king@canonical.com>
>>>
>>> The ACPICA Semaphore deletion does not free the allocated semaphore so
>>> we re-implement the creation and deletion for fwts to fix this ACPICA
>>> core
>>> bug.
>>>
>>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>>> ---
>>>  src/acpica/Makefile.am   |    2 ++
>>>  src/acpica/fwts_acpica.c |   47
>>> ++++++++++++++++++++++++++++++++++++++++++++++
>>>  2 files changed, 49 insertions(+)
>>>
>>> diff --git a/src/acpica/Makefile.am b/src/acpica/Makefile.am
>>> index 5a475e9..7d1249c 100644
>>> --- a/src/acpica/Makefile.am
>>> +++ b/src/acpica/Makefile.am
>>> @@ -20,6 +20,8 @@ osunixxf_munged.c: $(ACPICA_OSL)/osunixxf.c
>>>        sed 's/^AcpiOsReadPciConfiguration/__AcpiOsReadPciConfiguration/'
>>> | \
>>>        sed 's/^AcpiOsSignalSemaphore/__AcpiOsSignalSemaphore/' | \
>>>        sed 's/^AcpiOsWaitSemaphore/__AcpiOsWaitSemaphore/' | \
>>> +       sed 's/^AcpiOsCreateSemaphore/__AcpiOsCreateSemaphore/' | \
>>> +       sed 's/^AcpiOsDeleteSemaphore/__AcpiOsDeleteSemaphore/' | \
>>>        sed 's/^AcpiOsVprintf/__AcpiOsVprintf/' | \
>>>        sed 's/^AcpiOsSignal/__AcpiOsSignal/' | \
>>>        sed 's/^AcpiOsSleep/__AcpiOsSleep/' \
>>> diff --git a/src/acpica/fwts_acpica.c b/src/acpica/fwts_acpica.c
>>> index 3dc5ced..b8119cb 100644
>>> --- a/src/acpica/fwts_acpica.c
>>> +++ b/src/acpica/fwts_acpica.c
>>> @@ -485,6 +485,53 @@ void AcpiOsVprintf(const char *fmt, va_list args)
>>>  }
>>>
>>>  /*
>>> + *  AcpiOsCreateSemaphore()
>>> + *     Override ACPICA AcpiOsCreateSemaphore
>>> + */
>>> +ACPI_STATUS AcpiOsCreateSemaphore(UINT32 MaxUnits,
>>> +       UINT32 InitialUnits,
>>> +       ACPI_HANDLE *OutHandle)
>>> +{
>>> +       sem_t *sem;
>>> +
>>> +       if (!OutHandle)
>>> +               return AE_BAD_PARAMETER;
>>> +
>>> +       sem = AcpiOsAllocate(sizeof(sem_t));
>>> +       if (!sem)
>>> +               return AE_NO_MEMORY;
>>> +
>>> +       if (sem_init(sem, 0, InitialUnits) == -1) {
>>> +               AcpiOsFree(sem);
>>> +               return AE_BAD_PARAMETER;
>>> +       }
>>> +
>>> +       *OutHandle = (ACPI_HANDLE)sem;
>>> +
>>> +       return AE_OK;
>>> +}
>>> +
>>> +/*
>>> + *  AcpiOsDeleteSemaphore()
>>> + *     Override ACPICA AcpiOsDeleteSemaphore to fix semaphore memory
>>> freeing
>>> + */
>>> +ACPI_STATUS AcpiOsDeleteSemaphore(ACPI_HANDLE Handle)
>>> +{
>>> +       sem_t *sem = (sem_t *)Handle;
>>> +
>>> +       if (!sem)
>>> +               return AE_BAD_PARAMETER;
>>> +
>>> +       if (sem_destroy(sem) == -1)
>>> +               return AE_BAD_PARAMETER;
>>> +
>>> +       AcpiOsFree(sem);
>>> +
>>> +       return AE_OK;
>>> +}
>>> +
>>> +
>>> +/*
>>>  *  AcpiOsWaitSemaphore()
>>>  *     Override ACPICA AcpiOsWaitSemaphore to keep track of semaphore
>>> acquires
>>>  *     so that we can see if any methods are sloppy in their releases.
>>> --
>>> 1.7.10
>>>
>>
>> Hi Colin,
>>   Is there a leak detected by valgrind on this?
>
>
> Yes there is, this is how I found it.
>
>
>>   sem_destroy() seems to be supposed to do the house keeping.
>>
>
> sem_destroy() destroys the semaphore but we still have the memory allocated
> for the semaphore to free afterwards, as allocated by the
> AcpiOsCreateSemaphore().   Normal use where the semaphore is a stack based
> variable is as follows:
>
> sem_t sem;
>
> sem_init(&sem, ...);
> .. do stuff..
>
> sem_destroy(&sem);
>
> and for a heap bases semaphore:
>
> sem_t *sem;
>
> sem = malloc(sizeof(sem_t));
>
> sem_init(sem, ...);
> ... do stuff ..
>
> sem_destroy(sem);
> free(sem);
>
> Colin
>

Thanks for providing this. :-)

Acked-by: Keng-Yu Lin <kengyu@canonical.com>
Ivan Hu - May 22, 2012, 3:26 a.m.
On 05/16/2012 05:03 PM, Colin King wrote:
> From: Colin Ian King<colin.king@canonical.com>
>
> The ACPICA Semaphore deletion does not free the allocated semaphore so
> we re-implement the creation and deletion for fwts to fix this ACPICA core
> bug.
>
> Signed-off-by: Colin Ian King<colin.king@canonical.com>
> ---
>   src/acpica/Makefile.am   |    2 ++
>   src/acpica/fwts_acpica.c |   47 ++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 49 insertions(+)
>
> diff --git a/src/acpica/Makefile.am b/src/acpica/Makefile.am
> index 5a475e9..7d1249c 100644
> --- a/src/acpica/Makefile.am
> +++ b/src/acpica/Makefile.am
> @@ -20,6 +20,8 @@ osunixxf_munged.c: $(ACPICA_OSL)/osunixxf.c
>   	sed 's/^AcpiOsReadPciConfiguration/__AcpiOsReadPciConfiguration/' | \
>   	sed 's/^AcpiOsSignalSemaphore/__AcpiOsSignalSemaphore/' | \
>   	sed 's/^AcpiOsWaitSemaphore/__AcpiOsWaitSemaphore/' | \
> +	sed 's/^AcpiOsCreateSemaphore/__AcpiOsCreateSemaphore/' | \
> +	sed 's/^AcpiOsDeleteSemaphore/__AcpiOsDeleteSemaphore/' | \
>   	sed 's/^AcpiOsVprintf/__AcpiOsVprintf/' | \
>   	sed 's/^AcpiOsSignal/__AcpiOsSignal/' | \
>   	sed 's/^AcpiOsSleep/__AcpiOsSleep/' \
> diff --git a/src/acpica/fwts_acpica.c b/src/acpica/fwts_acpica.c
> index 3dc5ced..b8119cb 100644
> --- a/src/acpica/fwts_acpica.c
> +++ b/src/acpica/fwts_acpica.c
> @@ -485,6 +485,53 @@ void AcpiOsVprintf(const char *fmt, va_list args)
>   }
>
>   /*
> + *  AcpiOsCreateSemaphore()
> + *	Override ACPICA AcpiOsCreateSemaphore
> + */
> +ACPI_STATUS AcpiOsCreateSemaphore(UINT32 MaxUnits,
> +	UINT32 InitialUnits,
> +	ACPI_HANDLE *OutHandle)
> +{
> +	sem_t *sem;
> +
> +	if (!OutHandle)
> +		return AE_BAD_PARAMETER;
> +
> +	sem = AcpiOsAllocate(sizeof(sem_t));
> +	if (!sem)
> +		return AE_NO_MEMORY;
> +
> +	if (sem_init(sem, 0, InitialUnits) == -1) {
> +		AcpiOsFree(sem);
> +		return AE_BAD_PARAMETER;
> +	}
> +
> +	*OutHandle = (ACPI_HANDLE)sem;
> +
> +	return AE_OK;
> +}
> +
> +/*
> + *  AcpiOsDeleteSemaphore()
> + *	Override ACPICA AcpiOsDeleteSemaphore to fix semaphore memory freeing
> + */
> +ACPI_STATUS AcpiOsDeleteSemaphore(ACPI_HANDLE Handle)
> +{
> +	sem_t *sem = (sem_t *)Handle;
> +
> +	if (!sem)
> +		return AE_BAD_PARAMETER;
> +
> +	if (sem_destroy(sem) == -1)
> +		return AE_BAD_PARAMETER;
> +
> +	AcpiOsFree(sem);
> +
> +	return AE_OK;
> +}
> +
> +
> +/*
>    *  AcpiOsWaitSemaphore()
>    *	Override ACPICA AcpiOsWaitSemaphore to keep track of semaphore acquires
>    *	so that we can see if any methods are sloppy in their releases.
Acked-by: Ivan Hu<ivan.hu@canonical.com>

Patch

diff --git a/src/acpica/Makefile.am b/src/acpica/Makefile.am
index 5a475e9..7d1249c 100644
--- a/src/acpica/Makefile.am
+++ b/src/acpica/Makefile.am
@@ -20,6 +20,8 @@  osunixxf_munged.c: $(ACPICA_OSL)/osunixxf.c
 	sed 's/^AcpiOsReadPciConfiguration/__AcpiOsReadPciConfiguration/' | \
 	sed 's/^AcpiOsSignalSemaphore/__AcpiOsSignalSemaphore/' | \
 	sed 's/^AcpiOsWaitSemaphore/__AcpiOsWaitSemaphore/' | \
+	sed 's/^AcpiOsCreateSemaphore/__AcpiOsCreateSemaphore/' | \
+	sed 's/^AcpiOsDeleteSemaphore/__AcpiOsDeleteSemaphore/' | \
 	sed 's/^AcpiOsVprintf/__AcpiOsVprintf/' | \
 	sed 's/^AcpiOsSignal/__AcpiOsSignal/' | \
 	sed 's/^AcpiOsSleep/__AcpiOsSleep/' \
diff --git a/src/acpica/fwts_acpica.c b/src/acpica/fwts_acpica.c
index 3dc5ced..b8119cb 100644
--- a/src/acpica/fwts_acpica.c
+++ b/src/acpica/fwts_acpica.c
@@ -485,6 +485,53 @@  void AcpiOsVprintf(const char *fmt, va_list args)
 }
 
 /*
+ *  AcpiOsCreateSemaphore()
+ *	Override ACPICA AcpiOsCreateSemaphore
+ */
+ACPI_STATUS AcpiOsCreateSemaphore(UINT32 MaxUnits,
+	UINT32 InitialUnits,
+	ACPI_HANDLE *OutHandle)
+{
+	sem_t *sem;
+
+	if (!OutHandle)
+		return AE_BAD_PARAMETER;
+
+	sem = AcpiOsAllocate(sizeof(sem_t));
+	if (!sem)
+		return AE_NO_MEMORY;
+
+	if (sem_init(sem, 0, InitialUnits) == -1) {
+		AcpiOsFree(sem);
+		return AE_BAD_PARAMETER;
+	}
+
+	*OutHandle = (ACPI_HANDLE)sem;
+
+	return AE_OK;
+}
+
+/*
+ *  AcpiOsDeleteSemaphore()
+ *	Override ACPICA AcpiOsDeleteSemaphore to fix semaphore memory freeing
+ */
+ACPI_STATUS AcpiOsDeleteSemaphore(ACPI_HANDLE Handle)
+{
+	sem_t *sem = (sem_t *)Handle;
+
+	if (!sem)
+		return AE_BAD_PARAMETER;
+
+	if (sem_destroy(sem) == -1)
+		return AE_BAD_PARAMETER;
+
+	AcpiOsFree(sem);
+
+	return AE_OK;
+}
+
+
+/*
  *  AcpiOsWaitSemaphore()
  *	Override ACPICA AcpiOsWaitSemaphore to keep track of semaphore acquires
  *	so that we can see if any methods are sloppy in their releases.