| Summary: | crash when calling CreateAccount with a malformed property | ||
|---|---|---|---|
| Product: | Telepathy | Reporter: | Tomeu Vizoso <tomeu> |
| Component: | mission-control | Assignee: | Telepathy bugs list <telepathy-bugs> |
| Status: | RESOLVED FIXED | QA Contact: | Telepathy bugs list <telepathy-bugs> |
| Severity: | major | ||
| Priority: | medium | Keywords: | patch |
| Version: | git master | ||
| Hardware: | Other | ||
| OS: | All | ||
| URL: | http://git.collabora.co.uk/?p=user/tomeu/telepathy-mission-control.git;a=shortlog;h=refs/heads/bug-27021 | ||
| Whiteboard: | review+ | ||
| i915 platform: | i915 features: | ||
Not sure how to test this condition, suggestions most welcome. I don't think this patch is right. If set_error is non-NULL, you'll copy it into cad->error, then use g_set_error to put an MCD_ACCOUNT_MANAGER_ERROR into cad->error, leading to a GLib assertion failure. What you probably want to do is to have set_new_account_properties() put its error into a temporary variable, which is then propagated into cad->error? Or don't call g_set_error if cad->error is already non-NULL? Or refactor the function so that cad->ok and cad->error are always set at the same time, reducing confusion considerably? Or something. Reworked branch following the comments Can you reproduce this crash in a regression test by attempting to set a nonexistent parameter name over D-Bus, for instance?
(To be honest, your stack trace looks as though there's been a missing NUL termination further down the stack, so you're fixing symptoms rather than causes. However, if the same symptom can be caused in a valid way, there's no harm in fixing *that* bug.)
This patch is rather less obvious than it could be... it might well be *valid*, but I think it'd be considerably clearer if expressed like this:
> refactor the
> function so that cad->ok and cad->error are always set at the same time
Something like this:
cad->ok = TRUE;
if (set_error != NULL)
{
cad->ok = FALSE;
/* moved from [A] */
g_set_error (&cad->error, blah blah blah, set_error->message);
}
if (cad->ok && cad->properties != NULL)
{
cad->ok = ... as before ... (&cad->error);
}
if (cad->ok)
{
... as before ...
}
else
{
/* [A] */
complete_account_creation_finish (account, TRUE, cad);
}
(The comments mentioning [A] wouldn't be committed, obviously.)
(For a relatively simple change like this that's been rewritten during review, I'd also be inclined to rebase your branch on master then squash the various attempts at it into a single patch, rather than having a branch containing a wrong patch, a revert, a merge from master, etc.) (In reply to comment #4) > Can you reproduce this crash in a regression test by attempting to set a > nonexistent parameter name over D-Bus, for instance? A non-existent property name makes it crash, yes: diff --git a/test/twisted/account-manager/create-with-properties.py b/test/twisted/account-manager/create-with-properties.py index bfd5b2b..3849524 100644 --- a/test/twisted/account-manager/create-with-properties.py +++ b/test/twisted/account-manager/create-with-properties.py @@ -71,6 +71,7 @@ def test(q, bus, mc): creation_properties = dbus.Dictionary({ cs.ACCOUNT + '.Enabled': True, + cs.ACCOUNT + '.NonExistent': 'foo', cs.ACCOUNT + '.AutomaticPresence': dbus.Struct(( dbus.UInt32(cs.PRESENCE_TYPE_BUSY), 'busy', 'Exploding'), signature='uss'), What would be better, a new test in its own module, or one more test inside account-manager/create-with-properties.py ? > (To be honest, your stack trace looks as though there's been a missing NUL > termination further down the stack, so you're fixing symptoms rather than > causes. However, if the same symptom can be caused in a valid way, there's no > harm in fixing *that* bug.) I'm confused, AFAICS the crash is caused because when the parameters are valid but the properties are invalid, the code incorrectly tries to propagate the params error, which doesn't exist. > This patch is rather less obvious than it could be... it might well be *valid*, > but I think it'd be considerably clearer if expressed like this: > > > refactor the > > function so that cad->ok and cad->error are always set at the same time > > Something like this: > > cad->ok = TRUE; > > if (set_error != NULL) > { > cad->ok = FALSE; > /* moved from [A] */ > g_set_error (&cad->error, blah blah blah, set_error->message); > } > > if (cad->ok && cad->properties != NULL) > { > cad->ok = ... as before ... (&cad->error); > } > > if (cad->ok) > { > ... as before ... > } > else > { > /* [A] */ > complete_account_creation_finish (account, TRUE, cad); > } > > (The comments mentioning [A] wouldn't be committed, obviously.) Sounds good, thanks. Thanks, that looks much better. Please commit (Tollef has set up ACLs so people in group 'telepathy', like you, should be able to commit to MC). This appears to be fixed in master. |
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.
Program received signal SIGSEGV, Segmentation fault. 0x00a2ca59 in complete_account_creation_set_cb (account=0x9269010, not_yet=0x9267190, set_error=0x0, user_data=0x925aba0) at mcd-account-manager.c:455 455 g_set_error (&cad->error, MCD_ACCOUNT_MANAGER_ERROR, (gdb) bt full #0 0x00a2ca59 in complete_account_creation_set_cb (account=0x9269010, not_yet=0x9267190, set_error=0x0, user_data= 0x925aba0) at mcd-account-manager.c:455 cad = 0x925aba0 account_manager = 0x9258768 #1 0x00a251a7 in set_parameters_finish (data=0x925ec68) at mcd-account.c:2295 priv = 0x9269020 __PRETTY_FUNCTION__ = "set_parameters_finish" #2 0x00a2527f in set_parameters_set_single (account=0x9269010, error=0x0, user_data=0x925ec68) at mcd-account.c:2376 data = 0x925ec68 name = 0x4ca525 "\205\300t\b\215d$ [^]Í\203h\311\372\377\211t$\020\211D$\f\307D$\004\004" value = 0xbfcecd18 #3 0x00a256f8 in set_parameters_iter_param (account=0x0, ret_value=0x0, error=0x0, user_data=0x925ec68) at mcd-account.c:2465 data = 0x925ec68 out_error = 0x0 __PRETTY_FUNCTION__ = "set_parameters_iter_param" #4 0x00a25564 in set_parameters_iter_param (account=0x0, ret_value=0x0, error=0x0, user_data=0x925ec68) at mcd-account.c:2453 type = 64 data = 0x925ec68 out_error = 0x0 __PRETTY_FUNCTION__ = "set_parameters_iter_param" #5 0x00a25564 in set_parameters_iter_param (account=0x0, ret_value=0x0, error=0x0, user_data=0x925ec68) at mcd-account.c:2453 type = 64 data = 0x925ec68 out_error = 0x0 __PRETTY_FUNCTION__ = "set_parameters_iter_param" #6 0x00a25564 in set_parameters_iter_param (account=0x0, ret_value=0x0, error=0x0, user_data=0x925ec68) at mcd-account.c:2453 type = 64 data = 0x925ec68 out_error = 0x0 __PRETTY_FUNCTION__ = "set_parameters_iter_param" #7 0x00a25564 in set_parameters_iter_param (account=0x0, ret_value=0x0, error=0x0, user_data=0x925ec68) at mcd-account.c:2453 type = 64 data = 0x925ec68 out_error = 0x0 __PRETTY_FUNCTION__ = "set_parameters_iter_param" #8 0x00a25564 in set_parameters_iter_param (account=0x0, ret_value=0x0, error=0x0, user_data=0x925ec68) at mcd-account.c:2453 type = 64 data = 0x925ec68 out_error = 0x0 __PRETTY_FUNCTION__ = "set_parameters_iter_param" #9 0x00a25564 in set_parameters_iter_param (account=0x0, ret_value=0x0, error=0x0, user_data=0x925ec68) at mcd-account.c:2453 type = 64 data = 0x925ec68 out_error = 0x0 __PRETTY_FUNCTION__ = "set_parameters_iter_param" #10 0x00a258de in _mcd_account_set_parameters (account=0x9269010, params=Traceback (most recent call last): File "/usr/share/glib-2.0/gdb/glib.py", line 126, in children return self._iterator(self.val, self.keys_are_strings) File "/usr/share/glib-2.0/gdb/glib.py", line 79, in __init__ self.array = ht["nodes"] RuntimeError: There is no member named nodes. 0x9251950, unset=0x0, callback=0xa2c8b0 <complete_account_creation_set_cb>, user_data=0x925aba0) at mcd-account.c:2536 priv = <value optimized out> param = 0x926be80 not_yet = <value optimized out> error = 0x0 unset_size = <value optimized out> __PRETTY_FUNCTION__ = "_mcd_account_set_parameters" #11 0x00a2c11d in complete_account_creation (account=0x9269010, cb_error=0x0, user_data=0x925aba0) at mcd-account-manager.c:484 cad = 0x925aba0 account_manager = <value optimized out> #12 0x00a38a1a in mcd_object_invoke_ready_callbacks (rd=0x9264c18, error=0x0) at mcd-misc.c:112 cb = <value optimized out> list = 0x9264ca8 = {0x9264ca0, 0x9264c28} #13 0x00a38af4 in _mcd_object_ready (object=0x9269010, quark=240, error=0x0) at mcd-misc.c:177 rd = 0x9264c18 #14 0x00a26c3e in mcd_account_loaded (account=0x9269010) at mcd-account.c:397 __PRETTY_FUNCTION__ = "mcd_account_loaded" #15 0x00a226ef in check_parameters_get_param_cb (account=0x9269010, value=0x0, error=0x9265180, user_data=0x9265170) at mcd-account.c:2158 data = 0x9265170 p = <value optimized out> #16 0x00a23826 in get_parameter_from_file (account=0x9269010, name=<value optimized out>, callback= 0xa226b0 <check_parameters_get_param_cb>, user_data=0x9265170) at mcd-account.c:938 priv = <value optimized out> key = "param-first-name\000\364\311$\000z\004/\000p\276&\tX\371%\t\304ϼ\000\036\275\241\000\001\000\000\000\060\322οeû\000pS2\000H\000\000\000\300\266&\tH\322ο\251l+\000p\276&\tp\276&\t\001\000\000\000\000`\241\000\364\311$\000\264\271\245\000\000\n&\t$\000\000\000x\322ο0&\274\000\300\266&\t\264\271\245\000\000\n&\tp\276&\tx\322ο\243+\242\000\300\266&\tp\276&\t\260&\242\000p\276&\t\350\322ο0&\274\000 \220&\t\264\271\245\000\260&\242\000\264\271\245\000\350\322ο\225\215\242\000\000\n&\tH\351%\tp\276&\t\b", '\000' <repeats 11 times>, "\\\371H\000\001\000\000\000\264\271\245\000\377\377\377\377\304ϼ\000Ǒ\241\000\001\000\000\000\360\322ο(9W\000 \220&\t@\000\000\000pQ&\t\260&\242" param = <value optimized out> error = 0x9265180 value = 0x0 type = 64 #17 0x00a2274e in mcd_account_get_parameter (account=0x0, value=0x0, error=0x0, user_data=0x9265170) at mcd-account.c:2118 No locals. #18 check_parameters_get_param_cb (account=0x0, value=0x0, error=0x0, user_data=0x9265170) at mcd-account.c:2153 data = 0x9265170 p = <value optimized out> #19 0x00a22815 in mcd_account_check_parameters (account=0x9269010, callback=0xa26db0 <manager_ready_check_params_cb>, user_data=0x0) at mcd-account.c:2188 priv = <value optimized out> param = 0x926be80 data = <value optimized out> __PRETTY_FUNCTION__ = "mcd_account_check_parameters" #20 0x00a38a1a in mcd_object_invoke_ready_callbacks (rd=0x9264cd0, error=0x0) at mcd-misc.c:112 cb = <value optimized out> list = 0x9264c20 = {0x9264cc8} #21 0x00a38af4 in _mcd_object_ready (object=0x9260a00, quark=242, error=0x0) at mcd-misc.c:177 rd = 0x9264cd0 #22 0x00a3d43e in on_manager_ready (tp_conn_mgr=0x9264488, error=0x0, user_data=0x0, weak_object=0x9260a00) at mcd-manager.c:93 manager = 0x9260a00 priv = 0x9260a28 __PRETTY_FUNCTION__ = "on_manager_ready" #23 0x002b7555 in tp_connection_manager_ready_or_failed (self=<value optimized out>, error=0x0) at connection-manager.c:314 c = 0x9267280 waiters = 0x926b4a0 = {0x9267280} link = <value optimized out> __PRETTY_FUNCTION__ = "tp_connection_manager_ready_or_failed" #24 0x002ba71b in tp_connection_manager_idle_read_manager_file (data=0x9264488) at connection-manager.c:1188 error = 0x0 protocols = 0x9267178 self = 0x9264488 __PRETTY_FUNCTION__ = "tp_connection_manager_idle_read_manager_file" #25 0x004c0172 in ?? () from /lib/libglib-2.0.so.0 No symbol table info available. #26 0x004c1f88 in g_main_context_dispatch () from /lib/libglib-2.0.so.0 No symbol table info available. #27 0x004c58b8 in ?? () from /lib/libglib-2.0.so.0 No symbol table info available. #28 0x004c5d2f in g_main_loop_run () from /lib/libglib-2.0.so.0 No symbol table info available. #29 0x00a4ae40 in mcd_service_run (self=0x925c410) at mcd-service.c:195 No locals. #30 0x08048b17 in main (argc=1, argv=0xbfced764) at mc-server.c:78 mcd = 0x925c410