Replace serde-xml-rs by quick-xml #295

Closed zeenix closed 1 year ago

In GitLab by @tyrylu on Nov 2, 2022, 17:02

As quickly mentioned in the dependency update issue, I'd personally replace the xml parsing crate. It would make cargo audit happy, and if wanted, it would allow getting rid of the enums which handle the possibly overlapping lists in the XML definitions, yes, of course at a potential cost... What do you think, should I proceed with a MR? And would you change xml APIs as well?

In GitLab by @elmarco on Nov 2, 2022, 17:07

"yes, of course at a potential cost..."

Like what?

What do you think

If serde-xml-rs is dead, I am all for it.

In GitLab by @tyrylu on Nov 2, 2022, 17:17

Well, the docs talk about a potential denial of service on an unclosed container element, and also about quadratic behavior if more lists are parsed from the container. See https://docs.rs/quick-xml/latest/quick_xml/#features for the exact description for the overlapped-lists feature. And for the advisory, see https://rustsec.org/advisories/RUSTSEC-2022-0048.html for yourself, and yes, the issues on xml-rs does not look promising.

What do you think, should I proceed with a MR? And would you change xml APIs as well?

Yeah. I just have one question: are we exposing serde-xml in our API (i-e is this a breaking change)?

In GitLab by @tyrylu on Nov 3, 2022, 08:34

Actually, it seems it would, at least for the publicly exposed Error enum it would require replacing the SerdeXml variant with a QuickXml variant or something along these lines. And of course, accepting the overlapped lists feature would change the xml interface entirely, or it would allow it to.

Ouch. Since this API is behind a feature, how about:

we enable the new quick-xml behind a new feature (say quick-xml).
deprecate the serde-xml API (and/or the xml feature).
In the deprecation warning message, we declare that quick-xml will be a default feature when xml is dropped.

WDYT?

If we go this route, it'd be great for the new API to make use of zbus_names (which zbus imports as names mod) to use strong types instead of strings where possible.

In GitLab by @tyrylu on Nov 4, 2022, 10:04

That sounds like a plan. So, to make thinks clear, we go with the overlapped lists feature and do all the xml API redesign which this allows, right?

That sounds like a plan.

:thumbsup:

we go with the overlapped lists feature

That's too meta for my early-in-the-day brain. :) What do you mean by "overlapped lists feature"?

do all the xml API redesign

yeah if it's behind a non-default new feature, we can do pretty much whatever we want. :)

btw, once your MR for this is created, kindly create an issue for tracking the feature switch with API break label applied to it. I'm trying to collect all API breaking changes needed so we can handle them all at once when the time is right.

In GitLab by @tyrylu on Nov 4, 2022, 14:07

If we use the overlapped-lists feature of quick_xml, it should be possible to remove the InterfaceElement and related indirections. And I'll not forget the issue. :-)

Ah, you assumed I'm familiar w/ quick-xml API. :) Anyway, looks a bit of a bad idea to me cause the docs say:

Note, that enabling this feature can lead to high and even unlimited memory consumption

but I'll be ok with that if we add a hard limit on the size of the xml.

Oh it seems quick-xml provides a solution.

In GitLab by @tyrylu on Nov 4, 2022, 14:49

Well, I did not catch this helpful method either, I wanted to suggest keeping the enums, but it appears we can remove them, then. What limit would you put there, because you likely saw much more interface descriptions than I.

, I wanted to suggest keeping the enums, but it appears we can remove them,

Can you please elaborate a little. Why do we want to remove enums? :thinking:

What limit would you put there, because you likely saw much more interface descriptions than I.

How about we start with 1024? Should be more than enough for most (TM).

In GitLab by @tyrylu on Nov 4, 2022, 16:09

Agreed, lets put 1024 there. For the API simplifactions. The InterfaceElement etc. were there only to allow serde_xml to parse the data, and now they're redundant. And, fortunately, removal of these will not even be an API break because of the accessor methods.

mentioned in commit 3edd78ce2934bd428453cdbc520646fd932f52ad

I started wondering today why cargo audit that runs as part of our CI never complained about this. I then found out that serde-xml-rs seems very much maintained (the last release was in Aug 2022).

@tyrylu Is there any chance you confused xml-rs with serde-xml-rs? :face_palm: The advisory you point to is about xml-rs crate, not serde-xml-rs.

Ah, serde-xml-rs is based on xml-rs. Still, wondering why cargo audit never complained. :thinking:

nm, there is a warning. Not an error.

dbus2 / zbus-old

Replace serde-xml-rs by quick-xml #295