Created attachment 54653 [details] [review] use install root as base for relative pathes Starting dbus service executables on windows are processed with CreateProcess. On windows installations it is required to use relative pathes for service executables because install pathes may change. According to http://msdn.microsoft.com/en-us/library/windows/desktop/ms682425%28v=vs.85%29.aspx when using relatives pathes, the base path is determined from 1. The directory from which the application loaded 2. The current directory for the parent process. .. The appended patch fixes this issue.
Please get some review from *someone* before merging this. You seem to have merged it unreviewed. At the moment, the D-Bus Specification has no mention of what a relative Exec= would mean, or whether it's even meaningful. Please define and document what semantics we want it to have, and whether they're Windows-specific or meant to be cross-platform.
Comment on attachment 54653 [details] [review] use install root as base for relative pathes Review of attachment 54653 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-spawn-win.c @@ +569,5 @@ > + char *p; > + if (!_dbus_get_install_root (install_root, sizeof(install_root))) > + return INVALID_HANDLE_VALUE; > + > + strcat(install_root,name); This is how buffer overflows happen. I'm going to revert this. We have DBusString for safe string manipulation - please use it.
(In reply to comment #2) > Comment on attachment 54653 [details] [review] [review] > use install root as base for relative pathes > > Review of attachment 54653 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: dbus/dbus-spawn-win.c > @@ +569,5 @@ > > + char *p; > > + if (!_dbus_get_install_root (install_root, sizeof(install_root))) > > + return INVALID_HANDLE_VALUE; > > + > > + strcat(install_root,name); > > This is how buffer overflows happen. I'm going to revert this. no, you probably have overseen seen the following definition + char install_root[2*MAX_PATH]; it uses 2*MAX_PATH pathes on windows are not allowed to be longer then MAX_PATH
(In reply to comment #1) > At the moment, the D-Bus Specification has no mention of what a relative Exec= > would mean, or whether it's even meaningful. > Please define and document what > semantics we want it to have, In raw form: Message Bus Starting Services ... Figure 9. Example service description file # Sample service description file [D-BUS Service] Names=org.freedesktop.ConfigurationDatabase;org.gnome.GConf; Exec=/usr/libexec/gconfd-2 When an application asks to start a service by name, the bus daemon tries to find a service that will own that name. It then tries to spawn the executable associated with it. If this fails, it will report an error <new> Executable names may be absolute or relative. Relative pathes on windows are based on the dbus installation root. The dbus installation root is the grandparent directory of the dbus daemon executable if it is located in a bin subdirectory, otherwise the parent directory. Relative pathes on unix are based on the current working directory of dbus daemon [what is the value here on unix os - it the recent implementation usefull ?] Relative executable pathes in service files are useful on operating systems or environments where the path dbus daemon is running from differs from the path dbus is build for. Examples are the Windows operating system or movable disc installations. </new> > and whether they're Windows-specific yes, this is ths scope of the related patch. > or meant to be cross-platform. no, would be a different topic.
(In reply to comment #4) > or movable disc > installations. better portable installations.
Created attachment 56813 [details] [review] added relative path in service files doc to dbus spec
> > or meant to be cross-platform. > > no, would be a different topic. I documentated the recent behavior on unix like os as if had found at http://linux.die.net/man/2/execve and especially for path handling at http://linux.die.net/man/7/path_resolution Please correct if this is not the truth or need adjustments. Supporting portable installations on unix like os may require an additional (chdir) patch.
(In reply to comment #3) > it uses 2*MAX_PATH > > pathes on windows are not allowed to be longer then MAX_PATH This is far from clear. Please just use a DBusString, like the coding style in HACKING says. (I'm very tempted to ban strcat(), strcpy() etc. from our codebase entirely.)
Comment on attachment 56813 [details] [review] added relative path in service files doc to dbus spec Review of attachment 56813 [details] [review]: ----------------------------------------------------------------- ::: doc/dbus-specification.xml @@ +4216,4 @@ > choose one?] > </para> > <para> > + Executable names may be absolute or relative. Relative pathes on Windows are "paths" @@ +4216,5 @@ > choose one?] > </para> > <para> > + Executable names may be absolute or relative. Relative pathes on Windows are > + based on the dbus installation root. The dbus installation root is the "D-Bus" @@ +4218,5 @@ > <para> > + Executable names may be absolute or relative. Relative pathes on Windows are > + based on the dbus installation root. The dbus installation root is the > + grandparent directory of the dbus daemon executable if it is located in a bin/ > + subdirectory, otherwise the parent directory. This could really do with an example. I'd guess that what you're trying to achieve is: C:\MyApp\bin\dbus-daemon.exe -> search relative to C:\MyApp C:\MyApp\dbus-daemon.exe -> also search relative to C:\MyApp but I'd have phrased that as more like: Relative paths on Windows are interpreted as relative to the D-Bus installation root, which is found by looking up the folder containing the dbus-daemon executable. If that folder is named "bin", its parent folder is the installation root; otherwise, the folder containing dbus-daemon.exe is the installation root. (Without the context of knowing (for instance) how g_get_system_data_dirs() is documented, I'd have interpreted "grandparent" as going from C:\MyApp\bin\dbus-daemon.exe to C:\.) @@ +4222,5 @@ > + subdirectory, otherwise the parent directory. > + </para> > + <para> > + Relative pathes on unix are based on the current working directory of the dbus > + daemon executable. Whether this is what currently happens or not, I don't think this is useful. On Unix, the system dbus-daemon's current working directory is typically /, and the session dbus-daemon's current working directory is typically the user's home directory. @@ +4227,5 @@ > + </para> > + <para> > + Relative executable pathes in service files are useful on operating systems or > + environments where the path dbus daemon is running from differs from the path > + dbus is build for. Examples are the Windows operating system or portable installations. What would you typically write in your .service file? Presumably "Exec=bin/my-service.exe" for a Unix-style installation using bin/ lib/ share/ and so on, and "Exec=my-service.exe" for an installation with everything in the same folder? It seems strange that you have to say "bin/" in the Unix-style case, but not in the everything-in-the-same-folder case, when in both cases the executable is next to dbus-daemon.exe. Do you actually need arbitrary relative paths, or just "the same place as dbus-daemon.exe"? I wonder whether it'd be more useful to have different semantics (with the advantage that they make sense on Unix too), something like this: If the Exec line in the service file is absolute, only that exact path will be used. On the system bus, only absolute paths are supported, and all other activations will fail.[1] [1]: The system bus is security-sensitive. If the Exec line in the service file is relative and contains a directory separator, it is interpreted as relative to the directory containing the dbus-daemon executable. On operating systems where that directory cannot be determined[2], activation fails. [2]: The directory can be determined on Windows and Linux, but not on several other Unix-based operating systems. If the Exec line in the service file does not contain a directory separator at all, the dbus-daemon searches the directory containing the dbus-daemon executable (if it can be determined), followed by the system search path specified by the PATH environment variable. (Rationale: I'd expect Exec=something-in-slash-bin to work.) Perhaps if execution fails on Windows, it should also retry with ".exe" appended? I don't know.
D-Bus service files are based on the Desktop Entry Specification, which also has an Exec line. Here's what they have to say about it: "The executable program can either be specified with its full path or with the name of the executable only. If no full path is provided the executable is looked up in the $PATH environment variable used by the desktop environment." - <http://standards.freedesktop.org/desktop-entry-spec/latest/ar01s06.html> I think that's how name-only should work on Unix. I could be persuaded that on Windows, you should look in the directory containing dbus-daemon.exe before searching $PATH. I'm not sure that relative paths containing a directory separator should be allowed at all - it seems as though whatever we make them mean, it'll often be surprising.
Created attachment 84012 [details] [review] Use executable path as relative file root
Created attachment 84014 [details] [review] On windows add executable extension if not present
Created attachment 84015 [details] [review] Add documentation for relative pathes in service files
(In reply to comment #10) > D-Bus service files are based on the Desktop Entry Specification, which also > has an Exec line. Here's what they have to say about it: > > "The executable program can either be specified with its full path or with > the name of the executable only. If no full path is provided the executable > is looked up in the $PATH environment variable used by the desktop > environment." > - <http://standards.freedesktop.org/desktop-entry-spec/latest/ar01s06.html> > > I think that's how name-only should work on Unix. I could be persuaded that > on Windows, you should look in the directory containing dbus-daemon.exe > before searching $PATH. > > I'm not sure that relative paths containing a directory separator should be > allowed at all - it seems as though whatever we make them mean, it'll often > be surprising. At least KDE decided to move some internal tools into a subdirectory not in the search path, for example: /usr/lib64/kde4/libexec/filesharelist /usr/lib64/kde4/libexec/fileshareset /usr/lib64/kde4/libexec/kconf_update /usr/lib64/kde4/libexec/kdesu_stub /usr/lib64/kde4/libexec/kio_http_cache_cleaner /usr/lib64/kde4/libexec/kioslave /usr/lib64/kde4/libexec/klauncher /usr/lib64/kde4/libexec/kpac_dhcp_helpers /usr/lib64/kde4/libexec/ksendbugmail /usr/lib64/kde4/libexec/lnusertemp /usr/lib64/kde4/libexec/start_kdeinit /usr/lib64/kde4/libexec/start_kdeinit_wrapper On windows this will be <installroot>/kde4/libexec while the dbus-daemon is in <installroot>/bin which (with this patch) will result into an exec line as following: Exec=../kde4/libexec/ksendbugmail
(In reply to comment #9) > Comment on attachment 56813 [details] [review] [review] > added relative path in service files doc to dbus spec > > Review of attachment 56813 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: doc/dbus-specification.xml > @@ +4216,4 @@ > > choose one?] > > </para> > > <para> > > + Executable names may be absolute or relative. Relative pathes on Windows are > > "paths" > > @@ +4216,5 @@ > > choose one?] > > </para> > > <para> > > + Executable names may be absolute or relative. Relative pathes on Windows are > > + based on the dbus installation root. The dbus installation root is the > > "D-Bus" > > @@ +4218,5 @@ > > <para> > > + Executable names may be absolute or relative. Relative pathes on Windows are > > + based on the dbus installation root. The dbus installation root is the > > + grandparent directory of the dbus daemon executable if it is located in a bin/ > > + subdirectory, otherwise the parent directory. > > This could really do with an example. > > I'd guess that what you're trying to achieve is: > > C:\MyApp\bin\dbus-daemon.exe -> search relative to C:\MyApp > > C:\MyApp\dbus-daemon.exe -> also search relative to C:\MyApp > > but I'd have phrased that as more like: > > Relative paths on Windows are interpreted as relative to the D-Bus > installation root, which is found by looking up the folder containing the > dbus-daemon executable. If that folder is named "bin", its parent folder is > the installation root; otherwise, the folder containing dbus-daemon.exe is > the installation root. > > (Without the context of knowing (for instance) how g_get_system_data_dirs() > is documented, I'd have interpreted "grandparent" as going from > C:\MyApp\bin\dbus-daemon.exe to C:\.) > > @@ +4222,5 @@ > > + subdirectory, otherwise the parent directory. > > + </para> > > + <para> > > + Relative pathes on unix are based on the current working directory of the dbus > > + daemon executable. > > Whether this is what currently happens or not, I don't think this is useful. > On Unix, the system dbus-daemon's current working directory is typically /, > and the session dbus-daemon's current working directory is typically the > user's home directory. > > @@ +4227,5 @@ > > + </para> > > + <para> > > + Relative executable pathes in service files are useful on operating systems or > > + environments where the path dbus daemon is running from differs from the path > > + dbus is build for. Examples are the Windows operating system or portable installations. > > What would you typically write in your .service file? Presumably > "Exec=bin/my-service.exe" for a Unix-style installation using bin/ lib/ > share/ and so on, and "Exec=my-service.exe" for an installation with > everything in the same folder? > > It seems strange that you have to say "bin/" in the Unix-style case, but not > in the everything-in-the-same-folder case, when in both cases the executable > is next to dbus-daemon.exe. make sense, fixed to use dbus-daemon path as root. > > Do you actually need arbitrary relative paths, or just "the same place as > dbus-daemon.exe"? both > > I wonder whether it'd be more useful to have different semantics (with the > advantage that they make sense on Unix too), something like this: > > If the Exec line in the service file is absolute, only that exact path will > be used. On the system bus, only absolute paths are supported, and all other > activations will fail.[1] > > [1]: The system bus is security-sensitive. is this implemented yet on unix like os ? > If the Exec line in the service file is relative and contains a directory > separator, it is interpreted as relative to the directory containing the > dbus-daemon executable. On operating systems where that directory cannot be > determined[2], activation fails. is said in the doc patch, with other words. > > [2]: The directory can be determined on Windows and Linux, but not on > several other Unix-based operating systems. > > If the Exec line in the service file does not contain a directory separator > at all, the dbus-daemon searches the directory containing the dbus-daemon > executable (if it can be determined), > followed by the system search path specified by the PATH environment variable. not implemented yet, using path variable may open security holes. > > (Rationale: I'd expect Exec=something-in-slash-bin to work.) > > Perhaps if execution fails on Windows, it should also retry with ".exe" > appended? I don't know. .exe will be appended if not present by default, see second patch
(In reply to comment #15) > > If the Exec line in the service file is absolute, only that exact path will > > be used. On the system bus, only absolute paths are supported, and all other > > activations will fail.[1] > > > > [1]: The system bus is security-sensitive. > > is this implemented yet on unix like os ? The activation helper for the system bus uses execv(), so it will not search the $PATH, but it will not currently treat relative filenames as special: I think this means they're treated as relative to the dbus-daemon's getcwd(), which is "/" in practice. > > followed by the system search path specified by the PATH environment variable. > > not implemented yet, using path variable may open security holes. The session dbus-daemon is run by the user. How can its environment be influenced by an attacker? On Unix, the system dbus-daemon is run by root and drops privileges to a system user (messagebus on Debian/Ubuntu; perhaps dbus or something else on other systems); if its environment can be influenced by an attacker, something is very wrong. On Windows, there is no system dbus-daemon (and if you wanted to add one, it would be necessary to design it and its security model first). It would probably be good if our Exec= behaved the same as .desktop files, at least on Unix (which would probably require getting some clarifications in the .desktop spec).
Comment on attachment 84014 [details] [review] On windows add executable extension if not present Review of attachment 84014 [details] [review]: ----------------------------------------------------------------- Any remaining problems here ?
Comment on attachment 84012 [details] [review] Use executable path as relative file root Review of attachment 84012 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-spawn-win.c @@ +560,5 @@ > > env_string = build_env_string(envp); > > +#ifndef DBUS_WINCE > + // handle relative pathes D-Bus is (approximately) C89, so: /* handle relative paths */ (also, that's how you spell "paths") @@ +561,5 @@ > env_string = build_env_string(envp); > > +#ifndef DBUS_WINCE > + // handle relative pathes > + if (strlen(name) > 2 && name[0] != '\\' && name[0] != '/' && name[1] != ':') One possibility would be to only run this logic if name does not contain a "/", "\" or ":" anywhere (i.e. the circumstances under which execvp() would search $PATH). @@ +570,5 @@ > + > + _dbus_string_init (&app_name); > + _dbus_verbose ("spawning relative path '%s'\n", name); > + > + if (!_dbus_get_executable_path (app_path, sizeof (app_path))) "app_path" is a confusing name for this, since it is the path to the dbus-daemon executable, not to app_name. dbus_daemon_path? our_exe_path? @@ +594,5 @@ > + _dbus_string_free (&app_name); > + } > + else > +#endif > + strncpy (exe_path, name, MAX_PATH); I was about to say "strncpy? DBusString, please", but it looks as though SearchPathA's API makes that awkward (you'd have to extend the DBusString to some arbitrary length first) so I'll let you off. ::: dbus/dbus-sysdeps-win.c @@ +3459,5 @@ > } > > + > +/** > + * return the absolute path of the current executable Please say specifically that the returned path is guaranteed to end with a path-separator, if that's something you're relying on elsewhere @@ +3466,5 @@ > + * @param len length of buffer > + * @returns #FALSE on failure > + */ > +dbus_bool_t > +_dbus_get_executable_path(char *path, int len) I'm tempted to say this should "return" a DBusString, but I realise GetModuleFileNameA() doesn't make that easy, since you'd have to extend the DBusString to some arbitrary length anyway. @@ +3468,5 @@ > + */ > +dbus_bool_t > +_dbus_get_executable_path(char *path, int len) > +{ > + //To find the path, we cut the filename /* To find ... filename */ @@ +3480,5 @@ > + *path = '\0'; > + return FALSE; > + } > + > + lastSlash = _mbsrchr(path, '\\'); Is path guaranteed to be in the form C:\foo\bar\dbus-daemon.exe, and never C:/foo/bar/dbus-daemon.exe or a mixture? @@ +3487,5 @@ > + *path = '\0'; > + return FALSE; > + } > + > + //cut off file name /* */
Comment on attachment 84014 [details] [review] On windows add executable extension if not present Review of attachment 84014 [details] [review]: ----------------------------------------------------------------- I'm not sure about this. I feel as though the path in the .service should already include ${EXEEXT}. On Windows, when KDE is starting a .desktop file (does it ever do that?), does it append ${EXEEXT}?
Comment on attachment 84015 [details] [review] Add documentation for relative pathes in service files Review of attachment 84015 [details] [review]: ----------------------------------------------------------------- ::: doc/dbus-specification.xml @@ +4677,5 @@ > + will be added internally. > + </para> > + <para> > + Executable names may be absolute or relative. Relative pathes are based on the > + current working directory of the dbus daemon executable. This is not what you implemented. What you implemented is that relative paths are relative to the directory where dbus-daemon.exe is installed. This should say "On Windows, ..." if it is not implemented on Unix, which (as far as I know) nobody has.
I still think we need a good design (for Windows and Unix) before rushing into implementing semantics for relative executable paths, and in particular, I still think this: (In reply to comment #16) > It would probably be good if our Exec= behaved the same as .desktop files, > at least on Unix (which would probably require getting some clarifications > in the .desktop spec). Here is what the desktop entry spec says: """ The executable program can either be specified with its full path or with the name of the executable only. If no full path is provided the executable is looked up in the $PATH environment variable used by the desktop environment. """ So "Exec=foo" in a .desktop file should look up foo in $PATH. At the moment, "Exec=foo" in a .service file doesn't. If you think it would be good to extend a $PATH search on Windows with "on Windows, we always behave as if $PATH started with the directory containing dbus-daemon.exe" then that seems reasonable, since, if I remember correctly, that's the same logic that Windows uses when you execute an unqualified "foo.exe"? It is not clear what this means for paths that contain a path separator, but are not absolute, e.g. "Exec=../bin/foo". It would be good if .desktop and .service were consistent in this respect.
(In reply to comment #21) > I still think we need a good design (for Windows and Unix) before rushing > into implementing semantics for relative executable paths I've brought it up on the dbus and xdg lists.
(In reply to comment #19) > Comment on attachment 84014 [details] [review] [review] > On windows add executable extension if not present > > Review of attachment 84014 [details] [review] [review]: > ----------------------------------------------------------------- > > I'm not sure about this. I feel as though the path in the .service should > already include ${EXEEXT}. > > On Windows, when KDE is starting a .desktop file (does it ever do that?), > does it append ${EXEEXT}? here is an example from <install-root>\share\applications\kde4\kolf.desktop Type=Application Exec=kolf %U kdecore adds '.exe' if not present in case of exe resource type, see https://projects.kde.org/projects/kde/kdelibs/repository/entry/kdecore/kernel/kstandarddirs.cpp?rev=Active%2FTwo#L547
(In reply to comment #23) > kdecore adds '.exe' if not present in case of exe resource type Thanks, that's useful information.
See also https://lists.freedesktop.org/archives/dbus/2014-September/016330.html and I've just raised the same thing on the xdg and dbus lists again, after discussion at the GTK hackfest unanimously agreed on what Thiago proposed back in 2014. This is not the same as what Ralf has implemented in these patches: instead of resolving relative to the dbus-daemon, I would prefer to resolve relative to the .desktop or .service file as Thiago proposed (justification: that gives each .desktop or .service file a single consistent interpretation which doesn't depend on who is reading it). Please send any comments on the specification part to the dbus and xdg mailing list threads, not here, and we can use this bug for the implementation when we have consensus on a specification.
(In reply to Ralf Habacker from comment #23) > kdecore adds '.exe' if not present in case of exe resource type I think the right thing to do about this is to try with the literal contents of the Exec line, and if that fails on Windows, append .exe and retry. That can be orthogonal to how we interpret non-absolute paths.
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/dbus/dbus/issues/61.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.