nih_dir_walk_scan passes incorrect value to file filter

Bug #776532 reported by James Hunt on 2011-05-03
8
This bug affects 1 person
Affects Status Importance Assigned to Milestone
libnih
Undecided
Unassigned
libnih (Ubuntu)
Undecided
Unassigned

Bug Description

nih_watch_new() accepts a generic data pointer which is supposed to be passed to the create/modify/delete handlers *and* the file filter. However, there is a design issue in that the incorrect pointer is passed to the file filter as outlined in the psuedo-code below:

1. When nih_watch_new() is called, it creates a new NihWatch object and sets "watch->data = data".
2. nih_watch_add(watch, ...) is then called.
3. nih_watch_add calls nih_dir_walk(path, filter=watch->filter, visitor=nih_watch_add_visitor, error=NULL, data=watch).
   (Note that "data=watch" here).
4. nih_dir_walk() calls nih_dir_walk_scan(path, filter=watch->filter, data=watch).
5. BUG: nih_dir_walk_scan() calls filter(data=watch).
6. nih_dir_walk() then calls nih_dir_walk_visit(..., filter=watch->filter, visitor=nih_watch_add_filter, error=NULL, data=watch)
7. nih_dir_walk_visit() calls visitor (data, ...) which resolves to:

   nih_watch_add_filter(NihWatch *watch, ...).

Step 5 is incorrect. nih_dir_walk_scan() *should* call:

   filter(data=watch->data)

However, it cannot since nih_dir_walk_scan knows nothing about NihWatches. The design issue comes down to the fact that the data pointer passed to nih_dir_walk() needs to be an NihWatch so it can be passed to nih_watch_add_visitor(), but it needs to be a true generic pointer, as set by the user via the data parameter to nih_watch_new().

One possible solution would be to have *two* generic pointers passed to nih_dir_walk: one for the NihFileVisitor handler and the other for the NihFileFilter handler such that we would have something like:

int
nih_dir_walk (const char *path,
              NihFileFilter filter,
              NihFileVisitor visitor,
              NihFileErrorHandler error,
              void *visitor_data,
              void *filter_data)
{
          :
    filter (filter_data);
          :
    visitor (visitor_data);
          :
}

Related branches

Scott James Remnant (scott) wrote :

(as discussed today)

I see the problem; there are, as you say, two data pointers here; nih_dir_walk_scan() needs its data pointer passed to its visitor function, but is passing an external filter from the watch which needs an entirely different data pointer (from the watch).

The right fix without busting API would be to change step 4

nih_dir_walk() should pass filter=nih_dir_walk_filter, a new static function that expects data=watch

that static function then simply calls

  return watch->filter (watch->data, ...)

passing in the rest of the arguments.

That way data=watch is passed to both nih_dir_walk()s filter and visit, and the special filter translates it to the watch's filter passing in the watch's data

Changed in libnih:
status: New → Triaged
James Hunt (jamesodhunt) wrote :

The fix discussed does not work in the general case (where nih_dir_walk() is called directly) so we may need to break the API here.

Steve Langasek (vorlon) wrote :

I don't see any reason we need to break the API here. Anyone calling nih_dir_walk() directly should already be using values of data, filter, and visitor that are compatible. The only problem here is that nih_watch_add() is calling them in a manner that requires two different types for 'data'. So this is a bug in the caller; if other callers have this same bug, that's a bug in those other callers. This is not something we need (or want) to solve generically in nih_dir_walk() itself.

Steve Langasek (vorlon) wrote :

(and the only other caller of nih_dir_walk() that I find is in upstart - init/conf.c - which is using it correctly, passing a pair of function pointers which both take a ConfSource * as their argument.)

Dimitri John Ledkov (xnox) wrote :

This bug was fixed in the package libnih - 1.0.3-4ubuntu16

---------------
libnih (1.0.3-4ubuntu16) raring; urgency=low

  * debian/{libnih1.postinst,libnih-dbus1.postinst}: Force an upgrade to
    restart Upstart (to pick up new package version) if the running
    instance supports it.
  * Merge of important fixes from lp:~upstart-devel/libnih/nih
    (LP: #776532, LP: #777097, LP: #834813, LP: #1123588).
 -- James Hunt <email address hidden> Thu, 14 Mar 2013 09:14:22 +0000

Changed in libnih (Ubuntu):
status: New → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers