fs-watch: Use a properly sized buffer for inotify events #195

hawkw · 2019-02-22T01:22:49Z

The buffer passed to read(2) to read inotify events from the inotify
fd must be long enough to fit events with filenames of the maximum
allowable length. The inotify-rs library previously always constructed
such a buffer, but as of 1fe4e38, we
are required to provide our own buffer. The buffer we were providing was
insufficiently large for some filenames.

See also the man page for inotify:

The behavior when the buffer given to read(2) is too small to return
information about the next event depends on the kernel version: in
kernels before 2.6.21, read(2) returns 0; since kernel 2.6.21,
read(2) fails with the error EINVAL. Specifying a buffer of size
    sizeof(struct inotify_event) + NAME_MAX + 1
will be sufficient to read at least one event.

(http://man7.org/linux/man-pages/man7/inotify.7.html)

This branch updates the fs-watch lib to calculate the necessary buffer
length correctly, and use a properly-sized buffer. I've also added tests
that fail when the buffer is too short.

Fixes linkerd/linkerd2#2331

Signed-off-by: Eliza Weisman eliza@buoyant.io

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

olix0r

lgtm!

olix0r · 2019-02-22T04:18:45Z

lib/fs-watch/src/lib.rs

@@ -812,4 +819,26 @@ mod tests {
    fn inotify_nonexistent_files_dont_file_delete_events() {
        Fixture::new().test_inotify(test_detects_delete_and_recreate)
    }
+
+    const LONG: &str = "loooooooooooooooooooooooooooooooooooooooooooooooongboi";


this might be a good job for quickcheck

Yeah, that's a good point. I'm going to go ahead and merge the bugfix first, though, so it's not blocked on writing a QuickCheck generator for valid filenames.

This picks up the following commits: * 0fe8063 replace with (#2370) (linkerd/linkerd2-proxy#201) * 1ea7559 Minor cleanup in the config tests (linkerd/linkerd2-proxy#188) * d0ef56b Update *ring* to 0.14.6 (linkerd/linkerd2-proxy#197) * c54377f fs-watch: Use a properly sized buffer for inotify events (linkerd/linkerd2-proxy#195) * 23e02a6 Update Router to wait for inner poll_ready before calling inner call * 2de8e9b Update metrics quickcheck to 0.8, and hyper to 0.12.24 * d1bbd4b make: Optionally include debug symbols with builds (linkerd/linkerd2-proxy#193) * 738a541 Fix compilation warnings in fs-watch (linkerd/linkerd2-proxy#192) * 6cc7558 Apply rustfmt (linkerd/linkerd2-proxy#191) Signed-off-by: Ivan Sim <ivan@buoyant.io>

This picks up the following commits: * 0fe8063 replace `Error::cause` with `Error::source` (#2370) (linkerd/linkerd2-proxy#201) * 1ea7559 Minor cleanup in the config tests (linkerd/linkerd2-proxy#188) * d0ef56b Update *ring* to 0.14.6 (linkerd/linkerd2-proxy#197) * c54377f fs-watch: Use a properly sized buffer for inotify events (linkerd/linkerd2-proxy#195) * 23e02a6 Update Router to wait for inner poll_ready before calling inner call * 2de8e9b Update metrics quickcheck to 0.8, and hyper to 0.12.24 * d1bbd4b make: Optionally include debug symbols with builds (linkerd/linkerd2-proxy#193) * 738a541 Fix compilation warnings in fs-watch (linkerd/linkerd2-proxy#192) * 6cc7558 Apply rustfmt (linkerd/linkerd2-proxy#191) Signed-off-by: Ivan Sim <ivan@buoyant.io>

Any HTTP/1.1 requests seen by the proxy will automatically set up to prepare such that if the proxied responses agree to an upgrade, the two connections will converted into a standard TCP proxy duplex. Implementation ----------------- This adds a new type, `transparency::Http11Upgrade`, which is a sort of rendezvous type for triggering HTTP/1.1 upgrades. In the h1 server service, if a request looks like an upgrade (`h1::wants_upgrade`), the request body is decorated with this new `Http11Upgrade` type. It is actually a pair, and so the second half is put into the request extensions, so that the h1 client service may look for it right before serialization. If it finds the half in the extensions, it decorates the *response* body with that half (if it looks like a response upgrade (`h1::is_upgrade`)). The `HttpBody` type now has a `Drop` impl, which will look to see if its been decorated with an `Http11Upgrade` half. If so, it will check for hyper's new `Body::on_upgrade()` future, and insert that into the half. When both `Http11Upgrade` halves are dropped, its internal `Drop` will look to if both halves have supplied an upgrade. If so, the two `OnUpgrade` futures from hyper are joined on, and when they succeed, a `transparency::tcp::duplex()` future is created. This chain is spawned into the default executor. The `drain::Watch` signal is carried along, to ensure upgraded connections still count towards active connections when the proxy wants to shutdown. Closes linkerd#195

hawkw added 2 commits February 21, 2019 17:16

fs-watch: Add test for long filenames

089fcda

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

fs-watch: Use a properly sized buffer for inotify events

f5028b7

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

hawkw added the bug Something isn't working label Feb 22, 2019

hawkw self-assigned this Feb 22, 2019

hawkw requested a review from olix0r February 22, 2019 01:22

hawkw mentioned this pull request Feb 22, 2019

proxy: inotify watch error on Debian stretch linkerd/linkerd2#2331

Closed

olix0r approved these changes Feb 22, 2019

View reviewed changes

olix0r reviewed Feb 22, 2019

View reviewed changes

hawkw merged commit c54377f into master Feb 22, 2019

ihcsim mentioned this pull request Feb 27, 2019

Proxy: bump pinned version to 0fe8063 linkerd/linkerd2#2406

Merged

olix0r deleted the eliza/fix-inotify branch August 17, 2019 01:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fs-watch: Use a properly sized buffer for inotify events #195

fs-watch: Use a properly sized buffer for inotify events #195

hawkw commented Feb 22, 2019 •

edited

Loading

olix0r left a comment

olix0r Feb 22, 2019

hawkw Feb 22, 2019 •

edited

Loading

fs-watch: Use a properly sized buffer for inotify events #195

fs-watch: Use a properly sized buffer for inotify events #195

Conversation

hawkw commented Feb 22, 2019 • edited Loading

olix0r left a comment

Choose a reason for hiding this comment

olix0r Feb 22, 2019

Choose a reason for hiding this comment

hawkw Feb 22, 2019 • edited Loading

Choose a reason for hiding this comment

hawkw commented Feb 22, 2019 •

edited

Loading

hawkw Feb 22, 2019 •

edited

Loading