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

[Andrew Jones]

On Mon, Jan 26, 2015 at 04:09:20PM +0100, Igor Mammedov wrote:
> On Mon, 26 Jan 2015 10:57:21 +0100
> Igor Mammedov <address@hidden> wrote:
> 
> > On Sat, 24 Jan 2015 18:33:50 +0200
> > "Michael S. Tsirkin" <address@hidden> wrote:
> > 
> > > On Fri, Jan 23, 2015 at 06:56:20PM +0100, Igor Mammedov wrote:
> > > > On Fri, 23 Jan 2015 15:55:11 +0200
> > > > "Michael S. Tsirkin" <address@hidden> wrote:
> > > > 
> [...]
> > > > I refuse to give up on cleaner and simpler API yet :)
> > > > 
> > > > > 
> > > > > 
> > > > > Your patches are almost there, they are pretty clean, the only issue I
> > > > > think is this passing of AcpiAml by value, sometimes freeing buffer in
> > > > > the process, sometimes not.
> > > > Currently buffer is allocated by API and is always freed whenever
> > > > it's passed to another API function.
> > > > That's why it makes user not to care about memory mgmt.
> > > > 
> > > > The only limitation of it is if you store AcpiAml return value into some
> > > > variable you are responsible to use it only once for passing to another 
> > > > API
> > > > function. Reusing this variable's value (pass it to API function second 
> > > > time)
> > > > would cause cause use-after-free and freeing-freed bugs.
> > > > Like this:
> > > > AcpiAml table = acpi_definition_block("SSDT",...);
> > > > AcpiAml scope = acpi_scope("PCI0");
> > > > aml_append(&table, scope); // <- here scope becomes invalid
> > > > // a bug
> > > > aml_append(&table, scope); // use-after-free + freeing-freed bugs
> > > > 
> > > > There are several approaches to look for resolving above issues:
> > > > 1. Adopt and use memory mgmt model used by GTK+
> > > >    in nutshell: 
> > > > http://www.cs.hunter.cuny.edu/~sweiss/course_materials/csci493.70/lecture_notes/GTK_memory_mngmt.pdf
> > > >    In particular adopt behavior of GInitiallyUnowned usage model
> > > > 
> > > >    that will allow to keep convenient chained call style and if 
> > > > necessary
> > > >    reuse objects returned by API by explicitly referencing/dereferencing
> > > >    them if needed.
> > > 
> > > Hmm, it's still easy to misuse. I think I prefer option 2 below.
> > That's basically what we have/use in QOM with object_new(FOO) + 
> > object_unref()
> > I have no idea why we invented our own Object infrastructure
> > when we could just use GObject one from already used glib.
> > 
> > > 
> > > > 2. It's possible to drop freeing inside API completely and
> > > >    record(store in list) every new object inside a table context.
> > > >    When table is constructed, list of created objects could be
> > > >    safely freed.
> > > >    With that it would be safe to reuse every AcpiAml object
> > > >    and avoid free-after-use issues with limitation that created
> > > >    AcpiAml objects shouldn't be used after table was closed.
> > > >    It should cover all practical use of API, i.e. no cross
> > > >    table AcpiAml objects.
> > > 
> > > So each aml_alloc function gets pointer to this list,
> > > and adds the new element there.
> > > Eventually we do free_all to free all elements,
> > > so there isn't even an aml_free to mis-use.
> > I'm thinking a little bit different about implementation though.
> > I still don't like the use of explicit alloc/free being called
> > by API user since it doesn't allow chained API calls and
> > I think it's unnecessary complication see below why.
> > 
> > Here is what's true about current API and a I'd like to with it:
> > 
> >   1. Every API call (except aml_append) makes aml_alloc(), it's just
> >      like a wrapper about object_new(FOO). (current + new impl.)
> > 
> >   2 Every API call that takes AML type as input argument
> >   2.1 consumes (frees) it (current impl.)
> >       (it's easy to fix use after free concern too,
> >        just pass AML by pointer and zero-out memory before it's freed
> >        and assert whenever one of input arguments is not correct,
> >        i.e. it was reused second time)
> >       There is no need for following steps after this one.
> >   2.2 takes ownership of GInitiallyUnowned and adds it to its list
> >       of its children.
> >   3. Free children when AML object is destroyed (i.e. ref count zero)
> >      That way when toplevel table object (definition block in 42/47)
> >      is added to ACPI blob we can unref it, which will cause
> >      its whole children tree freed, except for AML objects where
> >      API user explicitly took extra reference (i.e. wanted them
> >      to reuse in another table)
> > 
> > I'd prefer:
> >  *  2.1 way to address your current concern of use-after-free
> >     as the most simplest one (no reuse is possible however)
> > or
> >  * follow already used by QEMU QOM/GObject pattern of
> >    implicit alloc/free
> > 
> > since they allow to construct AML in a more simple/manageable way i.e.
> >  
> >   aml_append(method,
> >       aml_store(aml_string("foo"), aml_local(0)))
> >   );
> > 
> > v.s. explicit headache of alloc/free, which doesn't fix
> >      use-after-free anyway and just adds more boiler plate
> >      plus makes code har to read read
> > 
> >   str = aml_alloc();
> >   aml_string(str, "foo");
> >   loc0 = aml_alloc();
> >   aml_local(loc0, 0);
> >   store = aml_alloc();
> >   aml_store(store, str, loc0);
> >   aml_append(method, store);
> >   aml_free(store);
> >   aml_free(loc0);
> >   aml_free(str);
> 
> Here is a compromise what I and Michael came to on a phone call:
> 
> Externally API usage would look like:
> 
> AmlAllocList *p = some_list_alloc();
> 
> Aml *ssdt = aml_def_block(p, "SSDT", ...);
> Aml *dev = aml_device(p, "PCI0");
> aml_append(dev,
>     aml_name_def(p, "_STA", aml_int(p, 0xF /* present */))
> );
> aml_append(ssdt, dev);
> 
> aml_append(acpi_tables_blob, ssdt);
> 
> free_aml_alloc_list(p);
> 
> 
> Each of aml_foo() will take other Aml arguments by pointer.
> Also every aml_foo(), except of aml_append() will allocate
> Aml struct and return pointer to it and also add this pointer
> into AmlAllocList which is passed as first argument to each
> aml_foo() call.
> aml_append() becomes nondestructive function and just adds
> child(2nd arg) to the parent context (1st arg).
> 
> After API user is done with building table and pushed it
> into tables blob, he/she calls free_aml_alloc_list() to free
> all Aml objects created during process of building the table
> content.

