manifest --resolve normalizes west-commands: "sometimes"

[marc-hb]

Depending on various conditions, manifest-resolve sometimes outputs special characters exactly as entered like this:

    west-commands:
    - mf_subdir/scripts/\winsubA\\\west-commands-from-subdir-windows.yml

but in other situations it normalizes paths like this instead:

    west-commands:
    - mf_subdir/scripts/winsubA/west-commands-from-subdir-windows.yml

At first I thought it would behave one way on Windows and the other way on Unix. It does, but it is unfortunately worse. I just discovered while extending test coverage (#962) for the very recent ca6bb64 fix for #725 that the two different behaviors can be observed on Windows alone. I think that's really not OK.

This was already discussed in:

• fix: Allow west commands to be imported from a project subdirectory if manifest is located in subdirectory #920 (comment)
• fix: Allow west commands to be imported from a project subdirectory if manifest is located in subdirectory #920 (comment)

... and in probably a few other places in #920 but I didn't think it would be inconsistent even on the same OS.

PS: in a private chat @pdgendt also suspects that mf_subdir might be "silently" added. Should it be?

cc:

• tests: manifest: Add parse/yaml roundtrip test for nested west-commands #965
• manifest: model west-commands as per-entry records #966

[marc-hb]

From @nmunnich in #920 (comment)

Regarding our inconsistency, you are in the end correct that it does seem to affect this after all. My understanding was that we have:

• the west file itself
• the output of west manifest --resolve, i.e. what west sees
• the ability of west to find and run the files found by the manifest

The previous discussion appeared when we were discussing west turning 1 into 2, where both Linux and Windows seem to behave perfectly fine regardless of what type of paths we choose, and hence I thought we were safe here. Going from 2 to 3, we see an issue appearing in Linux where it cannot interpret the windows style paths in the resolved manifest correctly.

So the ability of find and run files is OK, which is good. On other hand I found that your #920 seems to introduce inconsistent .as_posix() normalization... on Windows alone. Can you please look into this?

I've added a windows-only test in the last commit which shows this. If the test is run on Linux it fails, on windows it passes.

I've ported this test to Unix in #962, please review.

[marc-hb]

Setting priority "high" until we have at the very least a clear decision on what the behavior SHOULD be in both the short and long terms. A bug is an unexpected behavior. It's not possible to assess the severity of a bug when we don't even know what the expected behavior is!

I could be wrong but I suspect the #920 fix for #725 made this worse = a regression somewhat.

[nmunnich]

Did some digging. #920 did indeed introduce this inconsistency, because of the use of pathlib and as_posix on L2729. If this is changed to a form of plain string manipulation

            str(mfst_dir) + "/" + str(cmd) if str(mfst_dir) != "." else str(cmd) for cmd in submanifest._ctx.manifest_west_commands

I can get all tests to pass without the inconsistency.

I believe that the root cause of everything is that prior to #920, we did not perform any sort of validation or parsing on west-commands prior to resolving the manifest. The value was simply taken as a plain string, and then the handling of turning it into a filepath is performed when trying to execute a command. #920 introduced some parsing by converting to a pathlib object, but only when resolving from a project under certain circumstances and not when resolving from self.

I would strongly oppose taking the route of plain string manipulation. Not only does it feel jank, it is also inconsistent with the behavior of import where we do parse strange characters - there we (as far as I recall) always convert to posix style paths for the resolved manifest. I would suggest that the correct thing to do would be to always parse and convert west-commands (and any other paths) to posix style paths. This would be easier to reason about, consistent with import, make Unix and Windows
behave the same, and potentially allow for some simplification somewhere.

[marc-hb]

Thanks @nmunnich for the analysis and direct pointers, much appreciated!

I would strongly oppose taking the route of plain string manipulation. [...] I would suggest that the correct thing to do would be to always parse and convert west-commands (and any other paths) to posix style paths.

That would be my inclination as well, not the least because:

• forward slashes work everywhere
• backslashes are also used for escape sequences - a never ending source of pain ("raw" strings etc.)

Unfortunately, this goes somewhat against the manifest --resolve "round-trip" objective, see @pdgendt's #965 as an example.

BTW "forward slashes work everywhere" may sound like "POSIX paths work everywhere". Absolutely not! Even if we decide to give up perfect round-trips and use .as_posix() to normalize everything, then we will still have the issue that pretty much any garbage character is valid in POSIX paths.
https://dwheeler.com/essays/fixing-unix-linux-filenames.html

So we've just "normalized" with .as_posix() to something that is actually different or even does not work at all on Windows! Just look at the backslashes I added in #928 and #963. Is that OK/good enough? We just tell users "Don't do that"? I don't know...

Last but not least, this GH issue is currently focused on west-commands because that has just been the messenger but we should really make sure all filepaths are treated the same eventually, not just west-commands.

To finish on a less negative note: I think the reason these problems have not surfaced earlier is just because most people use very simple and portable paths. So there's no urgency, not after all these years. However, we really need to have a well-defined and formalized plan to clean up this mess. Who knows, we could even have some security issues lurking there because "all input is evil". Just imagine a manifest with really "bad" characters... This inconsistency between two west-commands: field in the same project on the same OS just happens to be a good "alarm bell".

Less negative... fail? :-D

[marc-hb]

Maybe we can:

1. Parse with pathlib everywhere and use that everywhere we need.
2. Remember the original string in a field in parallel and dump that back in the output manifest --resolve. As a field in a new class like Improvement: WestCommandsType should be a class #959.

Now there is a bit of a twist because manifest --resolve reads multiple .yml files and outputs only one. But maybe we can just use forward slashes to concatenate the original strings 2. and perform a quick check that the result is equivalent to pathlib concatenation of 1. If not equivalent, then abort with an error message like "Don't use too weird characters in paths".

[nmunnich]

I would argue that trying to keep perfect OS compatibility between paths is not the role of west, and is out of scope of this discussion. Such a topic belongs to pathlib or similar libraries that deal with paths directly. If a user decides to write a manifest which has special characters in filepaths that are only accepted on Posix-style systems, or decides to use filepaths which have strange side effects like detailed in the linked blog post, then that is on them I would say. Making a note of that is a documentation detail.

What west can do is a simple "best effort" around resolving paths in the same way between operating systems. How functional the paths are afterwards can depend on the strangeness of the paths and the unique traits of the operating system. Different filepath separators are one thing, the strange edge cases are another. I really would strongly encourage to simply state that the manifest resolves all filepaths to posix-style paths and be done with it.

Regarding security, again, not the role of west imo.

[marc-hb]

Different filepath separators are one thing, the strange edge cases are another.

I wish.

I really would strongly encourage to simply state that the manifest resolves all filepaths to posix-style paths and be done with it.

So what happens when a "naive" Windows developer uses backslashes in its manifest and shares it with Unix teammates thanks to git? As you just wrote, backslashes are not "strange". So, should west running on Unix ignore/re-invent Pathlib and interpret backslashes as forward slashes? If yes, any other Windows character to special-case?

Regarding security, again, not the role of west imo.

I wasn't referring to a "role" but to an attack surface. So far, we are guaranteeing that west update and other west commands do not run any code (unlike Zephyr's extension west build for instance). Each input and each parsing is an attack surface.