mlxml: preserve percent-encoding in URI paths

[ssahani]

xmlParseURI automatically decodes percent-encoded characters in the path component, which causes issues when paths contain %2f (encoded forward slash). For example, a VMX file path containing '%2f' would be decoded to '/', creating an invalid double-slash in the path.

Extract the raw path from the original URI string before xmlParseURI processes it, preserving percent-encoding like %2f. This allows proper handling of paths with special characters that need to remain encoded.

Fixes: https://issues.redhat.com/browse/RHEL-136481

[rwmjones]

What is this file for?

[rwmjones]

I don't understand why this change is needed at all. Can you explain what problem you're trying to solve, ideally with examples.

In general, trying to hand-parse URIs is a big code smell. Libraries (like libxml2) exist to take care of the many details, standards and complications around URIs. Therefore the barrier to entry for this code is much larger than normal since it is doing something that is probably inadvisable. And since I don't understand the purpose of this code, I cannot right now think about accepting it.

[ssahani]

This file name
esx8.0-rhel9.6-x86_64-efi-disk-name-contains-specical-chars-%2f/esx8.0-rhel9.6-x86_64-efi-disk-name-contains-specical-chars-%2f.vmx

is getting changed into

esx8.0-rhel9.6-x86_64-efi-disk-name-contains-specical-chars-//esx8.0-rhel9.6-x86_64-efi-disk-name-contains-specical-chars-/.vmx

This path does not exist . xmlParseURI automatically decodes percent-encoded characters (e.g. %2f becomes /), which corrupts paths where %2f is a literal part of the filename on VMFS/ESXi filesystems. xmlParseURIRaw with raw=1 preserves the percent-encoding so the path is passed to SFTP exactly as the user specified it.

[crobinso]

This changes the semantics of this API. You'd need to audit every user that's calling parse_uri throughout libguestfs code to see if they are passing in escaped URIs or not. If they are expecting the URI to be unescaped, this patch breaks those callers.

If the path is literally esx8.0-rhel9.6-x86_64-efi-disk-name-contains-specical-chars-%2f/esx8.0-rhel9.6-x86_64-efi-disk-name-contains-specical-chars-%2f.vmx, the valid RI form of that would be splitting on the / and passing each part through URL escaping, so '%2f' becomes '%252f'.

As other patches have addressed, vmware has obvious bugs here. So IMO probably better to finding whereever this path is passed to parse_uri in our vmware specific code, and URI escaping it first.

[rwmjones]

The proposed commit now changes Xml.parse_uri for all callers throughout virt-v2v and guestfs-tools (I counted at least 13 places where this function is called). It's doubtful this change is correct for all of them, and I also doubt that you have studied those call sites.

mlxml is essentially a mini-binding around the xml* functions from libxml2, as it says in the documentation. If a binding around xmlParseURIRaw is needed then it'll need to be a separate binding in this library (eg. Xml.parse_uri_raw : string -> bool -> uri).

[ssahani]

Thanks for the reviews. Updated .

[rwmjones]

It's OK, but the whole of the code building the tuple is now duplicated. It'd be nice not to have that.

It's hard to understand from the libxml2 docs what xmlParseURIRaw even does. I had to look at the source code. It causes some parts of the URI (at least, authority, hostname, path, query string) to be left %-encoded rather than being decoded.

The libxml2 function isn't used anywhere else as far as I can see. But glib's g_uri_parse does have a similar feature (G_URI_FLAGS_ENCODED) so I guess that's fine. URI parsing is hard, news at 11...

[ssahani]

Indeed. Updated. Thanks for the review.