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
=== modified file 'ChangeLog'
--- ChangeLog 2011-06-20 18:47:02 +0000
+++ ChangeLog 2011-06-21 16:26:00 +0000
@@ -1,3 +1,17 @@
12011-06-21 James Hunt <james.hunt@ubuntu.com>
2
3 Special case code to handle inotify event queue overflow (LP:#777093).
4
5 * nih/watch.c: nih_watch_reader(): Handle situation where kernel
6 is generating events faster than can be consumed by simply
7 ignoring them. Failure to do so results in an assertion failure in
8 nih_watch_handle_by_wd() since the inotify_event object will
9 contain an invalid watch descriptor (-1).
10 * nih/tests/test_watch.c:
11 - Added PROC_MAX_INOTIFY define.
12 - test_reader(): Added test to check for inotify event queue overflow
13 handling.
14
12011-06-20 James Hunt <james.hunt@ubuntu.com>152011-06-20 James Hunt <james.hunt@ubuntu.com>
216
3 * nih/watch.c (nih_watch_handle): Handle non-directory watches;17 * nih/watch.c (nih_watch_handle): Handle non-directory watches;
418
=== modified file 'nih/tests/test_watch.c'
--- nih/tests/test_watch.c 2011-06-20 16:09:08 +0000
+++ nih/tests/test_watch.c 2011-06-21 16:26:00 +0000
@@ -39,6 +39,7 @@
39#include <nih/error.h>39#include <nih/error.h>
40#include <nih/logging.h>40#include <nih/logging.h>
4141
42#define PROC_MAX_INOTIFY "/proc/sys/fs/inotify/max_queued_events"
4243
43static int44static int
44my_filter (void *data,45my_filter (void *data,
@@ -1199,6 +1200,74 @@
11991200
1200 nih_free (last_path);1201 nih_free (last_path);
12011202
1203 /* only run this test if /proc mounted and inotify available */
1204 if (access (PROC_MAX_INOTIFY, F_OK) == 0)
1205 {
1206 size_t max_queued_events = 0, i;
1207
1208 TEST_FEATURE ("with inotify event queue overflow");
1209
1210 modify_called = 0;
1211 last_watch = NULL;
1212 last_path = NULL;
1213 last_data = NULL;
1214
1215 /* establish per process inotify event queue size */
1216 fd = fopen (PROC_MAX_INOTIFY, "r");
1217 TEST_NE_P (fd, NULL);
1218 fscanf (fd, "%lu", (long unsigned int *)&max_queued_events);
1219 fclose (fd);
1220
1221 fd = fopen (filename, "w");
1222 fprintf (fd, "foo\n");
1223 fclose (fd);
1224
1225 nfds = 0;
1226 FD_ZERO (&readfds);
1227 FD_ZERO (&writefds);
1228 FD_ZERO (&exceptfds);
1229
1230 nih_io_select_fds (&nfds, &readfds, &writefds, &exceptfds);
1231 select (nfds, &readfds, &writefds, &exceptfds, NULL);
1232 nih_io_handle_fds (&readfds, &writefds, &exceptfds);
1233
1234 TEST_EQ (modify_called, 1);
1235
1236 modify_called = 0;
1237
1238 /* Force an inotify event queue overflow.
1239 *
1240 * Note that we cannot force max_queued_events events and
1241 * _then_ try to generate a few more later since this
1242 * test is timing critical: we want to guarantee that
1243 * userspace is flooded with inotify events such that it
1244 * cannot handle them fast enough.
1245 * */
1246
1247 for (i=0; i < max_queued_events + 100; ++i) {
1248 fd = fopen (filename, "a+");
1249 TEST_NE_P (fd, NULL);
1250 fputc ('\0', fd);
1251 fclose (fd);
1252
1253 nfds = 0;
1254 FD_ZERO (&readfds);
1255 FD_ZERO (&writefds);
1256 FD_ZERO (&exceptfds);
1257
1258 nih_io_select_fds (&nfds, &readfds, &writefds, &exceptfds);
1259 select (nfds, &readfds, &writefds, &exceptfds, NULL);
1260 nih_io_handle_fds (&readfds, &writefds, &exceptfds);
1261 }
1262
1263 /* If we got here, the test has passed since the original
1264 * problem caused an assertion failure in
1265 * nih_watch_handle_by_wd().
1266 */
1267 TEST_GT (modify_called, 0);
1268
1269 nih_free (last_path);
1270 }
12021271
1203 /* Check that we can rename the file, we should get the delete1272 /* Check that we can rename the file, we should get the delete
1204 * handler called followed by the create handler.1273 * handler called followed by the create handler.
12051274
=== modified file 'nih/watch.c'
--- nih/watch.c 2011-06-20 16:09:08 +0000
+++ nih/watch.c 2011-06-21 16:26:00 +0000
@@ -424,6 +424,13 @@
424 if (len < sz)424 if (len < sz)
425 goto finish;425 goto finish;
426426
427 /* Handle situation where kernel is generating events faster
428 * than can be consumed. Inotify will (somewhat ironically)
429 * generate an event to signify this, so ignore it.
430 */
431 if (event->mask & IN_Q_OVERFLOW)
432 goto consume;
433
427 /* Find the handle for this watch */434 /* Find the handle for this watch */
428 handle = nih_watch_handle_by_wd (watch, event->wd);435 handle = nih_watch_handle_by_wd (watch, event->wd);
429 if (handle)436 if (handle)
@@ -437,6 +444,7 @@
437 if (caught_free)444 if (caught_free)
438 return;445 return;
439446
447consume:
440 /* Remove the event from the front of the buffer, and448 /* Remove the event from the front of the buffer, and
441 * decrease our own length counter.449 * decrease our own length counter.
442 */450 */
443451
=== modified file 'po/libnih.pot'
--- po/libnih.pot 2010-12-23 22:04:08 +0000
+++ po/libnih.pot 2011-06-21 16:26:00 +0000
@@ -6,9 +6,9 @@
6#, fuzzy6#, fuzzy
7msgid ""7msgid ""
8msgstr ""8msgstr ""
9"Project-Id-Version: libnih 1.0.3\n"9"Project-Id-Version: libnih 1.0.4\n"
10"Report-Msgid-Bugs-To: libnih-bugs@netsplit.com\n"10"Report-Msgid-Bugs-To: libnih-bugs@netsplit.com\n"
11"POT-Creation-Date: 2010-12-23 21:53+0000\n"11"POT-Creation-Date: 2011-06-21 16:53+0100\n"
12"PO-Revision-Date: YEAR-MO-DA HO:MI+ZONE\n"12"PO-Revision-Date: YEAR-MO-DA HO:MI+ZONE\n"
13"Last-Translator: FULL NAME <EMAIL@ADDRESS>\n"13"Last-Translator: FULL NAME <EMAIL@ADDRESS>\n"
14"Language-Team: LANGUAGE <LL@li.org>\n"14"Language-Team: LANGUAGE <LL@li.org>\n"
@@ -158,7 +158,7 @@
158msgid "Options:\n"158msgid "Options:\n"
159msgstr ""159msgstr ""
160160
161#: nih/watch.c:550161#: nih/watch.c:567
162msgid "Unable to watch directory"162msgid "Unable to watch directory"
163msgstr ""163msgstr ""
164164

Subscribers

People subscribed via source and target branches