[patch] system activation 3

[Richard Hughes]

On Wed, 2007-06-27 at 21:09 -0400, Havoc Pennington wrote:
> 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.

Cool, thanks.

> >  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.

I don't follow - we need the define for the launch helper.

>  > +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? 

Yes.

> 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 ;-)

It seems to work, although I admit it's not ideal. Ideas welcome, but it
has to be a define and not a env variable for obvious reasons.

> > +/* dbus-spawn-EXIT_CODEs.h  Return values for the launch helper which is set
> 
> search-and-replace glitch?

Yup.

> > +#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.

Ahh, partially your fault, as you made me move it from there... :-)

> > +/** 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

Done.

> > +  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.

Gotcha, I'll add that.

> > +  /* 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>

Done.

> > +  /* 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?

PATH is not searched, that's why we hardcode DBUS_SYSTEM_CONFIG_FILE.

>  I think some "how to write a setuid thing" 
> guides may say to do this, just a faint memory.)

We already do _dbus_clearenv, surely this would nuke PATH?

> 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.)

Indeed I do. I'll add the oom check here also.

> 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.

Sure, I've refactored this file pretty heavily (attached, would
appreciate new preliminary review) and added the OOM check. I'm still
fixing some OOM failures (arrh!!) but I'll be finished soon. I'm not
sure how to set TEST_OOM_CHECKS either.

> 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.

Done.

> 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.)

Bahh, errors are handled better now.

> > +#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).

Done.

> I'm assuming there's a way to say "am I setuid" - does getuid() != 
> geteuid() work? I don't know to be honest.

Yes, that would work.

> > +              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.

It does re-initializes it.

> > +  /* 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"

Yup, see below.

> > +  /* 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.

Agree, see below.

> > +  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 ;-)

No, I'm on crack. I've removed the two lists and we now have one as
before.

> > +#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.

Hmm, it's difficult as BusConfigParser is a different defined symbol in
each file. I'll see what I can do.

> > +#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).

Sure.

> 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.

I've fixed up the error handling.

> >  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_.

Agree, see above.

> > +                    sitter->errnum = WEXITSTATUS (sitter->status);
> 
> Again in the nitpick category, errnum is an odd name for an exit status 
> variable.

Hmm, I reused it to avoid refactoring large chunks of code. I'll see
what I can do.

> >                      _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?

Fixed.

> I'm not quite sure this is true but it looks like it.

Nah, you're on the money ;-)

> > +/**
> > + * 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.

Done.

> Looking good overall, almost there!

Cheers!

Richard.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: activation-helper.c
Type: text/x-csrc
Size: 17364 bytes
Desc: not available
Url : http://lists.freedesktop.org/archives/dbus/attachments/20070628/a2658979/attachment-0001.c