client.wait with a continuous request (loop with EAGAIN) leaks memory and watches

[hannesm]

//cc @palainp

The story unfolds in mirage/qubes-mirage-firewall#155, the code in question is:

let vifs ~handle domid =
        let path = Printf.sprintf "backend/vif/%d" domid in
        directory ~handle path >>=
        Lwt_list.filter_map_p (fun device_id ->
            match String.to_int device_id with
            | None -> Log.err (fun f -> f "Invalid device ID %S for domid %d" device_id domid); Lwt.return_none
            | Some device_id ->
              let vif = { ClientVif.domid; device_id } in
              Lwt.try_bind
                (fun () -> Xen_os.Xs.read handle (Printf.sprintf "%s/%d/ip" path device_id))
                ...

let watch_clients fn =
  Xen_os.Xs.make () >>= fun xs ->
  let backend_vifs = "backend/vif" in
  Log.info (fun f -> f "Watching %s" backend_vifs);
  Xen_os.Xs.wait xs (fun handle ->
  directory ~handle backend_vifs >>= fun items ->
   Lwt_list.map_p (vifs handle) items >>= fun items ->
   fn (List.concat items |> VifMap.of_list);
   (* Wait for further updates *)
   Lwt.fail Xs_protocol.Eagain)

So, we use wait to loop forever (we want to dynamically react on changes (add/remove) to new backend virtual interfaces) within watch_clients. What is done once an event happened is the vif directory and it's IP is read (see vifs). With this code, in wait these paths are added to the watch set (in client_lwt/xs_client_lwt.ml), and there's no way in Xs_handle (core/xs_handle.ml) that accessed_paths is ever shrinking, there's only functionality to extend the set of paths.

This means (a) the string set will always grow (leaking memory) and (b) the set of watchers will always increase (leaking watchers, and there's only 128 of them allowed AFAICT). This is unlikely an issue for most applications that wait for a value being changed and then be done with it, but the firewall tracks changes over the entire lifetime.

While several approaches to "fix xenstore" failed, the approach that looks promising is to do the read with a separate client (see https://github.com/mirage/qubes-mirage-firewall/pull/160/files?w=1), but we're curious whether this affects other xenstore users, and/or whether there is a plan to support such a usecase?

Best and thanks for your hard work on xenstore!