| Summary: | Crash while reading service files with missing parameters | ||
|---|---|---|---|
| Product: | dbus | Reporter: | Mantas Mikulėnas <grawity> |
| Component: | core | Assignee: | Havoc Pennington <hp> |
| Status: | RESOLVED FIXED | QA Contact: | |
| Severity: | normal | ||
| Priority: | medium | CC: | chengwei.yang.cn |
| Version: | unspecified | Keywords: | patch |
| Hardware: | Other | ||
| OS: | All | ||
| Whiteboard: | review- | ||
| i915 platform: | i915 features: | ||
| Attachments: |
Fix dbus-deamon crash due to invalid service file
[PATCH v2] Fix dbus-daemon crash due to invalid service file |
||
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.
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>