[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v10 12/13] qapi: Delete unused visit_start_union
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v10 12/13] qapi: Delete unused visit_start_union() |
Date: |
Wed, 17 Feb 2016 14:19:40 -0700 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.0 |
On 02/17/2016 11:08 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> writes:
>
>> Commit cee2dedb noticed that if you have a partial flat union
>> (such as if an input parse failed due to a missing
>> discriminator), calling the dealloc visitor could result in
>> trying to dereference a NULL pointer if we attempted to visit
>> an object branch without an earlier successful call to
>> visit_start_implicit_struct() allocating the pointer for that
>> branch. But the "fix" it implemented requires the use of a
>> '.data' member in the union, which may or may not be the same
>> size as other branches of the union (consider a 32-bit platform
>> where one of the branches is an int64), which feels fairly dirty.
>
> Well, until the previous commit, it was the same, wasn't it? All
> pointers.
For simple unions, you could have (well, still can have, until my later
patch gets rid of the simple_union_type() magic):
struct SU {
SUKind type;
union {
void *data;
int8_t byte;
} u;
};
But you're right - for flat unions, ALL branches were represented as
pointers (until this series unboxed them).
>
>> Plus, as mentioned in that commit, it only works if you can
>> assume that '.data' would be zero-initialized even if '.kind' was
>> uninitialized, which is rather poor logic: our usage of
>> visit_start_struct() happens to zero-initialize both fields,
>> which means '.kind' is never truly uninitialized - but if we
>> changed visit_start_struct() to use g_new() instead of g_new0(),
>> then '.data' would not be any more reliable as a condition on
>> whether to visit the branch matching '.kind', regardless of
>> whether '.kind' was 0).
>>
>> Menawhile, now that we have just inlined the fields of all flat
Meanwhile,
>> unions, there is no longer the possibility of a null pointer to
>> dereference in the first place. Where the branch structure used
>> to be separately allocated by visit_start_implicit_struct(), it
>> is now just pointing to a subset of the memory already
>> zero-allocated by visit_start_struct().
I guess I may try and reword this slightly, and point to the fact that
the NULL dereference was due to calling visit_start_implicit_FOO() (only
done for flat unions; for simple unions the branches call
visit_type_FOO(), and that call safely handled NULL); because we were
using visit_start/end_implicit_struct() for its allocation effects. But
the net result is the same - now that we no longer call
visit_start_implicit_struct() for a union visit, the dealloc visitor no
longer has to worry about a NULL dereference on a partially constructed
object, so we no longer need to probe if the union contains any data.
>> +++ b/scripts/qapi-visit.py
>> @@ -246,9 +246,6 @@ void visit_type_%(c_name)s(Visitor *v, const char *name,
>> %(c_name)s **obj, Error
>> if variants:
>> ret += gen_err_check(label='out_obj')
>> ret += mcgen('''
>> - if (!visit_start_union(v, !!(*obj)->u.data, &err) || err) {
>> - goto out_obj;
>> - }
>
> I'm afraid the previous commit broke this for flat unions.
>
> Before the previous commit, all members of (*obj)->u were pointers to
> the struct holding the variant members both for flat and simple unions.
> !!(*obj)->u.data tests whether the struct holding the variant members
> has been allocated. This relies on uniform pointer format.
>
> The dealloc visitor uses the "has been allocated" bit to suppress
> visiting the struct when it hasn't been allocated.
>
> The previous commit unboxes the struct for flat unions. Now ->u.data
> reinterprets the first few bytes of that struct as pointer. If you're
> "lucky", they're not all zero, and the struct gets visited.
You're right - and I bet I could come up with a case where valgrind
could call me on it.
>
> Obvious fix: squash this hunk into the previous commit, then let this
> commit drop the code that's no longer used.
Yep, for bisectability, I think that's what I'll end up doing.
>
> However, simple unions are still boxed. Why can't their pointer be null
> in the dealloc visitor?
Simple unions still go through visit_type_FOO(), and _that_ function
properly checks for NULL. It was only visit_type_implicit_FOO() that
blindly dereferenced things. In fact, in the earlier incantation of
this patch, my fix was to teach visit_type_implicit_FOO() how to check
for NULL:
https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg05442.html
But now that visit_type_implicit_FOO() is gone, my earlier incantation
got reduced in size. I guess it's all in how I document the commit message.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
- Re: [Qemu-devel] [PATCH v10 11/13] qapi: Don't box branches of flat unions, (continued)
[Qemu-devel] [PATCH v10 13/13] qapi: Change visit_start_implicit_struct to visit_start_alternate, Eric Blake, 2016/02/15
[Qemu-devel] [PATCH v10 09/13] qapi: Emit structs used as variants in topological order, Eric Blake, 2016/02/15
[Qemu-devel] [PATCH v10 12/13] qapi: Delete unused visit_start_union(), Eric Blake, 2016/02/15