Re: [Qemu-devel] [PATCH v2 01/47] acpi: introduce AML composer aml_append()

[Michael S. Tsirkin]

On Thu, Jan 22, 2015 at 02:49:45PM +0000, Igor Mammedov wrote:
> Adds for dynamic AML creation, which will be used
> for piecing ASL/AML primitives together and hiding
> from user/caller details about how nested context
> should be closed/packed leaving less space for
> mistakes and necessity to know how AML should be
> encoded, allowing user to concentrate on ASL
> representation instead.
> 
> For example it will allow to create AML like this:
> 
> AcpiAml scope = acpi_scope("PCI0")
> AcpiAml dev = acpi_device("PM")
>     aml_append(&dev, acpi_name_decl("_ADR", acpi_int(addr)))
> aml_append(&scope, dev);
> 
> Signed-off-by: Igor Mammedov <address@hidden>
> ---
>  hw/acpi/acpi-build-utils.c         | 39 
> ++++++++++++++++++++++++++++++++++++++
>  include/hw/acpi/acpi-build-utils.h | 16 ++++++++++++++++
>  2 files changed, 55 insertions(+)
> 
> diff --git a/hw/acpi/acpi-build-utils.c b/hw/acpi/acpi-build-utils.c
> index 602e68c..547ecaa 100644
> --- a/hw/acpi/acpi-build-utils.c
> +++ b/hw/acpi/acpi-build-utils.c
> @@ -267,3 +267,42 @@ void build_append_int(GArray *table, uint32_t value)
>          build_append_value(table, value, 4);
>      }
>  }
> +
> +static void build_prepend_int(GArray *array, uint32_t value)
> +{
> +    GArray *data = build_alloc_array();
> +
> +    build_append_int(data, value);
> +    g_array_prepend_vals(array, data->data, data->len);
> +    build_free_array(data);
> +}
> +
> +void aml_append(AcpiAml *parent_ctx, AcpiAml child)
> +{
> +    switch (child.block_flags) {
> +    case EXT_PACKAGE:
> +        build_extop_package(child.buf, child.op);
> +        break;
> +
> +    case PACKAGE:
> +        build_package(child.buf, child.op);
> +        break;
> +
> +    case RES_TEMPLATE:
> +        build_append_byte(child.buf, 0x79); /* EndTag */
> +        /*
> +         * checksum operations is treated as succeeded if checksum
> +         * field is zero. [ACPI Spec 5.0, 6.4.2.9 End Tag]
> +         */
> +        build_append_byte(child.buf, 0);
> +        /* fall through, to pack resources in buffer */
> +    case BUFFER:
> +        build_prepend_int(child.buf, child.buf->len);
> +        build_package(child.buf, child.op);
> +        break;
> +    default:
> +        break;
> +    }
> +    build_append_array(parent_ctx->buf, child.buf);
> +    build_free_array(child.buf);

So looking at this, the API is a bit tricky to use
to avoid use after free.

For example:
        aml_append(&a, b);
        aml_append(&a, b);
this is use after free.

This is C, memory management should not be automatic.
So just move free out of there, get child by const pointer.
Same for alloc: split out allocation and init.
You can still return pointer back to caller,
this will allow chaining just like you do here.

We'll get:

        AcpiAml dev = aml_alloc();

        aml_append(&scope, acpi_device(&dev));

        aml_free(&dev);


which is more verbose, but makes it clear
there's no use after free, additionally,
compiler checks that you don't modify child
where not necessary.


> +}
> diff --git a/include/hw/acpi/acpi-build-utils.h 
> b/include/hw/acpi/acpi-build-utils.h
> index 199f003..64e7ec3 100644
> --- a/include/hw/acpi/acpi-build-utils.h
> +++ b/include/hw/acpi/acpi-build-utils.h
> @@ -5,6 +5,22 @@
>  #include <glib.h>
>  #include "qemu/compiler.h"
>  
> +typedef enum {
> +    NON_BLOCK,
> +    PACKAGE,
> +    EXT_PACKAGE,
> +    BUFFER,
> +    RES_TEMPLATE,
> +} AcpiBlockFlags;
> +
> +typedef struct AcpiAml {
> +    GArray *buf;
> +    uint8_t op;
> +    AcpiBlockFlags block_flags;
> +} AcpiAml;
> +
> +void aml_append(AcpiAml *parent_ctx, AcpiAml child);
> +
>  GArray *build_alloc_array(void);
>  void build_free_array(GArray *array);
>  void build_prepend_byte(GArray *array, uint8_t val);
> -- 
> 1.8.3.1