Merge lp:~jamesodhunt/libnih/libnih-inotify-overflow-fix-for-777093 into lp:libnih

Proposed by James Hunt on 2011-06-21
Status: Needs review
Proposed branch: lp:~jamesodhunt/libnih/libnih-inotify-overflow-fix-for-777093
Merge into: lp:libnih
Diff against target: 158 lines (+94/-3)
4 files modified
ChangeLog (+14/-0)
nih/tests/test_watch.c (+69/-0)
nih/watch.c (+8/-0)
po/libnih.pot (+3/-3)
To merge this branch: bzr merge lp:~jamesodhunt/libnih/libnih-inotify-overflow-fix-for-777093
Reviewer Review Type Date Requested Status
Scott James Remnant 2011-06-21 Needs Fixing on 2011-06-21
Review via email: mp+65372@code.launchpad.net

Description of the change

  Special case code to handle inotify event queue overflow (LP:#777093).

  * nih/watch.c: nih_watch_reader(): Handle situation where kernel
    is generating events faster than can be consumed by simply
    ignoring them. Failure to do so results in an assertion failure in
    nih_watch_handle_by_wd() since the inotify_event object will
    contain an invalid watch descriptor (-1).
  * nih/tests/test_watch.c:
    - Added PROC_MAX_INOTIFY define.
    - test_reader(): Added test to check for inotify event queue overflow
      handling.

To post a comment you must log in.
Scott James Remnant (scott) wrote :

NAK.

This code is still not "handling" the problem, it's just ignoring it.

Take Upstart for example; it uses inotify to watch its configuration directory. A queue overflow could mean someone just untarred a giant /etc/init config set into the directory, ignoring overflow means that Upstart will miss some files.

Upstart will probably want to respond to queue overflow by walking the directory manually to see what it missed.

review: Needs Fixing
Frank Ch. Eigler (fche) wrote :

> NAK.
> This code is still not "handling" the problem, it's just ignoring it.
> [...]

It is still an improvement over crashing.

Unmerged revisions

1050. By James Hunt on 2011-06-21

Special case code to handle inotify event queue overflow (LP:#777093).

* nih/watch.c: nih_watch_reader(): Handle situation where kernel
  is generating events faster than can be consumed by simply
  ignoring them. Failure to do so results in an assertion failure in
  nih_watch_handle_by_wd() since the inotify_event object will
  contain an invalid watch descriptor (-1).
* nih/tests/test_watch.c:
  - Added PROC_MAX_INOTIFY define.
  - test_reader(): Added test to check for inotify event queue overflow
    handling.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'ChangeLog'
2--- ChangeLog 2011-06-20 18:47:02 +0000
3+++ ChangeLog 2011-06-21 16:26:00 +0000
4@@ -1,3 +1,17 @@
5+2011-06-21 James Hunt <james.hunt@ubuntu.com>
6+
7+ Special case code to handle inotify event queue overflow (LP:#777093).
8+
9+ * nih/watch.c: nih_watch_reader(): Handle situation where kernel
10+ is generating events faster than can be consumed by simply
11+ ignoring them. Failure to do so results in an assertion failure in
12+ nih_watch_handle_by_wd() since the inotify_event object will
13+ contain an invalid watch descriptor (-1).
14+ * nih/tests/test_watch.c:
15+ - Added PROC_MAX_INOTIFY define.
16+ - test_reader(): Added test to check for inotify event queue overflow
17+ handling.
18+
19 2011-06-20 James Hunt <james.hunt@ubuntu.com>
20
21 * nih/watch.c (nih_watch_handle): Handle non-directory watches;
22
23=== modified file 'nih/tests/test_watch.c'
24--- nih/tests/test_watch.c 2011-06-20 16:09:08 +0000
25+++ nih/tests/test_watch.c 2011-06-21 16:26:00 +0000
26@@ -39,6 +39,7 @@
27 #include <nih/error.h>
28 #include <nih/logging.h>
29
30+#define PROC_MAX_INOTIFY "/proc/sys/fs/inotify/max_queued_events"
31
32 static int
33 my_filter (void *data,
34@@ -1199,6 +1200,74 @@
35
36 nih_free (last_path);
37
38+ /* only run this test if /proc mounted and inotify available */
39+ if (access (PROC_MAX_INOTIFY, F_OK) == 0)
40+ {
41+ size_t max_queued_events = 0, i;
42+
43+ TEST_FEATURE ("with inotify event queue overflow");
44+
45+ modify_called = 0;
46+ last_watch = NULL;
47+ last_path = NULL;
48+ last_data = NULL;
49+
50+ /* establish per process inotify event queue size */
51+ fd = fopen (PROC_MAX_INOTIFY, "r");
52+ TEST_NE_P (fd, NULL);
53+ fscanf (fd, "%lu", (long unsigned int *)&max_queued_events);
54+ fclose (fd);
55+
56+ fd = fopen (filename, "w");
57+ fprintf (fd, "foo\n");
58+ fclose (fd);
59+
60+ nfds = 0;
61+ FD_ZERO (&readfds);
62+ FD_ZERO (&writefds);
63+ FD_ZERO (&exceptfds);
64+
65+ nih_io_select_fds (&nfds, &readfds, &writefds, &exceptfds);
66+ select (nfds, &readfds, &writefds, &exceptfds, NULL);
67+ nih_io_handle_fds (&readfds, &writefds, &exceptfds);
68+
69+ TEST_EQ (modify_called, 1);
70+
71+ modify_called = 0;
72+
73+ /* Force an inotify event queue overflow.
74+ *
75+ * Note that we cannot force max_queued_events events and
76+ * _then_ try to generate a few more later since this
77+ * test is timing critical: we want to guarantee that
78+ * userspace is flooded with inotify events such that it
79+ * cannot handle them fast enough.
80+ * */
81+
82+ for (i=0; i < max_queued_events + 100; ++i) {
83+ fd = fopen (filename, "a+");
84+ TEST_NE_P (fd, NULL);
85+ fputc ('\0', fd);
86+ fclose (fd);
87+
88+ nfds = 0;
89+ FD_ZERO (&readfds);
90+ FD_ZERO (&writefds);
91+ FD_ZERO (&exceptfds);
92+
93+ nih_io_select_fds (&nfds, &readfds, &writefds, &exceptfds);
94+ select (nfds, &readfds, &writefds, &exceptfds, NULL);
95+ nih_io_handle_fds (&readfds, &writefds, &exceptfds);
96+ }
97+
98+ /* If we got here, the test has passed since the original
99+ * problem caused an assertion failure in
100+ * nih_watch_handle_by_wd().
101+ */
102+ TEST_GT (modify_called, 0);
103+
104+ nih_free (last_path);
105+ }
106
107 /* Check that we can rename the file, we should get the delete
108 * handler called followed by the create handler.
109
110=== modified file 'nih/watch.c'
111--- nih/watch.c 2011-06-20 16:09:08 +0000
112+++ nih/watch.c 2011-06-21 16:26:00 +0000
113@@ -424,6 +424,13 @@
114 if (len < sz)
115 goto finish;
116
117+ /* Handle situation where kernel is generating events faster
118+ * than can be consumed. Inotify will (somewhat ironically)
119+ * generate an event to signify this, so ignore it.
120+ */
121+ if (event->mask & IN_Q_OVERFLOW)
122+ goto consume;
123+
124 /* Find the handle for this watch */
125 handle = nih_watch_handle_by_wd (watch, event->wd);
126 if (handle)
127@@ -437,6 +444,7 @@
128 if (caught_free)
129 return;
130
131+consume:
132 /* Remove the event from the front of the buffer, and
133 * decrease our own length counter.
134 */
135
136=== modified file 'po/libnih.pot'
137--- po/libnih.pot 2010-12-23 22:04:08 +0000
138+++ po/libnih.pot 2011-06-21 16:26:00 +0000
139@@ -6,9 +6,9 @@
140 #, fuzzy
141 msgid ""
142 msgstr ""
143-"Project-Id-Version: libnih 1.0.3\n"
144+"Project-Id-Version: libnih 1.0.4\n"
145 "Report-Msgid-Bugs-To: libnih-bugs@netsplit.com\n"
146-"POT-Creation-Date: 2010-12-23 21:53+0000\n"
147+"POT-Creation-Date: 2011-06-21 16:53+0100\n"
148 "PO-Revision-Date: YEAR-MO-DA HO:MI+ZONE\n"
149 "Last-Translator: FULL NAME <EMAIL@ADDRESS>\n"
150 "Language-Team: LANGUAGE <LL@li.org>\n"
151@@ -158,7 +158,7 @@
152 msgid "Options:\n"
153 msgstr ""
154
155-#: nih/watch.c:550
156+#: nih/watch.c:567
157 msgid "Unable to watch directory"
158 msgstr ""
159

Subscribers

People subscribed via source and target branches