TOCTOU bug when adding a watcher

[matklad]

See user visible bug report here: lomirus/live-server#73

I believe the following code is incorrect:

notify/notify/src/inotify.rs

Lines 400 to 407 in 2511ebc

	for entry in WalkDir::new(path)
	.follow_links(true)
	.into_iter()
	.filter_map(filter_dir)
	{
	self.add_single_watch(entry.path().to_path_buf(), is_recursive, watch_self)?;
	watch_self = false;
	}

Here, we use walkdir to list paths, and then use inotify to watch the path. The problem is that between the moment that walkdir returned us a PathBut, and us giving this path to inotify to watch, the path can get deleted. This results in inotify returning a FileNotFound IO error, and notify bailing out with it all the way up to RecommendedWatcher::watch.

The expected behavior is for notify to either report removal for this path, or not report anything at all, but clearly not to crash.

EDIT: to clarify, the path in question is not the root path which we ask to watch, but some subdirectory of it

[matklad]

https://mail.gnome.org/archives/dashboard-hackers/2004-October/msg00022.html details a solution to this problem.

[sunshowers]

Hi! Just wanted to chime in as someone who did a lot of resiliency work on watchman around a decade ago.

It took many months of work to get watchman to the state where it could reliably be used to obtain information within source control. There are a very large number of possible TOCTTOU events -- this is a distributed system with really chaotic event sources, such as the user doing large source control checkouts or updates :)

Note that watchman solves a much harder problem -- it stats and caches file metadata and hashes, which adds many new and exciting sources of TOCTTOU issues. I don't think notify solves that problem at the moment, but if you decide to do so in the future and are at all interested in being correct, prepare for months of grueling work.

BTW, I would definitely put some effort towards writing a property-based tester or fuzzer -- something that does many FS operations at random, against a real filesystem and in-memory data structures, and ensures invariants are upheld.

[matklad]

Not that in this specific issues what we want is not “report a sequence of events that correctly reconstructs the state of the file system”, but a much weaker property of “do not crash user application”.

[sunshowers]

Not that in this specific issues what we want is not “report a sequence of events that correctly reconstructs the state of the file system”, but a much weaker property of “do not crash user application”.

Sure — it's just that there are many possible TOCTTOUs just in this process:

• file deleted, as you pointed out
• file replaced with directory
• directory replaced with file
• parent directory moved in the middle
• parent directory deleted in the middle (which has different behavior from move because the inode goes away)

The last two are why it's important to use the "at" functions that specify files relative to an fd that corresponds to the directory (I don't think this code does that)

An implementation that doesn't crash and doesn't quickly corrupt its internal data structures would have to handle all of these.