Re: [PATCH v3 0/9] Require error handling for dynamically created objects

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> NB, this series is targetting 10.0, NOT for 9.2 freeze.
>
> With code like
>
>     Object *obj = object_new(TYPE_BLAH)
>
> the caller can be pretty confident that they will successfully create
> an object instance of TYPE_BLAH. They know exactly what type has been
> requested, so it passing an abstract type for example, it is a clear
> programmer error that they'll get an assertion failure.
>
> Conversely with code like
>
>    void somefunc(const char *typename) {
>       Object * obj = object_new(typename)
>       ...
>    }
>
> all bets are off, because the call of object_new() knows nothing
> about what 'typename' resolves to.

We know nothing *locally*.

Commonly, a non-local argument can demonstrate safety.  Only when the
type name comes from the user, we truly know nothing.

>                                    It could easily be an abstract
> type.

It could also be no type at all.

>       As a result, many code paths have added a manual check ahead
> of time
>
>    if (object_class_is_abstract(typename)) {
>       error_setg(errp, ....)
>    }

Actually, object_class_is_abstract() takes an ObjectClass, not a type
name string.

The actual guards we use are variations of

    klass = object_class_by_name(typename);
    if (!klass) {
        error_setg(errp, "invalid object type: %s", typename);
        return NULL;
    }

    if (object_class_is_abstract(klass)) {
        error_setg(errp, "object type '%s' is abstract", typename);
        return NULL;
    }

which covers "no type at all", too.

Sometimes, we use module_object_class_by_name() instead, which I believe
additionally loads the module providing the type, if any.  Which of the
two should be used where is a mystery to me, and I suspect we're getting
it wrong in places.  But this is turning into a digression.  To
hopefully maintain focus, I'm pretending modules don't exist until later
in this message.

Sometimes, we throw in an object_class_dynamic_cast(klass, T) to check
@typename resolves to a subtype of some T.

> ...except for where we forget to do this, such as qdev_new().

We did not forget it there!  It's by design a thin wrapper around
object_new(), with preconditions just like object_new().

> Overall 'object_new' is a bad design because it is inherantly
> unsafe to call with unvalidated typenames.

To be fair, object_new() was not designed for use with user-provided
type names.  When it chokes on type names not provided by the user, it's
clearly a programming error, and assert() is a perfectly fine way to
catch programming errors.  Same for qdev_new().

However, we do in fact use these functions with user-provided type
names, if rarely.  When we do, we need to validate the type name before
we pass it to them.

Trouble is the validation code is a bit involved, and reimplementing it
everywhere it's needed is asking for bugs.

Creating and using more interfaces that are more convenient for this
purpose would avoid that.

> This problem is made worse by the proposal to introduce the idea
> of 'singleton' classes[1].
>
> Thus, this series suggests a way to improve safety at build
> time. The core idea is to allow 'object_new' to continue to be
> used *if-and-only-if* given a static, const string, because that
> scenario indicates the caller is aware of what type they are
> creating at build time.
>
> A new 'object_new_dynamic' method is proposed for cases where
> the typename is dynamically chosen at runtime. This method has
> an "Error **errp" parameter, which can report when an abstract
> type is created, leaving the assert()s only for scenarios which
> are unambiguous programmer errors.
>
> With a little macro magic, we guarantee a compile error is
> generated if 'object_new' is called with a dynamic type, forcing
> all potentially unsafe code over to object_new_dynamic.

Three cases:

1. Type name is literal string.  No change.  This is the most common
   case.

2. It's not.

2a. Type name is user-provided.  This is rare.  We replace

        if (... guard ...) {
            ... return failure ...
        }
        obj = object_new(...);

    by

        obj = object_new_dynamic(..., errp);
        if (!obj) {
            ... return failure ...
        }

    This is an improvement.

2b. It's not.  We should replace

        obj = object_new(...);

    by

        obj = object_new_dynamic(..., &error_abort);

    Exact same behavior, just wordier, to placate the compiler.
    Tolerable as long as it's relatively rare.

    But I'm not sure it's worthwhile.  All it really does is helping
    some towards not getting case 2a wrong.  But 2a is rare.

Same for similar interfaces, e.g. qdev_new().

> This is more tractable than adding 'Error **errp' to 'object_new'
> as only a handful of places use a dynamic type name.

True!

Alright, enter modules.

Modules break a fundamental design assumption: object_new() on a
compiled-in type name is safe, i.e. the failure modes are all
programming errors.

Modules add new failure modes that are *not* programming errors:

* The module providing the type was not deployed correctly.

* It was, but the host system lacks the resources to load it.

Before modules, object_new(T) was safe unless T was user-provided.
Which implies it's safe when T is a literal string.

Since modules, object_new(T) is safe unless T is user-provided or the
type named by it is compiled as module.  This does *not* imply it's safe
when T is a literal string.

When looking at a use of object_new(), whether the argument names a type
that could be compiled as module cannot be known locally.  Therefore, we
cannot know locally whether we need to handle failure, either with a
suitable guard or by switching to a new function like
object_new_dynamic().  This is bad.

Breaking fundamental design assumptions tends to have ugly and expensive
consequences.  Consequences like having to rework every single call of
object_new() & friends.

Can we reduce the damage?  Maybe.  What if we create a
module_object_new() that takes an Error **, and make object_new() crash
& burn when the @typename argument resolves to a type provided by a
module?

Maybe module_object_new() and object_new_dynamic() could be fused into a
single function with a better name.

> With this series, my objections to Peter Xu's singleton series[1]
> would be largely nullified.
>
> [1] https://lists.nongnu.org/archive/html/qemu-devel/2024-10/msg05524.html