I accidentally created a file in dbus-1/services/*.service containing just this single line: SystemdService=foo.service All dbus-daemon instances promptly crashed. This bug is present in dbus 1.6.8 and latest Git master (53c593b07df0f69). Backtrace: #0 new_line (parser=<optimized out>) at desktop-file.c:327 section = 0xffffffffffffffe0 line = <optimized out> #1 parse_key_value (error=0x7fff41046760, parser=0x7fff41046640) at desktop-file.c:529 line_end = 26 p = <optimized out> value = 0xe0de30 "foo.service" key = {dummy1 = 0x7fbeaf821640 <main_arena>, dummy2 = 14736880, dummy3 = 0, dummy_bit1 = 0, dummy_bit2 = 0, dummy_bit3 = 0, dummy_bits = 4} line = 0xe0dad8 key_end = 14 tmp = 0xe0de20 "" value_start = <optimized out> eol_len = 1 key_start = 0 #2 bus_desktop_file_load (filename=filename@entry=0x7fff41046740, error=error@entry=0x7fff41046760) at desktop-file.c:693 str = {dummy1 = 0xe0db30, dummy2 = 27, dummy3 = 35, dummy_bit1 = 0, dummy_bit2 = 0, dummy_bit3 = 0, dummy_bits = 0} parser = {data = {dummy1 = 0xe0db30, dummy2 = 27, dummy3 = 35, dummy_bit1 = 0, dummy_bit2 = 0, dummy_bit3 = 0, dummy_bits = 0}, desktop_file = 0xe0e390, current_section = -1, pos = 0, len = 27, line_num = 1} sb = {mode = 33188, nlink = 1, uid = 1000, gid = 1000, size = 27, atime = 1360864194, mtime = 1360864194, ctime = 1360864194} #3 0x00000000004059ec in update_directory (activation=activation@entry=0xe0dad0, s_dir=s_dir@entry=0xe0e160, error=error@entry=0x7fff41046a80) at activation.c:655 iter = 0xe0e0e0 dir = {dummy1 = 0xe0e070, dummy2 = 42, dummy3 = 50, dummy_bit1 = 1, dummy_bit2 = 1, dummy_bit3 = 0, dummy_bits = 0} filename = {dummy1 = 0xe0db80, dummy2 = 11, dummy3 = 19, dummy_bit1 = 0, dummy_bit2 = 0, dummy_bit3 = 0, dummy_bits = 0} desktop_file = <optimized out> tmp_error = {name = 0x0, message = 0x0, dummy1 = 1, dummy2 = 0, dummy3 = 0, dummy4 = 0, dummy5 = 0, padding1 = 0xe0f610} retval = 0 entry = <optimized out> full_path = {dummy1 = 0xe0def0, dummy2 = 54, dummy3 = 100, dummy_bit1 = 0, dummy_bit2 = 0, dummy_bit3 = 0, dummy_bits = 0} #4 0x0000000000405d3c in bus_activation_reload (activation=activation@entry=0xe0dad0, address=address@entry=0x7fff41046890, directories=directories@entry=0xe0b400, error=error@entry=0x7fff41046a80) at activation.c:864 s_dir = 0xe0e160 link = 0xe0dc48 dir = 0xe0e070 "/home/grawity/.local/share/dbus-1/services" #5 0x0000000000405ea0 in bus_activation_new (context=context@entry=0xe0a0c0, address=0x7fff41046890, directories=0xe0b400, error=0x7fff41046a80) at activation.c:901 activation = 0xe0dad0 #6 0x0000000000407317 in process_config_every_time (context=context@entry=0xe0a0c0, parser=parser@entry=0xe0b3b0, is_reload=is_reload@entry=0, error=error@entry=0x7fff41046a80) at bus.c:598 full_address = {dummy1 = 0xe0f790, dummy2 = 72, dummy3 = 80, dummy_bit1 = 0, dummy_bit2 = 0, dummy_bit3 = 0, dummy_bits = 0} link = <optimized out> dirs = 0xe0b400 addr = 0x0 servicehelper = 0x0 s = 0x0 retval = 0 #7 0x00000000004081d3 in bus_context_new (config_file=config_file@entry=0x7fff41046a00, flags=flags@entry=(BUS_CONTEXT_FLAG_FORK_NEVER | BUS_CONTEXT_FLAG_WRITE_PID_FILE), print_addr_pipe=print_addr_pipe@entry=0x7fff410469e0, print_pid_pipe=print_pid_pipe@entry=0x7fff410469f0, address=<optimized out>, error=error@entry=0x7fff41046a80) at bus.c:768 context = 0xe0a0c0 parser = <optimized out> #8 0x000000000040443e in main (argc=<optimized out>, argv=<optimized out>) at main.c:600 error = {name = 0x0, message = 0x0, dummy1 = 1, dummy2 = 1, dummy3 = 1, dummy4 = 1, dummy5 = 1, padding1 = 0x7fff41046aff} config_file = {dummy1 = 0xe0a090, dummy2 = 24, dummy3 = 32, dummy_bit1 = 0, dummy_bit2 = 0, dummy_bit3 = 0, dummy_bits = 0} address = {dummy1 = 0xe0a030, dummy2 = 0, dummy3 = 8, dummy_bit1 = 0, dummy_bit2 = 0, dummy_bit3 = 0, dummy_bits = 0} addr_fd = {dummy1 = 0xe0a050, dummy2 = 0, dummy3 = 8, dummy_bit1 = 0, dummy_bit2 = 0, dummy_bit3 = 1, dummy_bits = 0} pid_fd = {dummy1 = 0xe0a070, dummy2 = 0, dummy3 = 8, dummy_bit1 = 0, dummy_bit2 = 0, dummy_bit3 = 1, dummy_bits = 0} prev_arg = <optimized out> print_addr_pipe = {fd = -1} print_pid_pipe = {fd = -1} i = <optimized out> print_address = <optimized out> print_pid = <optimized out> flags = <optimized out>
Created attachment 80115 [details] [review] Fix dbus-deamon crash due to invalid service file This bug caused by invalid service file, whih key/value starts without any section, so in new_line(), it will try to access invalid address.
Comment on attachment 80115 [details] [review] Fix dbus-deamon crash due to invalid service file Review of attachment 80115 [details] [review]: ----------------------------------------------------------------- ::: bus/desktop-file.c @@ +322,5 @@ > BusDesktopFileSection *section; > BusDesktopFileLine *line; > > + if (parser == NULL || parser->current_section < 0) > + return NULL; Is it valid for parser to be NULL here? If not, don't check for it. The only caller of new_line() assumes that returning NULL means out-of-memory: if (line == NULL) { dbus_free (value); parser_free (parser); BUS_SET_OOM (error); return FALSE; } so if you want new_line() to be able to fail, it will need to take a DBusError argument so it can signal the right error. However, I think a better way to fix this would be to change parse_key_value() so that it raises an error if current_section < 0 instead of calling new_line(), or even to change bus_desktop_file_load() so that it doesn't call parse_key_value() before it has opened the first section. Something like this pseudocode: if (/* it's a new section heading */) ... else if (/* .. it's a comment or blank */) ... else if (parser.current_section < 0) { raise error FAILED, "invalid desktop file: key=value before [Section]" } else { ... parse_key_value (...) ... }
(In reply to comment #2) > Comment on attachment 80115 [details] [review] [review] > Fix dbus-deamon crash due to invalid service file > > Review of attachment 80115 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: bus/desktop-file.c > @@ +322,5 @@ > > BusDesktopFileSection *section; > > BusDesktopFileLine *line; > > > > + if (parser == NULL || parser->current_section < 0) > > + return NULL; > > Is it valid for parser to be NULL here? If not, don't check for it. OK, will drop it in v2. > > The only caller of new_line() assumes that returning NULL means > out-of-memory: > > if (line == NULL) > { > dbus_free (value); > parser_free (parser); > BUS_SET_OOM (error); > return FALSE; > } > > so if you want new_line() to be able to fail, it will need to take a > DBusError argument so it can signal the right error. > > However, I think a better way to fix this would be to change > parse_key_value() so that it raises an error if current_section < 0 instead > of calling new_line(), or even to change bus_desktop_file_load() so that it > doesn't call parse_key_value() before it has opened the first section. Yes, a simple check is already there, however, I'll upload v2 with raise an error like below you suggested. > Something like this pseudocode: > > if (/* it's a new section heading */) > ... > else if (/* .. it's a comment or blank */) > ... > else if (parser.current_section < 0) > { > raise error FAILED, "invalid desktop file: key=value before [Section]" > } > else > { > ... parse_key_value (...) ... > }
Created attachment 80380 [details] [review] [PATCH v2] Fix dbus-daemon crash due to invalid service file
Applied, will be in 1.7.4.
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.