On Mon, Jan 26, 2015 at 04:34:01PM +0100, Andrew Jones wrote: > On Mon, Jan 26, 2015 at 04:09:20PM +0100, Igor Mammedov wrote: > > On Mon, 26 Jan 2015 10:57:21 +0100 > > Igor Mammedov <imamm...@redhat.com> wrote: > > > > > On Sat, 24 Jan 2015 18:33:50 +0200 > > > "Michael S. Tsirkin" <m...@redhat.com> 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" <m...@redhat.com> 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?
I'd rather not go there: QOM was really invented for introspection, and for long-lived heavy-weight objects. And to me, code using QOM is harder to understand than simple alloc/free. It's worth it where we need the features it offers, e.g. it has run-time checks where we previously just did a cast. But in this case I'd rather use something simpler and with compile-time checks. > > > > > > > > > > > > > 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 > > > > > > > > > > > > > > > > > > > > > >