use of uninitialized variable involving visit_type_uint32() and friends

use of uninitialized variable involving visit_type_uint32() and friends

[Peter Maydell]

Coverity warns about use of uninitialized data in what seems
to be a common pattern of use of visit_type_uint32() and similar
functions. Here's an example from target/arm/cpu64.c:

static void cpu_max_set_sve_max_vq(Object *obj, Visitor *v, const char *name,
                                   void *opaque, Error **errp)
{
    ARMCPU *cpu = ARM_CPU(obj);
    uint32_t max_vq;

    if (!visit_type_uint32(v, name, &max_vq, errp)) {
        return;
    }

    [code that does something with max_vq here]
}

This doesn't initialize max_vq, on the apparent assumption
that visit_type_uint32() will do so. But that function is:


bool visit_type_uint32(Visitor *v, const char *name, uint32_t *obj,
                       Error **errp)
{
    uint64_t value;
    bool ok;

    trace_visit_type_uint32(v, name, obj);
    value = *obj;
    ok = visit_type_uintN(v, &value, name, UINT32_MAX, "uint32_t", errp);
    *obj = value;
    return ok;
}

So it reads the value of *obj (the uninitialized max_vq).

What's the right way to write this kind of object-property
setter function? Just pre-initialize the variable to 0?

thanks
-- PMM

Re: use of uninitialized variable involving visit_type_uint32() and friends

[Daniel Henrique Barboza]

On 3/31/22 14:35, Peter Maydell wrote:
> Coverity warns about use of uninitialized data in what seems
> to be a common pattern of use of visit_type_uint32() and similar
> functions. Here's an example from target/arm/cpu64.c:
> 
> static void cpu_max_set_sve_max_vq(Object *obj, Visitor *v, const char *name,
>                                     void *opaque, Error **errp)
> {
>      ARMCPU *cpu = ARM_CPU(obj);
>      uint32_t max_vq;
> 
>      if (!visit_type_uint32(v, name, &max_vq, errp)) {
>          return;
>      }
> 
>      [code that does something with max_vq here]
> }
> 
> This doesn't initialize max_vq, on the apparent assumption
> that visit_type_uint32() will do so. But that function is:
> 
> 
> bool visit_type_uint32(Visitor *v, const char *name, uint32_t *obj,
>                         Error **errp)
> {
>      uint64_t value;
>      bool ok;
> 
>      trace_visit_type_uint32(v, name, obj);
>      value = *obj;
>      ok = visit_type_uintN(v, &value, name, UINT32_MAX, "uint32_t", errp);
>      *obj = value;
>      return ok;
> }
> 
> So it reads the value of *obj (the uninitialized max_vq).
> 
> What's the right way to write this kind of object-property
> setter function? Just pre-initialize the variable to 0?


This reminds me of Valgrind ppc-related warnings I sent patches yesterday. In a
code like this:

