GNOME Bugzilla – Bug 471791
Move away from g_asserts to g_ret*
Last modified: 2015-06-17 10:16:15 UTC
Make really non-fatal things to critical warnings. Im gonna make this a tracker with lots of patches.
Created attachment 94624 [details] [review] Proposed patch
Created attachment 94626 [details] [review] Proposed patch
(In reply to comment #1) > Created an attachment (id=94624) [edit] > Proposed patch > On some places I can be too paranoic, but please consider those comments, like a hint, at least. In mail_vfolder_add_uri is checked for non-NULL rule->name, but in mail_vfolder_delete_uri isn't. I know, it was there before, but it should be safer to add there this check too, because you are trying to print that name. Same for mail_vfolder_rename_uri. Calls of xml_encode (em-vfolder-rule.c, em-filter-rule.c) didn't expect NULL as returned value, it should cause troubles in bad cases. In em_format_add_puri: "size (%d)less than size of puri" add there a space ;) and who will free that new puri if one of those three statements got used? Also some of the calls to this function trusts it returns non-NULL values (not mentioning debug only assignments of the returned value in calls). Please check if mail_config_control_factory_cb can return NULL (I don't know how it works with 'factory' function.) Also keep good coding style here (and on some other places too) ;) Calls of mail_component_get_folder, mail_component_get_folder_uri, don't always check for non-NULL return value, but these can be safe for now, I think? Why that return at ml_tree_set_value_at?
(In reply to comment #2) > Created an attachment (id=94626) [edit] > Proposed patch > Some minor coding style issues, but nothing important. I think at table/e-table-config.c:configure_combo_box_add is not check for "g_return_if_fail (reference != NULL);" necessary, because if I recall it correctly, then if the GLib will not be able allocate memory, then the program stops automatically. Changes in e-timezone-dialog/e-timezone-dialog.c may cause crash because the calls of get_zone_from_point didn't check for non-NULL values.
When you will make this for camel_nntp, close bug #378121 (or do something similar with it).
Milan correct a few things. I think it the caller crashes, that is sort of uncovering the real bug. May be we need to take them separately after the bugs :) Im doing at my own pace, and feel free to add to my effort by jumping into EDS before me :)
There is only one pity thing about your new approach. I don't think the crash with SIGSEGV is better than g_assert crash. g_assert crash is cleaner from my point of view.
Im not sure how to differentiate between the both, but Im sure that both the crashers developers can fix, but mine would happen on a debug/developer env only.
Created attachment 94690 [details] [review] Calendar assert conversions
Created attachment 94691 [details] [review] Addressbook Patch
Created attachment 94692 [details] [review] Composer patch
Created attachment 94695 [details] [review] Shell patch
Created attachment 94697 [details] [review] filter patch
(In reply to comment #9) > Created an attachment (id=94690) [edit] > Calendar assert conversions > These are only minor issues and advices, nothing important. I even didn't look on all places for possible leaks, only a few. gui/tasks-control.c: sensitize_items: minor coding style issue and should be good to write there a command name to see which one is wrong. gui/calendar-commands.c: sensitize_items: same as above. (typo 'itemS', not 'item') gui/cal-search-bar.c: free_categories: should be better to 'continue' and free the array too. Other code seemed to check for pdata[i] for NULL, so it can happen sometimes? Probably not, but be sure we don't leak. gui/comp-editor-factory.c: free_request: same as above, just a non-probably-able leak. gui/gnome-cal.c: open_ecal: please add g_free (msg); right after 'default' gui/e-tasks.c: e_tasks_setup_view_menus/e_tasks_discard_view_menus: double check for "g_return_if_fail (priv->view_instance == NULL);", you did this blindly, right? ;) gui/e-week-view-layout.c: e_week_view_find_day: minor coding style issue ------------------------------------- Can cause SIGSEGV: calls for e_calendar_table_get_selected_comp should be checked better for NULL return (it wasn't even before this patch) ------------------------------------- Blocker: ;) gui/dialogs/event-page.c: alarm_custom_clicked_cb: negate that expression (minor, use NULL not null in a string)
(In reply to comment #10) > Created an attachment (id=94691) [edit] > Addressbook Patch > Advice: printing/e-contact-print.c: contact_compare: it should be better return zero when both are NULL, otherwise, in some sorting algorithm implementations it can stuck in an endless loop. ------------------------------------------- Blockers: gui/contact-editor/e-contact-editor.c: weird change here, not supposed to be here gui/component/addressbook-config.c: source_to_uri_parts: that's not the way we need it, I suppose :)
(In reply to comment #11) > Created an attachment (id=94692) [edit] > Composer patch > only two minor coding-style issues at e-msg-composer.c: update_auto_recipients
(In reply to comment #12) > Created an attachment (id=94695) [edit] > Shell patch > minor coding style issues at: e-shell.c:e_shell_construct, remove_dir e-shell-window-commands.c: update_offline_menu_item, offline_toggle_clicked_callback e-shell-importer.c: create_help ==================================================================== (In reply to comment #13) > Created an attachment (id=94697) [edit] > filter patch > produces new warnings ("nvoid" was "nonvoid"): rule-context.c: In function 'rule_context_load': rule-context.c:288: warning: 'return' with no value, in function returning nvoid rule-context.c: In function 'rule_context_save': rule-context.c:420: warning: 'return' with no value, in function returning nvoid rule-context.c:421: warning: 'return' with no value, in function returning nvoid -------------------------------------- Minor coding style issues: rule-context.c: rule_context_add_part_set, rule_context_add_rule_set filter-option.c: filter_option_set_current, filter_option_add -------------------------------------- Can cause SIGSEGV: call of filter_rule_clone, filter_rule_xml_encode, rule_context_create_part, rule_context_next_part, filter_option_add
Created attachment 94817 [details] [review] e-util, smime, a11y fixes
(In reply to comment #14) > (In reply to comment #9) > > Created an attachment (id=94690) [edit] > > Calendar assert conversions > > > > Can cause SIGSEGV: > > calls for e_calendar_table_get_selected_comp should be checked better for NULL > return (it wasn't even before this patch) > Left for future work. > ------------------------------------- > Blocker: ;) > > gui/dialogs/event-page.c: alarm_custom_clicked_cb: negate that expression > (minor, use NULL not null in a string) Fixed. Just noticed that this is an commented block #if 0 :) >
Most of them fixed. Can cause SEGV left for future.
(In reply to comment #18) > Created an attachment (id=94817) [edit] > e-util, smime, a11y fixes > some minor coding style issues like spaces before brackets, nothing important. minor: calls to e_config_construct doesn't check the return value, so something bad can happen. but I think only in a very bad case. Just a notice, is it better to return default in e_config_listener_get_*_with_default then just a FALSE? I think it is better, but not possible do write the message and set the flag together, somehow. ----------------------------------------------------------------- Critical warnings: Calls to e_config_create_widget should check return values or something. ----------------------------------------------------------------- Blockers: got new warnings: +e-config.c: In function 'e_config_target_new': +e-config.c:1198: warning: 'return' with no value, in function returning non-vd +e-event.c: In function 'e_event_target_new': +e-event.c:324: warning: 'return' with no value, in function returning non-void +e-popup.c: In function 'e_popup_target_new': +e-popup.c:648: warning: 'return' with no value, in function returning non-void
Should be kept open for fixing till the lowest level.
Bumping version to a stable release.
Created commit_01f3e5d in eds master (3.17.3+) [1] Created commit_973a731 in evo master (3.17.3+) [2] Created commit_c6dd505 in ews master (3.17.3+) [3] Created commit_2c90495 in ema master (3.17.3+) [4] Created commit_d003102 in eds gnome-3-16 (3.16.4+) Created commit_2cf73fd in evo gnome-3-16 (3.16.4+) Created commit_f1efb01 in ews gnome-3-16 (3.16.4+) Created commit_26ce56f in ema gnome-3-16 (3.16.4+) [1] https://git.gnome.org/browse/evolution-data-server/commit/?id=01f3e5d [2] https://git.gnome.org/browse/evolution/commit/?id=973a731 [3] https://git.gnome.org/browse/evolution-ews/commit/?id=c6dd505 [4] https://git.gnome.org/browse/evolution-mapi/commit/?id=2c90495