dbuenzli
Infinite loop in `OS.Dir.create ~path:true`
Passing an unnormalized path, such as a/b/./c to OS.Dir.create ~path:true when b doesn't exist causes create to loop indefinitely until it blows up. Proof of concept below:
let () =
let problem = Fpath.(v "a/b/./c") in
match Bos.OS.Dir.create ~path:true problem with
| Ok b -> Format.printf "Exists: %B" b
| Error (`Msg err) -> failwith err
Outputs:
$ dune exec -- ./bin/main.exe
Fatal error: exception Failure("directory a/b/./../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../ exists: File name too long")
Calling Fpath.normalize seems to fix this, but it's a bit annoying that it blows up when it's not normalized.
So this is because internally we use Fpath.parent. When the latter hits a relative segment it goes on to use ../ to move up (see these examples).
But when we use these paths to check for directory existence via stat the cwd is not reported as existing because stat reports ENOENT on the path a/b/./../.. so we continue to go up.
Still not exactly sure what's the best fix here. Perhaps the simplest is to simply normalize ourselves before running the logic.
Fun fact: while investigating this I discovered you can use mkdir -p to create an arbitrary number of directories at the same level for example with a/b/../c/../d :-)
Still not exactly sure what's the best fix here. Perhaps the simplest is to simply normalize ourselves before running the logic.
I agree. I think using normalize is a good solution. Alternatively, we could use Fpath.split_base instead of Fpath.parent, as it seems like a safer way to traverse through a path.
...
# Fpath.parent (p "a/b/.");;
- : Fpath.t = a/b/./../
# Fpath.split_base (p "a/b/.");;
- : Fpath.t * Fpath.t = (a/b/, .)
But I don't fully understand the semantics of split_base enough to say that it would simply be a matter of swapping one function for another.
Fun fact: while investigating this I discovered you can use
mkdir -pto create an arbitrary number of directories at the same level for example witha/b/../c/../d:-)
Hehe, looks like a cool and maintainable way to create directories in shell scripts! I'll definitely be using this technique to confuse my future self :sweat_smile:
Using split_base is possible but entails changing the logic quite a bit (you need e.g. to handle .. in the logic).
I'm starting to think that part of the problem lies with Fpath.parent. I took a wrong shortcut by starting to append ../ as soon as it sees it a relative path (it even admits it's dumb). It makes the correctness of the function easy to assert but makes the function an infinite loop footgun.
I think that the only time where appending .. is warranted is when you ask for the parent of . or a sequence of .. since there's nothing else you can do.
For example I don't like these results on the root:
# Fpath.(parent @@ v "/.");;
- : Fpath.t = /./../
# Fpath.(parent @@ v "/..");;
- : Fpath.t = /../../
# Fpath.(parent @@ v "/../..");;
- : Fpath.t = /../../../
They are correct but these should all be simply /. Somehow you expect that by calling Fpath.parent on any absolute path you end up at / at some point and then stay there forever:
# Fpath.(parent @@ v "/");;
- : Fpath.t = /
Basically I think that Fpath.parent should guarantee the following:
- Recursively calling
Fpath.parenton an absolute path always returns/at some point. - Recursively calling
Fpath.parenton a relative path always eventually leads to a path made only of..segments.
I don't think that fixing Fpath.parent is enough though, I suspect that relying on stat to stop might still generate infinite sequences of .., for example if you try to create ../../a and ../.. does not exist (perhaps if your cwd was deleted ?) you'd still parent and parent infinitely.
Just thinking out loud.
Over here I have a more recent, alternate, implementation of "mkdir -p" that notably does not rely on stat but only the error codes of calls to Unix.mkdir which I find more elegant.
Amusingly while it is using a slightly better behaved version of Fpath.parent[^1]. It fails on the same example you provide, the reason being that it goes up through the following paths:
a/b/./c
a/b/./
a/
and then down to create them with Unix.mkdir. That works for a/ but fails with ENOENT on a/b/./. So if we want to use Fpath.parent we should really make sure to get rid of these stray .. The only time Fpath.parent should return ./ is on paths with a single relative segment. That is we should have the sequence:
a/b/./c
a/b/
a/
.
..
../..
…
[^1]: It does however fail with an infinite loop if you have a stray .., e.g on a/b/../c for the same reasons, as it escapes the problem of dealing with .. paths by appending ../