New SE-DBUS patch

Havoc Pennington hp at redhat.com
Fri Jun 25 09:19:22 PDT 2004


Hi,

Thanks for the patch, and for posting iterations as you go.
Extending Colin's comments a bit, this list looks long but it's mostly
minor stuff.

- the #ifdef (and also the SELinux header file inclusion) should
  be confined to dbus-sysdeps.c basically, or at most to security.[hc]; 
  basically you want to have your SELinux routines exported in a way 
  that doesn't use the SELinux headers, and if SELinux is either not
  present at runtime or not present at compile time these routines 
  become no-ops (presumably they just always return "action is allowed")

- I wouldn't worry about get_unix_fd() multiple fds until we have a 
  DBusTransport with multiple, at that time we can add a new 
  function

- "FIXME Actually check errors like we should" should be fixed 
  probably

- e->d.policy.security_context = dbus_new0 (DBusString, 1);
  is a bit different from how D-BUS is normally done, usually 
  either DBusString is directly in the struct (rather than DBusString*) 
  or char* is used (char* is used in DBusPolicy right now)

- as an example of how to contain SELinux stuff to dbus-sysdeps.c, 
  you have the "Initialize user space AVC" code, that might 
  go in a routine called "_dbus_init_selinux()" that did nothing
  when compiled without SELinux.

- I might call the security.[hc] files selinux.[hc] or something,
  since there are a lot of other security-related files in dbus
  such as policy.[hc]

- I'd either put all the selinux system API access in security.[hc]
  or sysdeps, not in both

- It looks like some functions may be exported from security.[hc] 
  that are only used in security.[hc]? Ideally the only exports
  are the check() routines and an init() routine

- debugging routines such as print_service_sid_table should 
  be #ifdef DBUS_ENABLE_VERBOSE_MODE

- I'd default enable_selinux to auto rather than no

- the other config.h defines are HAVE_ rather than WITH_
  (is WITH_SELINUX significant to the system headers
  or something?)

- DBUS_BUS_CFLAGS="$XML_CFLAGS -DWITH_SELINUX"
  shouldn't be required if you define WITH_SELINUX in config.h

- you should check for the 
  selinux lib, look at the AC_CHECK_LIB(expat for an example

- the _dbus_warn() in dbus_connection_get_unix_fd() should 
  instead be e.g. "_dbus_return_val_if_fail (connection != NULL, FALSE)"

- dbus_bool_t result = FALSE; should be two lines, one declaration
  and one assignment

Thanks,
Havoc





More information about the dbus mailing list