(target/ppc/kvm.c)

  int kvmppc_enable_cap_large_decr(PowerPCCPU *cpu, int enable)
  {
     CPUState *cs = CPU(cpu);
     uint64_t lpcr;

     kvm_get_one_reg(cs, KVM_REG_PPC_LPCR_64, &lpcr);
     /* Do we need to modify the LPCR? */
     if (!!(lpcr & LPCR_LD) != !!enable) {

Valgrind complains of "Conditional jump or move depends on uninitialised value(s)"
because we're using 'lpcr' in the conditional and 'lpcr' isn't being initialized.
Valgrind doesn't seem to care that kvm_get_one_reg() might be writing 'lpcr'.
The fix I proposed consists of initializing the vars in these cases.


My suggestion in this case is to initialize 'max_vq' as well. Apparently these static
code analysis tools don't handle the "var being initialized by being passed as reference
to another function" scenarios.


Thanks,


Daniel



> 
> thanks
> -- PMM
> 

Re: use of uninitialized variable involving visit_type_uint32() and friends

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 1925 bytes --]

On Thu, Mar 31, 2022 at 7:35 PM Peter Maydell <peter.maydell@linaro.org>
wrote:

> Coverity warns about use of uninitialized data in what seems
> to be a common pattern of use of visit_type_uint32() and similar
> functions. Here's an example from target/arm/cpu64.c:
>
> static void cpu_max_set_sve_max_vq(Object *obj, Visitor *v, const char
> *name,
>                                    void *opaque, Error **errp)
> {
>     ARMCPU *cpu = ARM_CPU(obj);
>     uint32_t max_vq;
>     if (!visit_type_uint32(v, name, &max_vq, errp)) {
>         return;
>     }
>     [code that does something with max_vq here]
> }
>
> This doesn't initialize max_vq, on the apparent assumption
> that visit_type_uint32() will do so. But that function [...]
> reads the value of *obj (the uninitialized max_vq).
>

The visit_type_* functions are written to work for both getters and setters.
For the leaves, that means potentially reading uninitialized data.  It is
harmless but very ugly, and with respect to static analysis it was all but
a time bomb, all the time.

The best (but most intrusive) solution would be to add a parameter to all
visit_type_* functions with the expected "direction" of the visit, which
could be checked against v->type.

That is:

bool visit_type_uint32(VisitorType expected_type, Visitor *v, const char
*name, uint32_t *obj,
                       Error **errp)
{
    uint64_t value;
    bool ok;

    trace_visit_type_uint32(v, name, obj);
    assert (v->type == expected_type);
    if (expected_type & (VISITOR_INPUT | VISITOR_DEALLOC)) {
        value = *obj;
    }
    ok = visit_type_uintN(v, &value, name, UINT32_MAX, "uint32_t", errp);
    assert (ok || expected_type == VISITOR_INPUT);
    if (expected_type & VISITOR_OUTPUT) {
        *obj = value;
    }
    return ok;
}

Probably also renaming VISITOR_* to V_* for conciseness.  That *should*
quiesce Coverity, and also add some runtime checks.

Paolo

[-- Attachment #2: Type: text/html, Size: 5224 bytes --]

Re: use of uninitialized variable involving visit_type_uint32() and friends

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On Thu, Mar 31, 2022 at 7:35 PM Peter Maydell <peter.maydell@linaro.org>
> wrote:
>
>> Coverity warns about use of uninitialized data in what seems
>> to be a common pattern of use of visit_type_uint32() and similar
>> functions. Here's an example from target/arm/cpu64.c:
>>
>> static void cpu_max_set_sve_max_vq(Object *obj, Visitor *v, const char
>> *name,
>>                                    void *opaque, Error **errp)
>> {
>>     ARMCPU *cpu = ARM_CPU(obj);
>>     uint32_t max_vq;
>>     if (!visit_type_uint32(v, name, &max_vq, errp)) {
>>         return;
>>     }
>>     [code that does something with max_vq here]
>> }
>>
>> This doesn't initialize max_vq, on the apparent assumption
>> that visit_type_uint32() will do so. But that function [...]
>> reads the value of *obj (the uninitialized max_vq).
>>
>
> The visit_type_* functions are written to work for both getters and setters.

Yes.

This is convenient for uses that are actually visitor-agnostic, such as
the generated qapi-visit-FOO.c

It can be really ugly for output-only uses.  In particular for strings,
where we have to pass a char ** instead of a const char *.

> For the leaves, that means potentially reading uninitialized data.  It is
> harmless but very ugly, and with respect to static analysis it was all but
> a time bomb, all the time.
>
> The best (but most intrusive) solution would be to add a parameter to all
> visit_type_* functions with the expected "direction" of the visit, which
> could be checked against v->type.
>
> That is:
>
> bool visit_type_uint32(VisitorType expected_type, Visitor *v, const char
> *name, uint32_t *obj,
>                        Error **errp)
> {
>     uint64_t value;
>     bool ok;
>
>     trace_visit_type_uint32(v, name, obj);
>     assert (v->type == expected_type);
>     if (expected_type & (VISITOR_INPUT | VISITOR_DEALLOC)) {
>         value = *obj;
>     }
>     ok = visit_type_uintN(v, &value, name, UINT32_MAX, "uint32_t", errp);
>     assert (ok || expected_type == VISITOR_INPUT);
>     if (expected_type & VISITOR_OUTPUT) {
>         *obj = value;
>     }
>     return ok;
> }

As diff -w:

 -bool visit_type_uint32(Visitor *v, const char *name, uint32_t *obj,
 +bool visit_type_uint32(VisitorType expected_type, Visitor *v, const char
 +*name, uint32_t *obj,
                         Error **errp)
  {
      uint64_t value;
      bool ok;

      trace_visit_type_uint32(v, name, obj);
 +    assert (v->type == expected_type);
 +    if (expected_type & (VISITOR_INPUT | VISITOR_DEALLOC)) {

Backwards.

With an input visitor @v,

    visit_type_uint32(v, "name", &val, errp)

stores to @val without looking at it first.  In other words,
uninitialized @val is fine, just like for val = ...

Note: you don't actually need VISITOR_DEALLOC here, because a
deallocation visitor isn't going to do anything for non-pointer values.

With an output visitor @v,

    visit_type_uint32(v, "name", &val, errp)

reads from @val without changing it.

      value = *obj;
 +    }
      ok = visit_type_uintN(v, &value, name, UINT32_MAX, "uint32_t", errp);
 +    assert (ok || expected_type == VISITOR_INPUT);
 +    if (expected_type & VISITOR_OUTPUT) {

Also backwards.  

      *obj = value;
 +    }
      return ok;
  }

Two changes:

* Skip copying to and from full-width buffer @value:

  - Skip @value = *obj when we're going to overwrite @value without
    reading it first.

    This leaves @value uninitialized instead of initializing it from a
    (commonly) uninitialized variable.

    I'm not sure how this helps static analysis, but if it does...

  - Skip *obj = @value when value must be *obj anyway.

    Should have no observable effect.

    Again, I'm not sure how this helps static analysis.

  Note that only the functions for types narrower than 64 bits have such
  copying code.  Skipping the assignments creates a tiny inconsistency
  between narrow and fill-width visits.

* Pass visitor type in addition to the visitor.  Can you explain why
  that's useful?

> Probably also renaming VISITOR_* to V_* for conciseness.  That *should*
> quiesce Coverity, and also add some runtime checks.
>
> Paolo

Re: use of uninitialized variable involving visit_type_uint32() and friends

[Paolo Bonzini]

On 4/1/22 11:15, Markus Armbruster wrote:
>   +    assert (v->type == expected_type);
>   +    if (expected_type & (VISITOR_INPUT | VISITOR_DEALLOC)) {
> 
> Backwards.

Yes, I always get input vs output wrong.

> With an input visitor @v,
> 
>      visit_type_uint32(v, "name", &val, errp)
> 
> stores to @val without looking at it first.  In other words,
> uninitialized @val is fine, just like for val = ...
> 
> Note: you don't actually need VISITOR_DEALLOC here, because a
> deallocation visitor isn't going to do anything for non-pointer values.

There's a philosophical question on whether other deallocation visitors 
can exist than "the" deallocation visitor, but it's not particularly 
germane to the topic.

> Two changes:
> 
> * Skip copying to and from full-width buffer @value:
> 
>    - Skip @value = *obj when we're going to overwrite @value without
>      reading it first.
> 
>      This leaves @value uninitialized instead of initializing it from a
>      (commonly) uninitialized variable.
> 
>      I'm not sure how this helps static analysis, but if it does...

If it can do really serious interprocedural analysis, it _might_ be able 
to see through the visitor constructor and know that the "value = *obj" 
is not initialized (e.g. "all callers of object_property_set use an 
input visitor").  I doubt that honestly, but a man can dream.

If the conditionals are enough to shut it up, then we won the battle 
(for now).

If the conditionals are not enough to shut it up, then you have a bit 
more confidence when marking the false positives.

>    - Skip *obj = @value when value must be *obj anyway.
> 
>      Should have no observable effect.
> 
>      Again, I'm not sure how this helps static analysis.

Mostly consistency, could also be changed to an assert(*obj == value); 
/* output visitors don't really need obj to be passed by reference */

> * Pass visitor type in addition to the visitor.  Can you explain why
>    that's useful?

Because it communicates what the caller expects: "I have left this 
uninitialized because I expect my "v" argument to be the kind of visitor 
that fills it in".  It's this argument that gives me the confidence 
needed to shut up Coverity's false positives.

Embedding the visitor type in the signature makes it impossible not to 
pass it, unlike e.g. an assertion in every getter or setter.

Paolo

Re: use of uninitialized variable involving visit_type_uint32() and friends

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 4/1/22 11:15, Markus Armbruster wrote:
>>   +    assert (v->type == expected_type);
>>   +    if (expected_type & (VISITOR_INPUT | VISITOR_DEALLOC)) {
>> 
>> Backwards.
>
> Yes, I always get input vs output wrong.

Output becomes input becomes output.  How not to be confused!

>> With an input visitor @v,
>> 
>>      visit_type_uint32(v, "name", &val, errp)
>> 
>> stores to @val without looking at it first.  In other words,
>> uninitialized @val is fine, just like for val = ...
>> 
>> Note: you don't actually need VISITOR_DEALLOC here, because a
>> deallocation visitor isn't going to do anything for non-pointer values.
>
> There's a philosophical question on whether other deallocation visitors 
> can exist than "the" deallocation visitor, but it's not particularly 
> germane to the topic.

Agreed.  Same for "the" clone visitor.

>> Two changes:
>> 
>> * Skip copying to and from full-width buffer @value:
>> 
>>    - Skip @value = *obj when we're going to overwrite @value without
>>      reading it first.
>> 
>>      This leaves @value uninitialized instead of initializing it from a
>>      (commonly) uninitialized variable.
>> 
>>      I'm not sure how this helps static analysis, but if it does...
>
> If it can do really serious interprocedural analysis, it _might_ be able 
> to see through the visitor constructor and know that the "value = *obj" 
> is not initialized (e.g. "all callers of object_property_set use an 
> input visitor").  I doubt that honestly, but a man can dream.

I'm wary of arguments based on "a sufficiently smart compiler can"...

> If the conditionals are enough to shut it up, then we won the battle 
> (for now).

If they get us more milage per unit of work out of Coverity, I'm in
favor.  I'll want a comment explaining the conditionals, though.

> If the conditionals are not enough to shut it up, then you have a bit 
> more confidence when marking the false positives.
>
>>    - Skip *obj = @value when value must be *obj anyway.
>> 
>>      Should have no observable effect.
>> 
>>      Again, I'm not sure how this helps static analysis.
>
> Mostly consistency, could also be changed to an assert(*obj == value); 
> /* output visitors don't really need obj to be passed by reference */

I guess whatever is easier to explain in a comment.

>> * Pass visitor type in addition to the visitor.  Can you explain why
>>    that's useful?
>
> Because it communicates what the caller expects: "I have left this 
> uninitialized because I expect my "v" argument to be the kind of visitor 
> that fills it in".  It's this argument that gives me the confidence 
> needed to shut up Coverity's false positives.
>
> Embedding the visitor type in the signature makes it impossible not to 
> pass it, unlike e.g. an assertion in every getter or setter.

I think we got two kinds of code calling visitor methods:

1. Code for use with one kind of visitor only

   We get to pass a literal argument to the additional parameter you
   propose.

2. Code for use with arbitrary visitors (such as qapi-visit*.c)

   We need to pass v->type, where @v is the existing visitor argument.
   Except we can't: struct Visitor and VisitorType are private, defined
   in <visitor-impl.h>.  Easy enough to work around, but has a distinct
   "this design is falling apart" smell, at least to me.

Note that "intent explicit in every method call" is sufficient, but not
necessary for "intent is locally explicit, which lets us dismiss false
positives with confidence".  We could do "every function that calls
methods".  Like checking a precondition.  We already have
visit_is_input().  We could have visit_is_output().

The sane way to make output intent explicit is of course passing the
thing by value rather than by reference.  To get that, we could generate
even more code.  So, if the amount of code we currently generate isn't
disgusting enough, ...

Re: use of uninitialized variable involving visit_type_uint32() and friends

[Paolo Bonzini]

On 4/1/22 15:11, Markus Armbruster wrote:
>> If it can do really serious interprocedural analysis, it _might_ be able
>> to see through the visitor constructor and know that the "value = *obj"
>> is not initialized (e.g. "all callers of object_property_set use an
>> input visitor").  I doubt that honestly, but a man can dream.
> 
> I'm wary of arguments based on "a sufficiently smart compiler can"...

Absolutely.

>> Because it communicates what the caller expects: "I have left this
>> uninitialized because I expect my "v" argument to be the kind of visitor
>> that fills it in".  It's this argument that gives me the confidence
>> needed to shut up Coverity's false positives.
>>
>> Embedding the visitor type in the signature makes it impossible not to
>> pass it, unlike e.g. an assertion in every getter or setter.
> 
> I think we got two kinds of code calling visitor methods:
> 
> 1. Code for use with one kind of visitor only
> 
>     We get to pass a literal argument to the additional parameter you
>     propose.
> 
> 2. Code for use with arbitrary visitors (such as qapi-visit*.c)
> 
>     We need to pass v->type, where @v is the existing visitor argument.
>     Except we can't: struct Visitor and VisitorType are private, defined
>     in <visitor-impl.h>.  Easy enough to work around, but has a distinct
>     "this design is falling apart" smell, at least to me.

Hmm, maybe that's a feature though.  If we only need v->type in .c files 
for the generated visit_type_* functions, then it's not a huge deal that 
they will have to include <visitor-impl.h>.  All callers outside 
generated type visitors (which includes for example QMP command 
marshaling), instead, would _have_ to pass visitor type constants and 
make it clear what direction the visit is going.

> Note that "intent explicit in every method call" is sufficient, but not
> necessary for "intent is locally explicit, which lets us dismiss false
> positives with confidence".  We could do "every function that calls
> methods".  Like checking a precondition.  We already have
> visit_is_input().  We could have visit_is_output().
> 
> The sane way to make output intent explicit is of course passing the
> thing by value rather than by reference.  To get that, we could generate
> even more code.  So, if the amount of code we currently generate isn't
> disgusting enough, ...

Yeah, that would be ugly.  Or, we could generate the same code plus some 
static inline wrappers that take a

   struct InputVisitor {
       Visitor dont_use_me_it_hurts;
   }
   struct OutputVisitor {
       Visitor dont_use_me_it_hurts;
   }

That would be zero-cost abstraction at runtime.

Paolo

Re: use of uninitialized variable involving visit_type_uint32() and friends

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 4/1/22 15:11, Markus Armbruster wrote:
>>> If it can do really serious interprocedural analysis, it _might_ be able
>>> to see through the visitor constructor and know that the "value = *obj"
>>> is not initialized (e.g. "all callers of object_property_set use an
>>> input visitor").  I doubt that honestly, but a man can dream.
>> 
>> I'm wary of arguments based on "a sufficiently smart compiler can"...
>
> Absolutely.
>
>>> Because it communicates what the caller expects: "I have left this
>>> uninitialized because I expect my "v" argument to be the kind of visitor
>>> that fills it in".  It's this argument that gives me the confidence
>>> needed to shut up Coverity's false positives.
>>>
>>> Embedding the visitor type in the signature makes it impossible not to
>>> pass it, unlike e.g. an assertion in every getter or setter.
>> 
>> I think we got two kinds of code calling visitor methods:
>> 
>> 1. Code for use with one kind of visitor only
>> 
>>     We get to pass a literal argument to the additional parameter you
>>     propose.
>> 
>> 2. Code for use with arbitrary visitors (such as qapi-visit*.c)
>> 
>>     We need to pass v->type, where @v is the existing visitor argument.
>>     Except we can't: struct Visitor and VisitorType are private, defined
>>     in <visitor-impl.h>.  Easy enough to work around, but has a distinct
>>     "this design is falling apart" smell, at least to me.
>
> Hmm, maybe that's a feature though.  If we only need v->type in .c files 
> for the generated visit_type_* functions, then it's not a huge deal that 
> they will have to include <visitor-impl.h>.  All callers outside 
> generated type visitors (which includes for example QMP command 
> marshaling), instead, would _have_ to pass visitor type constants and 
> make it clear what direction the visit is going.

I quoted the generated qapi-visit*.c as an example.  There may
handwritten instances, too.

>> Note that "intent explicit in every method call" is sufficient, but not
>> necessary for "intent is locally explicit, which lets us dismiss false
>> positives with confidence".  We could do "every function that calls
>> methods".  Like checking a precondition.  We already have
>> visit_is_input().  We could have visit_is_output().
>> 
>> The sane way to make output intent explicit is of course passing the
>> thing by value rather than by reference.  To get that, we could generate
>> even more code.  So, if the amount of code we currently generate isn't
>> disgusting enough, ...
>
> Yeah, that would be ugly.  Or, we could generate the same code plus some 
> static inline wrappers that take a
>
>    struct InputVisitor {
>        Visitor dont_use_me_it_hurts;
>    }
>    struct OutputVisitor {
>        Visitor dont_use_me_it_hurts;
>    }
>
> That would be zero-cost abstraction at runtime.

Looks worth exploring!

Re: use of uninitialized variable involving visit_type_uint32() and friends

[Peter Maydell]

On Fri, 1 Apr 2022 at 09:08, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
>
> On Thu, Mar 31, 2022 at 7:35 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> Coverity warns about use of uninitialized data in what seems
>> to be a common pattern of use of visit_type_uint32() and similar
>> functions. Here's an example from target/arm/cpu64.c:
>>
>> static void cpu_max_set_sve_max_vq(Object *obj, Visitor *v, const char *name,
>>                                    void *opaque, Error **errp)
>> {
>>     ARMCPU *cpu = ARM_CPU(obj);
>>     uint32_t max_vq;
>>     if (!visit_type_uint32(v, name, &max_vq, errp)) {
>>         return;
>>     }
>>     [code that does something with max_vq here]
>> }
>>
>> This doesn't initialize max_vq, on the apparent assumption
>> that visit_type_uint32() will do so. But that function [...]
>> reads the value of *obj (the uninitialized max_vq).
>
>
> The visit_type_* functions are written to work for both getters and setters.
> For the leaves, that means potentially reading uninitialized data.  It is
> harmless but very ugly, and with respect to static analysis it was all but
> a time bomb, all the time.
>
> The best (but most intrusive) solution would be to add a parameter to all
> visit_type_* functions with the expected "direction" of the visit, which
> could be checked against v->type.

So do we have a plan for what we want to do with this issue?

In the meantime, any objections to my just marking all the Coverity
issues which are pointing out that visit_* functions use uninitialized
data as 'false positive' ? There are a ton of them, and they clog up
the issue UI and make it hard to see other actually interesting reports.

thanks
-- PMM

Re: use of uninitialized variable involving visit_type_uint32() and friends

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Fri, 1 Apr 2022 at 09:08, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>>
>>
>> On Thu, Mar 31, 2022 at 7:35 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>>>
>>> Coverity warns about use of uninitialized data in what seems
>>> to be a common pattern of use of visit_type_uint32() and similar
>>> functions. Here's an example from target/arm/cpu64.c:
>>>
>>> static void cpu_max_set_sve_max_vq(Object *obj, Visitor *v, const char *name,
>>>                                    void *opaque, Error **errp)
>>> {
>>>     ARMCPU *cpu = ARM_CPU(obj);
>>>     uint32_t max_vq;
>>>     if (!visit_type_uint32(v, name, &max_vq, errp)) {
>>>         return;
>>>     }
>>>     [code that does something with max_vq here]
>>> }
>>>
>>> This doesn't initialize max_vq, on the apparent assumption
>>> that visit_type_uint32() will do so. But that function [...]
>>> reads the value of *obj (the uninitialized max_vq).
>>
>>
>> The visit_type_* functions are written to work for both getters and setters.
>> For the leaves, that means potentially reading uninitialized data.  It is
>> harmless but very ugly, and with respect to static analysis it was all but
>> a time bomb, all the time.
>>
>> The best (but most intrusive) solution would be to add a parameter to all
>> visit_type_* functions with the expected "direction" of the visit, which
>> could be checked against v->type.
>
> So do we have a plan for what we want to do with this issue?
>
> In the meantime, any objections to my just marking all the Coverity
> issues which are pointing out that visit_* functions use uninitialized
> data as 'false positive' ? There are a ton of them, and they clog up
> the issue UI and make it hard to see other actually interesting reports.

I think to object, I would have to propose an alternative with a
reasonable chance of success within a reasonable time frame.  I can't,
so I won't.

Paolo proposed improvements, and I think they're worth exploring, but
such musings fall well short of my condition for "may object".