[PATCH] acpica: fwts_acpica.c: Override ACPICA Semaphores to fix memory leak bug

[Colin Ian King]

On 17/05/12 07:43, Keng-Yu Lin wrote:
> On Wed, May 16, 2012 at 5:03 PM, Colin King <colin.king at canonical.com> wrote:
>> From: Colin Ian King <colin.king at 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 at 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
>