[patch] system activation 3
Havoc Pennington
hp at redhat.com
Wed Jun 27 18:09:31 PDT 2007
Hi,
Richard Hughes wrote:
> Sure, no problem. I've attached my latest "working" version, with about
> half of the tests present for the launch helper. The patch is now huge,
> apologies.
No problem. I will go through it quickly now, then let me know when you
feel it is 100% baked and I will go through it one last time.
> INCLUDES=-I$(top_srcdir) $(DBUS_BUS_CFLAGS) \
> + -DDBUS_SYSTEM_CONFIG_FILE=\""$(configdir)/system.conf"\" \
The way this is done for session.conf recently changed; the reason to
change it for session.conf (portability) doesn't really apply to Windows
which has no system bus, but perhaps for symmetry the system bus should
be done the same way.
> +dbus_daemon_launch_helper_test_SOURCES= \
> + $(LAUNCH_HELPER_SOURCES)
> +dbus_daemon_launch_helper_test_CPPFLAGS= \
> + -DACTIVATION_LAUNCHER_TEST
Is the idea to build the launch helper twice with different CPPFLAGS?
Does that work with the dbus build setup? I know automake didn't used to
like it, but I have seen recent versions include the target binary in
the .o/.lo file names in some situations. I guess it probably works or
you would have noticed ;-)
> +/* dbus-spawn-EXIT_CODEs.h Return values for the launch helper which is set
search-and-replace glitch?
> +#ifndef DBUS_ACTIVATION_EXIT_CODES_H
> +#define DBUS_ACTIVATION_EXIT_CODES_H
> +
> +#if !defined (DBUS_INSIDE_DBUS_H) && !defined (DBUS_COMPILATION)
> +#error "Only <dbus/dbus.h> can be included directly, this file may disappear or change contents."
> +#endif
This boilerplate looks like it's copied from dbus/*.h rather than
bus/*.h (include guards BUS_ vs. DBUS_, and no need for the "only dbus.h
directly" warning for bus/*.h since bus/*.h is not installed anyhow.
> +/** Return codes from the launch helper - not public API */
> +#define _DBUS_SPAWN_EXIT_CODE_NO_MEMORY 1
> +#define _DBUS_SPAWN_EXIT_CODE_CONFIG_INVALID 2
> +#define _DBUS_SPAWN_EXIT_CODE_SETUP_FAILED 3
> +#define _DBUS_SPAWN_EXIT_CODE_NAME_INVALID 4
> +#define _DBUS_SPAWN_EXIT_CODE_SERVICE_NOT_FOUND 5
> +#define _DBUS_SPAWN_EXIT_CODE_PERMISSIONS_INVALID 6
> +#define _DBUS_SPAWN_EXIT_CODE_FILE_INVALID 7
> +#define _DBUS_SPAWN_EXIT_CODE_EXEC_FAILED 8
I'd expect to see the BUS_ prefix here instead of _DBUS
> + if (!_dbus_string_init (&filename) ||
> + !_dbus_string_init (&full_path))
> + {
> + BUS_SET_OOM (error);
> + goto failed;
> + }
In "failed" both strings are freed, but potentially only one is
initialized in the above code. OOM-handling unit test would catch this.
> + /* Didn't find desktop file; set error */
> + if (desktop_file == NULL)
> + {
> + dbus_set_error (error, DBUS_ERROR_SERVICE_UNKNOWN,
> + "The name %s was not provided by any .service files",
> + name);
> + }
> +
> +failed:
> + _dbus_string_free (&filename);
> + _dbus_string_free (&full_path);
> + return desktop_file;
> +}
I'd label this "out:" instead of "failed:" since it's also used on
success.</super nitpick>
> + /* get the complete path of the executable */
> + if (!bus_desktop_file_get_string (desktop_file,
> + DBUS_SERVICE_SECTION,
> + DBUS_SERVICE_EXEC,
> + &exec_tmp,
> + &error))
> + {
> + _dbus_warn ("dbus-daemon-activation-helper: could not find 'Exec' in service file!\n");
> + goto error;
> + }
I haven't looked but we strongly rely here on PATH not getting searched
I would think. You might verify that nothing in the desktop file parser
is doing anything of that nature. (Should we be resetting path on
startup of the setuid binary? I think some "how to write a setuid thing"
guides may say to do this, just a faint memory.)
Also for all three desktop_file_get_string in this function it looks
like the DBusError may not be getting freed. (An OOM-handling unit test
would catch this also, it includes leak checks.)
There are utilities for writing an OOM-handling test that are used in
some of the dbus/*.c unit tests, and I think in dispatch.c - called
run_failing_each_malloc or something like that.
What you could do is have a test that puts all or most of main() in the
run_failing_each_malloc, probably this would require splitting most of
main() into a separate function called from both a little "make check"
test that calls the separate function failing each malloc, and from the
helper itself.
> + retval = 255;
255 is a bit of a magic number. (I'm sure as I read on I will see what
the deal with it is, but reading the file sequentially I don't know yet.)
> +#ifdef ACTIVATION_LAUNCHER_TEST
> + /* this is not a security hole. The environment variable is only passed in the
> + * dbus-daemon-lauch-helper-test NON-SETUID launcher */
Just to put oomph behind this comment, you could add a "if we are setuid
then bail out" check if ACTIVATION_LAUNCHER_TEST is defined, perhaps (at
the start of main).
I'm assuming there's a way to say "am I setuid" - does getuid() !=
geteuid() work? I don't know to be honest.
> + dbus_error_free (&error);
> + handle_activation_exit_error (exit_code, &error);
I think dbus_set_error requires an initialized (not freed) error, so
this might break - or maybe dbus_error_free re-initializes the error
come to think of it. Nevermind. But maybe double check.
> + /* system bus uses helper */
> + bus_type = bus_context_get_type (activation->context);
> + if (bus_type != NULL && strcmp (bus_type, "system") == 0)
This is leftovers right? It should be "use helper if config file had a
helper in it"
> + /* find the type of the bus and get the correct direcories */
> + bus_type = bus_config_parser_get_type (parser);
> + if (bus_type != NULL && strcmp (bus_type, "system") == 0)
> + dirs = bus_config_parser_get_system_service_dirs (parser);
> + else
> + dirs = bus_config_parser_get_session_service_dirs (parser);
> +
I think this is in test code? If it isn't, it's not right though, the
bus type should not change behavior.
> + switch (parser->type)
> + {
> + case ELEMENT_SERVICEDIR:
> + {
> + const char *type;
> + char *cpath;
> + DBusList **service_dirs;
> +
> + /* If we are NULL, then we have not set a bus type prior to the
> + * servicedir tag. When testing we need to make sure that type is
> + * _above_ servicedir in the conf file, but since adding servicedirs
> + * for the system bus is probably a bad idea we don't need to document
> + * or test for it. */
> + type = _dbus_string_get_const_data (&parser->bus_type);
> + if (type == NULL)
> + {
> + _dbus_verbose ("no <type> so assuming session");
> + service_dirs = &parser->session_service_dirs;
> + }
> + else
> + {
> + if (strcmp (type, "system") == 0)
> + service_dirs = &parser->system_service_dirs;
> + else
> + service_dirs = &parser->session_service_dirs;
> + }
I'm not sure what's going on here - <servicedir> is to specify a single
random directory, while the
standard_session_servicedirs/standard_system_service_dirs should result
in the list - nothing should key off the bus type - maybe I am just
dehydrated from the heat today ;-)
> +
> + /* copy the sane data into a char array */
> + if (!_dbus_string_copy_data(&content_sane, &cpath))
> + goto nomem;
> +
You can use steal_data here probably as a totally pointless optimization ;-)
> +#ifdef DBUS_BUILD_TESTS
It would be nice to avoid cut-and-pasting the "parse all the sample
config files in test/data" tests between the trivial and full config
parser, ideally some utility function would just be available to both
tests. You could just stick the util function in the -common.c, with
DBUS_BUILD_TESTS around it.
> +#ifdef IS_IN_CHECK_MODE_TODO
> + /* check if helper exists... */
> + if (!_dbus_file_exists (filename_str))
> + {
> + dbus_set_error (error, DBUS_ERROR_FILE_NOT_FOUND, "setuid helper not found");
> + return FALSE;
> + }
> +#endif
presumably this can be handled however we handle servicedir, etc. now (I
think they have to exist).
I'd put the name of the file looked for in the error message, since
dbus_set_error takes a format string, it's an easy way to be nice.
> DBusList**
> -bus_config_parser_get_service_dirs (BusConfigParser *parser)
> +bus_config_parser_get_session_service_dirs (BusConfigParser *parser)
> +{
> + return &parser->session_service_dirs;
> +}
> +
> +DBusList**
> +bus_config_parser_get_system_service_dirs (BusConfigParser *parser)
> {
> - return &parser->service_dirs;
> + return &parser->system_service_dirs;
> }
This doesn't make sense to me (session vs. system service dirs getters)
- there is no way a single daemon can have both session and system. The
daemon just knows about service dirs in general, right? The only session
vs. system difference is in the _defaults_.
> + sitter->errnum = WEXITSTATUS (sitter->status);
Again in the nitpick category, errnum is an odd name for an exit status
variable.
> _dbus_verbose ("recorded child status exited = %d signaled = %d exitstatus = %d termsig = %d\n",
> WIFEXITED (sitter->status), WIFSIGNALED (sitter->status),
> WEXITSTATUS (sitter->status), WTERMSIG (sitter->status));
> @@ -623,6 +624,50 @@ _dbus_babysitter_get_child_exited (DBusBabysitter *sitter)
> }
>
> /**
> + * Gets the exit status of the child. We do this so implimentation specific
> + * detail is not cluttering up dbus, for example the system laucher code.
> + *
> + * @param sitter the babysitter
> + * @param status the returned status code
> + * @returns #FALSE on failure
> + */
> +dbus_bool_t
> +_dbus_babysitter_get_child_exit_status (DBusBabysitter *sitter, int *status)
It looks like all of the failures in this function should really be
assertion failures rather than runtime-reported failures; i.e. they are
warnings that we would fix by modifying the calling code, not by
modifying the runtime environment or "recovering," no?
I'm not quite sure this is true but it looks like it.
> +/**
> + * Returns the standard directories for a system bus to look for service
> + * activation files
> + *
> + * On UNIX this should be the standard xdg freedesktop.org data directories:
> + *
> + * XDG_DATA_DIRS=${XDG_DATA_DIRS-/usr/local/share:/usr/share}
> + *
> + * and
> + *
> + * DBUS_DATADIR
> + *
Perhaps document that on Windows this can just return nothing since
there is no system bus.
Looking good overall, almost there!
Havoc
More information about the dbus
mailing list