Sdbusplus allows serializing variant with a contained variant

[edtanous]

https://gerrit.openbmc.org/c/openbmc/bmcweb/+/67652

Exposed a minor problem in sdbusplus in that it allows serializing a double-wrapped variant container of dbus type.

v{v{d}}

Wrapping a variant within a variant on dbus is nonsensisical. This should ideally be a compile-time error when trying to instantiate a type_id of std::variant<std::variant>.

Opening this bug just for documentation at the moment.

Here is where the read call is getting instantiated. https://github.com/openbmc/sdbusplus/blob/ae01928016f2983aa44f1279a2575572514953f7/include/sdbusplus/message/read.hpp#L344

And this is the type_id definition for std::variant. I suspect we can put a static_assert here. https://github.com/openbmc/sdbusplus/blob/ae01928016f2983aa44f1279a2575572514953f7/include/sdbusplus/message/types.hpp#L260C75-L260C75

[williamspatrick]

Is this something that the dbus set-property does some magic where if the thing is already a variant it ignores the variant for the set-property call?

[edtanous]

I would rather we just make it a compile-time failure rather than trying to ignore the extra variant. That keeps the code paths the same throughout everything, and I can't think of a good reason to call set-property with a variant, when the code itself knows the correct type.

[williamspatrick]

I need to do some experimentation on this. If this is really a problem (variant in variant not allowed) then I suspect that all the client/server bindings are currently broken. If they aren’t broken, then I’m not sure what the catch would be.

[zevweiss]

FWIW, from what I recall when I accidentally tripped over this issue earlier this year, while I'm not sure why, dbus does seem to allow nested variants (both in terms of the spec and the implementation of dbus-broker). I'd also be generally inclined to have the library API make it a compile-time error since it seems like such a pointless footgun, but the fact that it is strictly speaking allowed makes me wonder if there might be some bizarro pathological case out there that's (perhaps accidentally) ended up with a nested variant baked into its interfaces and hence if it might be something a library intended for general-purpose use (i.e. not OpenBMC-specific) might want/need to support despite it being "obviously a bad idea"...

[williamspatrick]

My thinking is, without experimentation, that it is required to be supported. Consider a property which is itself a variant. The Set-Property call has to take a variant-variant, unless there is some explicit handling that collapses the outer variant in that case.