Hmm, passing 'p' around somewhat muddies an otherwise clean
interface, but the concern with aml_append silently freeing
memory still accessible by the caller is definitely valid. I
only wonder how things would look with Igor's option 2.2 above.
The caller still only needs to free the final table, but it
also becomes safe to use the same object references multiple
times before freeing the table. Using QOM also seems reasonable
to me, as it appears it's the accepted way to do garbage
collection in QEMU. Is it possible to do 2.2 with QOM?

> 
> > 
> > > 
> > > Good idea! I think this will address the issue.
> > > 
> > > 
> > > > 3. talloc implementation Amit've mentioned,
> > > >    perhaps it might work since it allows to set destructors for
> > > >    managed pointers. With this we might get clear abort when
> > > >    dereferencing freed pointer see talloc_set()
> > > 
> > > 
> > > I think it's a separate discussion. Maybe talloc is a good
> > > allocator to use in qemu, but using a separate allocator
> > > just for acpi generation would be an overkill.
> > > 
> > > > 
> > > > > 
> > > > > Just pass AcpiAml* everywhere, add APIs to allocate and free it
> > > > > together with the internal buffer.
> > > > > This makes it trivial to see that value is not misused:
> > > > > just check it's between alloc and free - and that there are
> > > > > no leaks - just check we call free on each value.
> > > > > We can write a semantic patch to catch missing free calls,
> > > > > it's easy.
> > > > > 
> > > > > 
> > > > > > > 
> > > > > > > > As for moving to to another file, during all this series 
> > > > > > > > lowlevel
> > > > > > > > build_(some_aml_related_costruct_helper)s are moved into this 
> > > > > > > > file
> > > > > > > > and should be make static to hide from user lowlevel helpers
> > > > > > > > (including build_package).
> > > > > > > > That will leave only high level API available.
> > > > > > > > 
> > > > > > > > TODO for me: make sure that moved lowlevel helpers are static
> > > > > > > > 
> > > > > > > > 
> 
>